Re: [PATCH v2] chainloader: Fix wrong break condition (must be AND not, OR)
On at 2018-02-23 23:05 +01:00, Daniel Kiper wrote: > Applied! > > FYI, this is last time when I sent commit confirmation. > If I send one in the future then this will be rather > exception than rule. So, if you receive my reviewed-by > and patch is not committed in a week or two then it means > that it fallen into abyss and you have to ping me. > > Have a nice weekend, > > Daniel Thanks, I'll keep that in mind. Regards, ecm ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2] chainloader: Fix wrong break condition (must be AND not, OR)
On Tue, Feb 20, 2018 at 06:01:19PM +0100, Daniel Kiper wrote: > On Mon, Feb 19, 2018 at 03:26:35PM +0100, C. Masloch wrote: > > 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 | 0x | > > > 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 > > > 0x1). > > > > > 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 0x1). > > > > (This specifies that an unused BPB_TotSec32 field is set to zero.) > > > > 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> > I will add to the commit message that by the way you are fixing comments in > include/grub/fat.h. > > Otherwise Reviewed-by: Daniel Kiper Applied! FYI, this is last time when I sent commit confirmation. If I send one in the future then this will be rather exception than rule. So, if you receive my reviewed-by and patch is not committed in a week or two then it means that it fallen into abyss and you have to ping me. Have a nice weekend, Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2] chainloader: Fix wrong break condition (must be AND not, OR)
On Mon, Feb 19, 2018 at 03:26:35PM +0100, C. Masloch wrote: > 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 | 0x | > > 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 > > 0x1). > > > 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 0x1). > > (This specifies that an unused BPB_TotSec32 field is set to zero.) > > 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. MaslochI will add to the commit message that by the way you are fixing comments in include/grub/fat.h. Otherwise Reviewed-by: Daniel Kiper Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v2] chainloader: Fix wrong break condition (must be AND not, OR)
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 | 0x | > 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 > 0x1). > 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 0x1). (This specifies that an unused BPB_TotSec32 field is set to zero.) 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--- 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 c79c4fe0f..18220b7aa 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 4a5aab793..8d7e4a1e5 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 -- 2.11.0