Re: [PATCH v2] chainloader: Fix wrong break condition (must be AND not, OR)

[C. Masloch]

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)

[Daniel Kiper]

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)

[Daniel Kiper]

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 

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)

[C. Masloch]

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