[Info-mtools] Incorrect assignment of partition type in mformat/mpartition

[Alain Knaff]

Hi,

On 17/08/18 00:21, Pali Rohár wrote:
> On Saturday 11 August 2018 17:48:03 Pali Rohár wrote:
>> Hi!
>>
>> I found out that mformat and mpartition incorrectly assigns MBR
>> partition type. This is because of following problems:
>>
>> 1) Begin and end location of partition in function setBeginEnd() is of
>> type "signed" integer which start overflowing when hitting 1TB partition
>> size. It should be "unsigned" type, it does not make sense to have
>> negative value. Result is that 1TB partitions are marked as FAT12
>> because "end-begin < 4096" is truth (end is negative number).>>
>> 2) There is missing partition type for FAT32 (MBR ID 0x0C).
>>
>> 3) Detection expects that sector size is 512 bytes, which is not truth
>> for new Native 4K disks with sector size of 4096 bytes.
>>
>> In attachment I'm sending a patch which fixes all these 3 problems.
>>
>> I created FAT32 image with mformat -F and then via fdisk verified that
>> partition type is 0x0C.
> 
> I did here one mistake. Partition types 0x01, 0x04 and 0x06 depend on
> number of sectors, and not on number of (mega)bytes -- as was written in
> original comments. 0x04 is used for original FAT16 and 0x06 for BIGDOS
> FAT16 (or big FAT12). So 0x04 is only for partitions which has less then
> 2^16 sectors (which is 32MB when disk sector size is 512b; this is where
> original comment "<32M" comes from). Therefore sector size is not needed
> in this function at all.
> 
> In attachment I'm sending fixed version of my previous patch
> mtools_partition.patch.
> 

Thanks for the patch.

As there were no references cited for the changed definitions of the
partition types, I had to do some googling around, and found the
following wikipedia article which does indeed seem to support (most of)
your proposed changes:

https://en.wikipedia.org/wiki/Partition_type#List_of_partition_IDs

There is only one difference which struck my eye: in order to chose
between 0x06 and 0x0e you check whether the partition is entirely below
the 1024 cylinder mark. However, wikipedia claims that the cutoff should
be at 8GB (or rather 16 G sectors). The Microsoft source cited by
wikipedia
(https://docs.microsoft.com/en-us/previous-versions/windows/it-pro/windows-2000-server/cc977219(v=technet.10)
claims a cut-off of 4GB...

So, now I'm puzzled which of the 3 is correct. Or maybe you still have
other supporting documentations which you'd like to share? In any case,
8GB does indeed roughly correspond to 1024 cylinders if 63 sectors and
255 heads are used (which would indeed be picked by LBA assyst for 8GB)

In the meantime, I re-arranged the tests to make it easier to understand
what is going on:

if(!type) {
        if (fat_bits == 0) {
                /**
                 * Fat bits unknown / not specified. We look
                 * at size to get a rough estimate what FAT
                 * bits are used.  Note: this is only an
                 * estimate, the precise calculation would
                 * involve the number of clusters, which is
                 * not necessarily known here.
                 */
                /* The cutoff number 32680 is based on cc977219,
                 * corresponding to a FAT12 partition with 4K
                 * clusters */
                if(end-begin < 32680)
                        fat_bits = 12;
                else
                        fat_bits = 16;
        }

        /* Description of various partition types in
         * https://en.wikipedia.org/wiki/Partition_type#List_of_partition_IDs
         * and
         *
https://docs.microsoft.com/en-us/previous-versions/windows/it-pro/windows-2000-server/cc977219(v=technet.10)
         */
        if (fat_bits == 32)
                /* FAT 32 partition. For now, we disregard the
                 * possibility of FAT 32 CHS partitions */
                type = 0x0C; /* Win95 FAT32, LBA */
        else if (end < 65536) {
                /* FAT 12 or FAT 16 partitions which fit entirely below
                   the 32M mark */
                /* The 32M restriction doesn't apply to logical
                   partitions within an extended partition, but for the
                   moment mpartition only makes primary partitions */
                if(fat_bits == 12)
                        /* FAT 12 partition */
                        type = 0x01; /* DOS FAT12, CHS */
                else if (fat_bits == 16)
                        /* FAT 16 partition */
                        type = 0x04; /* DOS FAT16, CHS */
        } else if (end < sectors * heads * 1024)
                /* FAT 12 or FAT16 partition above the 32M mark
                 * but below the 1024 cyliner mark */
                /* Shouldn't that be the 8GB mark (wikipedia) or
                 * 4GB (Microsoft)? */
                type = 0x06; /* DOS BIG FAT16 or FAT12, CHS */
        else
                type = 0x0E; /* Win95 BIG FAT16, LBA */
        }

Regards,

Alain