From 12a378c678f043ffb4f9f90500fc15fc895070cd Mon Sep 17 00:00:00 2001 From: Pavel Naberezhnev <78569117+stokescat@users.noreply.github.com> Date: Sun, 2 Nov 2025 17:45:38 +0300 Subject: [PATCH] OpenNtfsDxe: Fixed several issues in the NTFS driver (#588) * Incorrect EFI_FILE_PROTOCOL version The driver implements EFI_FILE_PROTOCOL_REVISION version, not EFI_FILE_PROTOCOL_REVISION2 version. * Fix directory reading logic by implementing EFI_FILE_INFO cache Problem: According to the UEFI specification, reading from a directory must return zero Size for the EFI_FILE_INFO structure when directory entries are exhausted. The original FileReadDir() implementation always returned a fixed EFI_FILE_INFO size before reporting end-of-directory. This caused fuzzing tests to enter an infinite directory iteration loop due to unexpected behavior. Solution: Introduced an EFI_FILE_INFO cache with the following logic: 1. FileReadDir() caches EFI_FILE_INFO on first read. The cache key combines directory path hash (with FNV-1a 64-bit hashing) and DirIndex value 2. When the key matches and buffer size is sufficient: - Data is returned from cache - Cache is cleared 3. When the key matches with insufficent buffer size: - Cache is preserved for subsequent retries 4. Key mismatch triggers cache reset Additional benefits: - Eliminates fixed MINIMUM_INFO_LENGTH requirement - FileReadDir() now requests only required buffer size Note: A more elegant solution is being considered for future NTFS driver improvements, requiring further analysis and testing. Links: FNV Hash: http://www.isthe.com/chongo/tech/comp/fnv/index.html * Fix memory leak in FileReadDir() function Function NtfsOppen() allocates additional memory for file structure. After use it, need to free by call FreeFile() function. * Fixed invalid pointer access in ReadAttr() function Solution: - Added a null check for the Current field in ReadAttr() function - Added validation for MFT record flags in the InitFile() function Signed-off-by: Pavel Naberezhnev --- Platform/OpenNtfsDxe/Cache.c | 120 +++++++++++++++++++++++++++ Platform/OpenNtfsDxe/Data.c | 7 +- Platform/OpenNtfsDxe/Disc.c | 7 ++ Platform/OpenNtfsDxe/Driver.h | 19 ++++- Platform/OpenNtfsDxe/Helper.h | 27 ++++++ Platform/OpenNtfsDxe/NTFS.c | 5 +- Platform/OpenNtfsDxe/Open.c | 48 ++++++++--- Platform/OpenNtfsDxe/OpenNtfsDxe.inf | 1 + Utilities/TestNtfsDxe/Makefile | 2 +- 9 files changed, 219 insertions(+), 17 deletions(-) create mode 100644 Platform/OpenNtfsDxe/Cache.c diff --git a/Platform/OpenNtfsDxe/Cache.c b/Platform/OpenNtfsDxe/Cache.c new file mode 100644 index 00000000..b52aed12 --- /dev/null +++ b/Platform/OpenNtfsDxe/Cache.c @@ -0,0 +1,120 @@ +/** @file + Copyright (c) 2025, Pavel Naberezhnev. All rights reserved. + SPDX-License-Identifier: BSD-3-Clause +**/ + +#include "NTFS.h" +#include "Helper.h" + +#define FNVHASH_PRIME (0x100000001B3ULL) +#define FNVHASH_OFFSET (0xCBF29CE484222325ULL) + +STATIC +UINT64 +FnvHash ( + IN CONST CHAR16 *Str + ) +{ + UINT64 Hash; + UINT64 Char; + + ASSERT (Str != NULL); + + Hash = FNVHASH_OFFSET; + while ((Char = (UINT64)*Str) != 0) { + Hash = Hash ^ (Char & 0x0FF); + Hash = Hash * FNVHASH_PRIME; + + Char = Char >> 8; + Hash = Hash ^ (Char & 0x0FF); + Hash = Hash * FNVHASH_PRIME; + + ++Str; + } + + return Hash; +} + +EFI_STATUS +NtfsCfiInit ( + IN EFI_FS *Fs + ) +{ + ASSERT (Fs != NULL); + + Fs->CFIData = AllocateZeroPool (MINIMUM_INFO_LENGTH); + if (Fs->CFIData == NULL) { + return EFI_OUT_OF_RESOURCES; + } + + // + // Set init state + // + Fs->CFIDataSize = 0; + + return EFI_SUCCESS; +} + +VOID +NtfsCfiFree ( + IN EFI_FS *Fs + ) +{ + ASSERT (Fs != NULL); + + if (Fs->CFIData != NULL) { + FreePool (Fs->CFIData); + Fs->CFIData = NULL; + } +} + +VOID +NtfsCfiPush ( + IN EFI_FS *Fs, + IN CONST CHAR16 *Path, + IN UINTN Size, + IN INT64 DirIndex + ) +{ + ASSERT (Fs != NULL); + ASSERT (Path != NULL); + + Fs->CFIHash = FnvHash (Path); + Fs->CFIDataSize = Size; + Fs->CFIDirIndex = DirIndex; +} + +INTN +NtfsCfiPop ( + IN EFI_FS *Fs, + IN CONST CHAR16 *Path, + IN UINTN Size, + IN INT64 DirIndex, + OUT VOID *Buffer + ) +{ + UINT64 Hash; + + ASSERT (Fs != NULL); + ASSERT (Path != NULL); + + if (Fs->CFIDataSize == 0) { + return 0; + } + + Hash = FnvHash (Path); + if ((Hash == Fs->CFIHash) && (DirIndex == Fs->CFIDirIndex)) { + if (Size < Fs->CFIDataSize) { + return -1; + } + + CopyMem (Buffer, Fs->CFIData, Fs->CFIDataSize); + ZeroMem (Fs->CFIData, Fs->CFIDataSize); + Fs->CFIDataSize = 0; + return 1; + } + + ZeroMem (Fs->CFIData, Fs->CFIDataSize); + Fs->CFIDataSize = 0; + return 0; +} diff --git a/Platform/OpenNtfsDxe/Data.c b/Platform/OpenNtfsDxe/Data.c index 75313bc7..49261e99 100644 --- a/Platform/OpenNtfsDxe/Data.c +++ b/Platform/OpenNtfsDxe/Data.c @@ -200,7 +200,12 @@ ReadAttr ( ASSERT (Attr != NULL); ASSERT (Dest != NULL); - Current = Attr->Current; + Current = Attr->Current; + if (Current == NULL) { + DEBUG ((DEBUG_INFO, "NTFS: Internal error! Incorrect initialization of attribute iterator\n")); + return EFI_VOLUME_CORRUPTED; + } + Attr->Next = Attr->Current; ClusterSize = Attr->BaseMftRecord->File->FileSystem->ClusterSize; diff --git a/Platform/OpenNtfsDxe/Disc.c b/Platform/OpenNtfsDxe/Disc.c index ee76b2a9..96f1cee9 100644 --- a/Platform/OpenNtfsDxe/Disc.c +++ b/Platform/OpenNtfsDxe/Disc.c @@ -751,6 +751,13 @@ InitFile ( Record = (FILE_RECORD_HEADER *)File->FileRecord; + if ((Record->Flags & ~IS_SUPPORTED_FLAGS) != 0) { + DEBUG ((DEBUG_INFO, "NTFS: MFT Record 0x%Lx has invalid or unsupported flags\n", RecordNumber)); + FreePool (File->FileRecord); + File->FileRecord = NULL; + return EFI_VOLUME_CORRUPTED; + } + if ((Record->Flags & IS_IN_USE) == 0) { DEBUG ((DEBUG_INFO, "NTFS: MFT Record 0x%Lx is not in use\n", RecordNumber)); FreePool (File->FileRecord); diff --git a/Platform/OpenNtfsDxe/Driver.h b/Platform/OpenNtfsDxe/Driver.h index ac1bf8e0..162af119 100644 --- a/Platform/OpenNtfsDxe/Driver.h +++ b/Platform/OpenNtfsDxe/Driver.h @@ -185,8 +185,15 @@ enum { /// Table 4.22. File record flags /// enum { - IS_IN_USE = 0x01, - IS_A_DIRECTORY = 0x02, + IS_IN_USE = 0x01, + IS_A_DIRECTORY = 0x02, + IS_AN_EXTENSION = 0x04, + IS_A_SPECIAL = 0x08, + + IS_SUPPORTED_FLAGS = IS_IN_USE + | IS_A_DIRECTORY + | IS_AN_EXTENSION + | IS_A_SPECIAL, }; /// @@ -649,6 +656,14 @@ typedef struct _EFI_FS { UINTN IndexRecordSize; UINTN SectorSize; UINTN ClusterSize; + + // + // CFI = Cache for FileInfo + // + UINTN CFIDataSize; + VOID *CFIData; + UINT64 CFIHash; + INT64 CFIDirIndex; } EFI_FS; typedef struct { diff --git a/Platform/OpenNtfsDxe/Helper.h b/Platform/OpenNtfsDxe/Helper.h index 92fad6f8..71c07eeb 100644 --- a/Platform/OpenNtfsDxe/Helper.h +++ b/Platform/OpenNtfsDxe/Helper.h @@ -202,4 +202,31 @@ NtfsToEfiTime ( UINT64 NtfsTime ); +EFI_STATUS +NtfsCfiInit ( + IN EFI_FS *Fs + ); + +VOID +NtfsCfiFree ( + IN EFI_FS *Fs + ); + +VOID +NtfsCfiPush ( + IN EFI_FS *Fs, + IN CONST CHAR16 *Path, + IN UINTN Size, + IN INT64 DirIndex + ); + +INTN +NtfsCfiPop ( + IN EFI_FS *Fs, + IN CONST CHAR16 *Path, + IN UINTN Size, + IN INT64 DirIndex, + OUT VOID *Buffer + ); + #endif // HELPER_H diff --git a/Platform/OpenNtfsDxe/NTFS.c b/Platform/OpenNtfsDxe/NTFS.c index bef5ebf1..ddb7819c 100644 --- a/Platform/OpenNtfsDxe/NTFS.c +++ b/Platform/OpenNtfsDxe/NTFS.c @@ -267,7 +267,7 @@ NTFSStart ( return Status; } - Instance->EfiFile.Revision = EFI_FILE_PROTOCOL_REVISION2; + Instance->EfiFile.Revision = EFI_FILE_PROTOCOL_REVISION; Instance->EfiFile.Open = FileOpen; Instance->EfiFile.Close = FileClose; Instance->EfiFile.Delete = FileDelete; @@ -286,6 +286,8 @@ NTFSStart ( return Status; } + NtfsCfiInit (Instance); + Status = gBS->InstallMultipleProtocolInterfaces ( &Controller, &gEfiSimpleFileSystemProtocolGuid, @@ -373,6 +375,7 @@ NTFSStop ( FreePool (Instance->RootIndex->FileRecord); FreePool (Instance->MftStart->FileRecord); FreePool (Instance->RootIndex->File); + NtfsCfiFree (Instance); return EFI_SUCCESS; } diff --git a/Platform/OpenNtfsDxe/Open.c b/Platform/OpenNtfsDxe/Open.c index b5b64f0a..df357cbf 100644 --- a/Platform/OpenNtfsDxe/Open.c +++ b/Platform/OpenNtfsDxe/Open.c @@ -163,22 +163,13 @@ FileReadDir ( CHAR16 Path[MAX_PATH]; EFI_NTFS_FILE *TmpFile = NULL; UINTN Length; + INTN Res; ASSERT (File != NULL); ASSERT (Size != NULL); ASSERT ((Data != NULL) || ((Data == NULL) && (*Size == 0))); - Info = (EFI_FILE_INFO *)Data; - - if (*Size < MINIMUM_INFO_LENGTH) { - *Size = MINIMUM_INFO_LENGTH; - return EFI_BUFFER_TOO_SMALL; - } - ZeroMem (Path, sizeof (Path)); - ZeroMem (Data, *Size); - Info->Size = *Size; - Status = StrCpyS (Path, MAX_PATH, File->Path); if (EFI_ERROR (Status)) { DEBUG ((DEBUG_INFO, "NTFS: Could not copy string.\n")); @@ -196,11 +187,35 @@ FileReadDir ( Length++; } + Res = NtfsCfiPop ( + File->FileSystem, + Path, + *Size, + (INT64)File->DirIndex, + Data + ); + + if (Res != 0) { + if (Res < 0) { + *Size = File->FileSystem->CFIDataSize; + return EFI_BUFFER_TOO_SMALL; + } + + Info = (EFI_FILE_INFO *)Data; + *Size = (UINTN)Info->Size; + ++File->DirIndex; + return EFI_SUCCESS; + } + + ZeroMem (File->FileSystem->CFIData, MINIMUM_INFO_LENGTH); + Info = (EFI_FILE_INFO *)File->FileSystem->CFIData; + Info->Size = MINIMUM_INFO_LENGTH; + mIndexCounter = (INT64)File->DirIndex; if (Length == 0) { - Status = NtfsDir (File->FileSystem, L"/", Data, DIR_HOOK); + Status = NtfsDir (File->FileSystem, L"/", File->FileSystem->CFIData, DIR_HOOK); } else { - Status = NtfsDir (File->FileSystem, File->Path, Data, DIR_HOOK); + Status = NtfsDir (File->FileSystem, File->Path, File->FileSystem->CFIData, DIR_HOOK); } if (mIndexCounter >= 0) { @@ -245,9 +260,18 @@ FileReadDir ( Info->PhysicalSize = TmpFile->RootFile.DataAttributeSize; } + FreeFile (&TmpFile->RootFile); FreePool (TmpFile); } + if (*Size < Info->Size) { + *Size = (UINTN)Info->Size; + NtfsCfiPush (File->FileSystem, Path, (UINTN)Info->Size, (INT64)File->DirIndex); + return EFI_BUFFER_TOO_SMALL; + } else { + CopyMem (Data, File->FileSystem->CFIData, (UINTN)Info->Size); + } + *Size = (UINTN)Info->Size; ++File->DirIndex; diff --git a/Platform/OpenNtfsDxe/OpenNtfsDxe.inf b/Platform/OpenNtfsDxe/OpenNtfsDxe.inf index 673cefe7..657b0bf1 100644 --- a/Platform/OpenNtfsDxe/OpenNtfsDxe.inf +++ b/Platform/OpenNtfsDxe/OpenNtfsDxe.inf @@ -33,6 +33,7 @@ Data.c Index.c Compression.c + Cache.c [Packages] MdePkg/MdePkg.dec diff --git a/Utilities/TestNtfsDxe/Makefile b/Utilities/TestNtfsDxe/Makefile index d3ed6b01..8256a694 100644 --- a/Utilities/TestNtfsDxe/Makefile +++ b/Utilities/TestNtfsDxe/Makefile @@ -6,7 +6,7 @@ PROJECT = TestNtfsDxe PRODUCT = $(PROJECT)$(INFIX)$(SUFFIX) OBJS = $(PROJECT).o -OBJS += Compression.o Data.o Disc.o Index.o Info.o NTFS.o Open.o Position.o +OBJS += Compression.o Data.o Disc.o Index.o Info.o NTFS.o Open.o Position.o Cache.o include ../../User/Makefile