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

[C. Masloch]

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

(Should I submit a PATCH v2 for this?)