From ae2a2745185c08d194775f28588f58ac90ae4351 Mon Sep 17 00:00:00 2001 From: Daniel Kiper Date: Fri, 23 Feb 2018 22:32:55 +0100 Subject: [PATCH] chainloader: Fix wrong break condition (must be AND not, OR) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The definition of bpb's num_total_sectors_16 and num_total_sectors_32 is that either the 16-bit field is non-zero and is used (in which case eg mkfs.fat sets the 32-bit field to zero), or it is zero and the 32-bit field is used. Therefore, a BPB is invalid only if *both* fields are zero; having one field as zero and the other as non-zero is the case to be expected. (Indeed, according to Microsoft's specification one of the fields *must* be zero, and the other non-zero.) This affects all users of grub_chainloader_patch_bpb which are in chainloader.c, freedos.c, and ntldr.c Some descriptions of the semantics of these two fields: https://www.win.tue.nl/~aeb/linux/fs/fat/fat-1.html The old 2-byte fields "total number of sectors" and "number of sectors per FAT" are now zero; this information is now found in the new 4-byte fields. (Here given in the FAT32 EBPB section but the total sectors 16/32 bit fields semantic is true of FAT12 and FAT16 too.) https://wiki.osdev.org/FAT#BPB_.28BIOS_Parameter_Block.29 19 | 2 | The total sectors in the logical volume. If this value is 0, it means there are more than 65535 sectors in the volume, and the actual count is stored in "Large Sectors (bytes 32-35). 32 | 4 | Large amount of sector on media. This field is set if there are more than 65535 sectors in the volume. (Doesn't specify what the "large" field is set to when unused, but as mentioned mkfs.fat sets it to zero then.) https://technet.microsoft.com/en-us/library/cc976796.aspx 0x13 | WORD | 0x0000 | Small Sectors . The number of sectors on the volume represented in 16 bits (< 65,536). For volumes larger than 65,536 sectors, this field has a value of zero and the Large Sectors field is used instead. 0x20 | DWORD | 0x01F03E00 | Large Sectors . If the value of the Small Sectors field is zero, this field contains the total number of sectors in the FAT16 volume. If the value of the Small Sectors field is not zero, the value of this field is zero. https://staff.washington.edu/dittrich/misc/fatgen103.pdf page 10 BPB_TotSec16 | 19 | 2 | This field is the old 16-bit total count of sectors on the volume. This count includes the count of all sectors in all four regions of the volume. This field can be 0; if it is 0, then BPB_TotSec32 must be non-zero. For FAT32 volumes, this field must be 0. For FAT12 and FAT16 volumes, this field contains the sector count, and BPB_TotSec32 is 0 if the total sector count “fits” (is less than 0x10000). BPB_TotSec32 | 32 | 4 | This field is the new 32-bit total count of sectors on the volume. This count includes the count of all sectors in all four regions of the volume. This field can be 0; if it is 0, then BPB_TotSec16 must be non-zero. For FAT32 volumes, this field must be non-zero. For FAT12/FAT16 volumes, this field contains the sector count if BPB_TotSec16 is 0 (count is greater than or equal to 0x10000). (This specifies that an unused BPB_TotSec32 field is set to zero.) By the way fix offsets in include/grub/fat.h. Tested with lDebug booted in qemu via grub2's FreeDOS direct loading support, refer to https://bitbucket.org/ecm/ldosboot + https://bitbucket.org/ecm/ldebug Signed-off-by: C. Masloch Reviewed-by: Daniel Kiper --- grub-core/loader/i386/pc/chainloader.c | 2 +- include/grub/fat.h | 17 ++++++----------- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/grub-core/loader/i386/pc/chainloader.c b/grub-core/loader/i386/pc/chainloader.c index c79c4fe0fc9..18220b7aaab 100644 --- a/grub-core/loader/i386/pc/chainloader.c +++ b/grub-core/loader/i386/pc/chainloader.c @@ -117,7 +117,7 @@ grub_chainloader_patch_bpb (void *bs, grub_device_t dev, grub_uint8_t dl) if (bpb->num_reserved_sectors == 0) break; - if (bpb->num_total_sectors_16 == 0 || bpb->num_total_sectors_32 == 0) + if (bpb->num_total_sectors_16 == 0 && bpb->num_total_sectors_32 == 0) break; if (bpb->num_fats == 0) diff --git a/include/grub/fat.h b/include/grub/fat.h index 4a5aab79346..8d7e4a1e54d 100644 --- a/include/grub/fat.h +++ b/include/grub/fat.h @@ -28,20 +28,15 @@ struct grub_fat_bpb grub_uint16_t bytes_per_sector; grub_uint8_t sectors_per_cluster; grub_uint16_t num_reserved_sectors; - grub_uint8_t num_fats; - /* 0x10 */ + grub_uint8_t num_fats; /* 0x10 */ grub_uint16_t num_root_entries; grub_uint16_t num_total_sectors_16; - grub_uint8_t media; - /*0 x15 */ + grub_uint8_t media; /* 0x15 */ grub_uint16_t sectors_per_fat_16; - grub_uint16_t sectors_per_track; - /*0 x19 */ - grub_uint16_t num_heads; - /*0 x1b */ - grub_uint32_t num_hidden_sectors; - /* 0x1f */ - grub_uint32_t num_total_sectors_32; + grub_uint16_t sectors_per_track; /* 0x18 */ + grub_uint16_t num_heads; /* 0x1A */ + grub_uint32_t num_hidden_sectors; /* 0x1C */ + grub_uint32_t num_total_sectors_32; /* 0x20 */ union { struct