grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Daniel Kiper
Subject: Re: [PATCH] chainloader: Fix wrong break condition (must be AND not OR)
Date: Tue, 6 Feb 2018 16:41:06 +0100
User-agent: Mutt/1.3.28i

On Wed, Jan 31, 2018 at 12:39:05PM +0100, C. Masloch wrote:
> On at 2018-01-29 18:09 +01:00, Daniel Kiper wrote:
> > On Sun, Jan 21, 2018 at 04:02:10PM +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.
> >
> > Could you provide reference to the spec which says that?
>
> Here's a few descriptions pointing to that fact:
>
>
> 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.)
>
>
> >> This affects all users of grub_chainloader_patch_bpb which are in
> >> chainloader.c, freedos.c, and ntldr.c
> >
> > I am happy that you fix that issue but
> >   https://en.wikipedia.org/wiki/Design_of_the_FAT_file_system#BPB331_OFS_15h
> > shows that life is more complicated.
> >
> > Could you take that into account?
>
> MS-DOS 3.20 and 3.30 BPBs aren't supported anyway. The special case for

Aren't supported by whom? GRUB2?

> using a partition table entry's partition length field isn't applicable
> here. (If it were, the function simply shouldn't check these fields.)

Please say why exactly it is not applicable here.

> And this is the first I've read of that DR-DOS extension. (And again,
> the simple solution to that one is also not to check the fields for zeros.)
>
> >> 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
> >
> > Could you put your SOB here?
>
> Like this?
>
> Signed-off-by: C. Masloch <address@hidden>

Yep.

> (Should I submit a PATCH v2 for this?)

Yes, please. I am happy to commit this patch if you provide
such a nice and detailed explanation like above but with better
formating. And please add some stuff which I asked for too.

Daniel



reply via email to

[Prev in Thread] Current Thread [Next in Thread]