From bc664d1830c9446cb1d33b10a41e6b3d207997f1 Mon Sep 17 00:00:00 2001 From: Oliver Smith-Denny Date: Thu, 13 Feb 2025 12:52:00 -0800 Subject: Revert "FatPkg: Validate Reserved FAT Entries on Volume Open" This reverts commit 58766a472932c485d41163b1746fb1d9e7984f07. In edk2 commit 58766a4, validation of the two reserved FAT entries was added. However, it also checked the return of FatGetFatEntry to MAX_UINT32, which is what FatGetFatEntry returns when it encounters an error, e.g. not being able to read the disk. However, MAX_UINT32 is also a valid value for the reserved FAT entries and under some conditions these will be returned in the success case. A FAT volume formatted with these valid values of the reserved FAT entries will fail to boot an OS because the opening of the volume will fail. However, the reason FatGetFatEntry returns MAX_UINT32 is that most other uses of the function are comparing it against the END_OF_CHAIN mark, which MAX_UINT32 will trip and those functions will fail out. Because this is a critical bug that can prevent OS booting and the bug the original commit was solving was accounting for a bad FAT filesystem formatting tool, this commit is reverted for now. Future work will clean up FatGetFatEntry so that it returns an EFI_STATUS, but that involves more work and this bug needs to be resolved in the meantime. Signed-off-by: Oliver Smith-Denny --- FatPkg/EnhancedFatDxe/Fat.h | 16 -------- FatPkg/EnhancedFatDxe/FatFileSystem.h | 2 - FatPkg/EnhancedFatDxe/FileSpace.c | 1 + FatPkg/EnhancedFatDxe/Init.c | 62 +++++-------------------------- FatPkg/FatPei/FatLiteAccess.c | 69 ----------------------------------- FatPkg/FatPei/FatLiteFmt.h | 7 ---- 6 files changed, 10 insertions(+), 147 deletions(-) diff --git a/FatPkg/EnhancedFatDxe/Fat.h b/FatPkg/EnhancedFatDxe/Fat.h index 525393dbd4..fb6699061e 100644 --- a/FatPkg/EnhancedFatDxe/Fat.h +++ b/FatPkg/EnhancedFatDxe/Fat.h @@ -975,22 +975,6 @@ FatComputeFreeInfo ( IN FAT_VOLUME *Volume ); -/** - - Get the FAT entry value of the volume, which is identified with the Index. - - @param Volume - FAT file system volume. - @param Index - The index of the FAT entry of the volume. - - @return The value of the FAT entry. - -**/ -UINTN -FatGetFatEntry ( - IN FAT_VOLUME *Volume, - IN UINTN Index - ); - // // Init.c // diff --git a/FatPkg/EnhancedFatDxe/FatFileSystem.h b/FatPkg/EnhancedFatDxe/FatFileSystem.h index 3bad7fceea..60b9c56b71 100644 --- a/FatPkg/EnhancedFatDxe/FatFileSystem.h +++ b/FatPkg/EnhancedFatDxe/FatFileSystem.h @@ -35,8 +35,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #define FAT_CLUSTER_SPECIAL_FAT32 0x0FFFFFF7 #define FAT_CLUSTER_MASK_FAT12 0xFFF #define FAT_CLUSTER_UNMASK_FAT12 0xF000 -#define FAT_CLUSTER_MASK_FAT16 0xFFFF -#define FAT_CLUSTER_UNMASK_FAT16 0xF0000 #define FAT_CLUSTER_MASK_FAT32 0x0FFFFFFF #define FAT_CLUSTER_UNMASK_FAT32 0xF0000000 #define FAT_POS_FAT12(a) ((a) * 3 / 2) diff --git a/FatPkg/EnhancedFatDxe/FileSpace.c b/FatPkg/EnhancedFatDxe/FileSpace.c index 6852c3ea00..909d4980d2 100644 --- a/FatPkg/EnhancedFatDxe/FileSpace.c +++ b/FatPkg/EnhancedFatDxe/FileSpace.c @@ -80,6 +80,7 @@ FatLoadFatEntry ( @return The value of the FAT entry. **/ +STATIC UINTN FatGetFatEntry ( IN FAT_VOLUME *Volume, diff --git a/FatPkg/EnhancedFatDxe/Init.c b/FatPkg/EnhancedFatDxe/Init.c index f020cf703d..9c51ed5b7b 100644 --- a/FatPkg/EnhancedFatDxe/Init.c +++ b/FatPkg/EnhancedFatDxe/Init.c @@ -95,6 +95,14 @@ FatAllocateVolume ( goto Done; } + // + // Initialize cache + // + Status = FatInitializeDiskCache (Volume); + if (EFI_ERROR (Status)) { + goto Done; + } + // // Install our protocol interfaces on the device's handle // @@ -229,7 +237,6 @@ FatOpenDevice ( UINTN SectorsPerFat; UINT8 SectorsPerClusterAlignment; UINT8 BlockAlignment; - UINTN ReservedFatEntry; // // Read the FAT_BOOT_SECTOR BPB info @@ -416,58 +423,7 @@ FatOpenDevice ( // We are now defining FAT Type // Volume->FatType = FatType; - - // - // Initialize cache before we use the helper functions that hit the cache - // - Status = FatInitializeDiskCache (Volume); - if (EFI_ERROR (Status)) { - return Status; - } - - // - // Check the reserved FAT entries to ensure they contain valid values - // - ReservedFatEntry = FatGetFatEntry (Volume, 0); - if (Volume->FatEntryBuffer == MAX_UINT32) { - return EFI_VOLUME_CORRUPTED; - } - - // Reserved FAT entry 0 should contain the BPB_MEDIA byte value in the low 8 bits with all other bits set to 1 - switch (FatType) { - case Fat12: - if ((ReservedFatEntry & FAT_CLUSTER_MASK_FAT12) != ((UINTN)FatBs.FatBsb.Media | 0xF00)) { - return EFI_VOLUME_CORRUPTED; - } - - break; - - case Fat16: - if ((ReservedFatEntry & FAT_CLUSTER_MASK_FAT16) != ((UINTN)FatBs.FatBsb.Media | 0xFF00)) { - return EFI_VOLUME_CORRUPTED; - } - - break; - - case Fat32: - // the upper 4 bits of a FAT32 entry are reserved, so are unchecked here - if ((ReservedFatEntry & FAT_CLUSTER_MASK_FAT32) != ((UINTN)FatBs.FatBsb.Media | 0x0FFFFF00)) { - return EFI_VOLUME_CORRUPTED; - } - - break; - - default: - return EFI_VOLUME_CORRUPTED; - } - - // Reserved FAT entry 1 should contain the end of chain mark. On FAT16 and FAT32, the high 2 bits may be used as - // dirty and hardware error bits, so are ignored in this check, but FatGetFatEntry already ignores them to unify the - // logic across FAT types - ReservedFatEntry = FatGetFatEntry (Volume, 1); - if ((Volume->FatEntryBuffer == MAX_UINT32) || !FAT_END_OF_FAT_CHAIN (ReservedFatEntry)) { - return EFI_VOLUME_CORRUPTED; - } + ASSERT (FatType != FatUndefined); return EFI_SUCCESS; } diff --git a/FatPkg/FatPei/FatLiteAccess.c b/FatPkg/FatPei/FatLiteAccess.c index 46a7563ab6..10df4516b2 100644 --- a/FatPkg/FatPei/FatLiteAccess.c +++ b/FatPkg/FatPei/FatLiteAccess.c @@ -43,7 +43,6 @@ FatGetBpbInfo ( UINT64 FatLba; UINT64 RootLba; UINT64 FirstClusterLba; - UINT32 ReservedFatEntries[2]; // // Read in the BPB @@ -168,74 +167,6 @@ FatGetBpbInfo ( Volume->FatType = Volume->MaxCluster < 4085 ? Fat12 : Fat16; } - // - // Read reserved FAT entries which are the first two entries from FatPos - // - Status = FatReadDisk ( - PrivateData, - Volume->BlockDeviceNo, - Volume->FatPos, - (Volume->FatType == Fat32) ? sizeof (UINT32) * 2 : sizeof (UINT16) * 2, - ReservedFatEntries - ); - if (EFI_ERROR (Status)) { - return Status; - } - - // - // Reserved FAT entry 0 should contain the BPB_MEDIA byte value in the low 8 bits with all other bits set to 1 - // Reserved FAT entry 1 should contain the end of chain mark. On FAT16 and FAT32, the high 2 bits may be used as - // dirty and hardware error bits, so are ignored in this check - // - switch (Volume->FatType) { - case Fat12: - // we read two entries and in FAT12, each entry is 12 bits, so we need to shift the first entry by 20 bits to - // only read it and not the second entry and beyond - if (((ReservedFatEntries[0] >> 20) & FAT_CLUSTER_MASK_FAT12) != ((UINTN)Bpb.Media | 0xF00)) { - return EFI_VOLUME_CORRUPTED; - } - - // the second entry starts 12 bits in and is 12 bits in length, so we shift by 8 bits to remove the start of the - // third entry and then mask to only read the second entry - if (!FAT_CLUSTER_END_OF_CHAIN (ReservedFatEntries[0] >> 8)) { - return EFI_VOLUME_CORRUPTED; - } - - break; - - case Fat16: - // in FAT16, each entry is 16 bits, so the first entry is the upper 16 bits of ReservedFatEntries[0] - if (((ReservedFatEntries[0] >> 16) & FAT_CLUSTER_MASK_FAT16) != ((UINTN)Bpb.Media | 0xFF00)) { - return EFI_VOLUME_CORRUPTED; - } - - // the second entry is simply the lower 16 bits of ReservedFatEntries[0], however, we must ignore the upper two - // bits. For the purposes of checking if the EOC mark exists, we treat those two bits as 1 - if (!FAT_CLUSTER_END_OF_CHAIN ((ReservedFatEntries[0] & 0x3FFF) | 0xC000)) { - return EFI_VOLUME_CORRUPTED; - } - - break; - - case Fat32: - // the upper 4 bits of a FAT32 entry are reserved, so are unchecked here - // FAT32 has 32 bit entries, so the first entry is ReservedFatEntries[0] - if ((ReservedFatEntries[0] & FAT_CLUSTER_MASK_FAT32) != ((UINTN)Bpb.Media | 0x0FFFFF00)) { - return EFI_VOLUME_CORRUPTED; - } - - // the second entry is simply ReservedFatEntries[1], but we must ignore the upper two bits. For the purposes of - // checking if the EOC mark exists, we treat those two bits as 1 - if (!FAT_CLUSTER_END_OF_CHAIN ((ReservedFatEntries[1] & 0x3FFFFFFF) | 0xC0000000)) { - return EFI_VOLUME_CORRUPTED; - } - - break; - - default: - return EFI_VOLUME_CORRUPTED; - } - return EFI_SUCCESS; } diff --git a/FatPkg/FatPei/FatLiteFmt.h b/FatPkg/FatPei/FatLiteFmt.h index b179b5f95f..fef70fe1de 100644 --- a/FatPkg/FatPei/FatLiteFmt.h +++ b/FatPkg/FatPei/FatLiteFmt.h @@ -27,13 +27,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #define FAT_CLUSTER_BAD (FAT_CLUSTER_SPECIAL) #define FAT_CLUSTER_LAST (-1) -#define FAT_CLUSTER_MASK_FAT12 0xFFF -#define FAT_CLUSTER_UNMASK_FAT12 0xF000 -#define FAT_CLUSTER_MASK_FAT16 0xFFFF -#define FAT_CLUSTER_UNMASK_FAT16 0xF0000 -#define FAT_CLUSTER_MASK_FAT32 0x0FFFFFFF -#define FAT_CLUSTER_UNMASK_FAT32 0xF0000000 - #define DELETE_ENTRY_MARK 0xE5 #define EMPTY_ENTRY_MARK 0x00 -- cgit