Fix dynamic FAT variant detection #55

[cookpate]

Fix dynamic FAT variant detection

Description

---

Only the first two bytes were considered when reading the end-of-chain value from the FAT table. This resulted in FAT32 not being fully detected.
This commit extends the check to the first 4 bytes in the table and makes FAT12 checks more strict while relaxing FAT16 and FAT32 checks since the first 3 bits are ignored but all other valid bits are checked.

Test Steps

---

Format a valid FAT32 partition (e.g. onto an SD card).
Observe debug output indicating bad partition magic when mounting the partition.
prvDetermineFatType: firstWord 0x0000FFF8

Checklist:

---

• I have tested my changes. No regression in existing tests.
• I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

#55

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[cookpate]

https://github.com/FreeRTOS/Lab-Project-FreeRTOS-FAT/blob/main/ff_fat.c#L737-L770
Related existing code.

[htibosch]

Hello @cookpate, thank you for this PR, but please mention something like this in "Description":

"This PR solves a problem that was reported in issue #55. Thank you Carl J Kugler for doing so.
The solution was suggested by @htibosch here."

Actually I was on my way to produce this PR my self, but you were quicker.

[htibosch]

@cookpate, have you tested these changes?

I did and now I see errors with a FAT16 volume that previously worked well:

Partition at 9 has strange FAT data FFFFFFF8
FF_RAMDiskInit: FF_Mount: IOMAN_INVALID_FORMAT

I think that replacing FF_getShort() with FF_getLong() is enough, beside removing the earlier _HT_ comment.

[htibosch]

Hi Patrick, I have checked the function prvDetermineFatType() and tested it in a Xilinx project which has multiple disks and partitions. I uploaded the results function here:

prvDetermineFatType_v2.zip

I would like to keep the name ulFirstWord, because it contains the first 32 bytes of the FAT, which is more than an endOfChain token.

A shortcoming in your code was that you compared a 32-bit value with a 12- or 16-bit pattern (0xFF8 or 0xFFF8). It missed the AND. In case of FAT12/16, ulFirstWord contains 2 or 3 cluster entries, only the entry 0 should be considered. It is like an affirmation that the cluster-count-rule was successful.

Thanks

[carlk3]

All I know about this I learned from Design of the FAT file system; Special entries.

The second entry (cluster 1 in the FAT) nominally stores the end-of-cluster-chain marker as used by the formater, but typically always holds 0xFFF / 0xFFFF / 0x0FFFFFFF, that is, with the exception of bits 31-28 on FAT32 volumes these bits are normally always set. Some Microsoft operating systems, however, set these bits if the volume is not the volume holding the running operating system (that is, use 0xFFFFFFFF instead of 0x0FFFFFFF here).[40] (In conjunction with alternative end-of-chain markers the lowest bits 2-0 can become zero for the lowest allowed end-of-chain marker 0xFF8 / 0xFFF8 / 0x?FFFFFF8; bit 3 should be reserved as well given that clusters 0xFF0 / 0xFFF0 / 0x?FFFFFF0 and higher are officially reserved. Some operating systems may not be able to mount some volumes if any of these bits are not set, therefore the default end-of-chain marker should not be changed.)
...
MS-DOS/PC DOS 3.3 and higher treats a value of 0xFF0[nb 10][6] on FAT12 (but not on FAT16 or FAT32) volumes as additional end-of-chain marker similar to 0xFF8-0xFFF.[6]
...

FAT12	FAT16	FAT32	Description
0xFF8 - 0xFFF (and optionally 0xFF0;[nb 10] see note)	0xFFF8 - 0xFFFF	0x?FFFFFF8 - 0x?FFFFFFF	Last cluster in file (EOC). File system implementations must treat all these values as end-of-chain marker at the same time.[7] Most file system implementations (including 86-DOS, MS-DOS, PC DOS and DR-DOS) use 0xFFF[7] / 0xFFFF[7] / 0x0FFFFFFF as end-of-file marker when allocating files, but versions of Linux before 2.5.40 used 0xFF8 / 0xFFF8 / 0x0FFFFFF8.[45] Versions of mkdosfs (dosfstools up to 3.0.26) continue to use 0x0FFFFFF8 for the root directory on FAT32 volumes, whereas some disk repair and defragment tools utilize other values in the set (e.g., SCANDISK may use 0xFF8 / 0xFFF8 / 0x0FFFFFF8 instead). While in the original 8-bit FAT implementation in Microsoft's Standalone Disk BASIC different end markers (0xC0..0xCD) were used to indicate the number of sectors (0 to 13) used up in the last cluster occupied by a file, different end markers were repurposed under DOS to indicate different types of media,[7] with the currently used end marker indicated in the cluster 1 entry, however, this concept does not seem to have been broadly utilized in practice—and to the extent that in some scenarios volumes may not be recognized by some operating systems, if some of the low-order bits of the value stored in cluster 1 are not set. Also, some faulty file system implementations only accept 0xFFF / 0xFFFF / 0x?FFFFFFF as valid end-of-chain marker. File system implementations should check cluster values in cluster-chains against the maximum allowed cluster value calculated by the actual size of the volume and treat higher values as if they were end-of-chain markers as well. (The low byte of the cluster number conceptually corresponds with the FAT ID and media descriptor values;[7] see note above for MS-DOS/PC DOS special usage of 0xFF0[nb 10] on FAT12 volumes.[6])

FAT32 uses 28 bits for cluster numbers. The remaining 4 bits in the 32-bit FAT entry are usually zero, but are reserved and should be left untouched. A standard conformant FAT32 file system driver or maintenance tool must not rely on the upper 4 bits to be zero and it must strip them off before evaluating the cluster number in order to cope with possible future expansions where these bits may be used for other purposes. [Emphasis mine] They must not be cleared by the file system driver when allocating new clusters, but should be cleared during a reformat.

So, I don't think the checks are quite right. For example,

ulEndOfChain != 0x0FFFFFF8

because allowed values are 0x?FFFFFF8 - 0x?FFFFFFF.

Maybe

 ulEndOfChain = FF_getLong( pxBuffer->pucBuffer, 0x0000 ) & 0xFFFFFFF8ul;

should be

 ulEndOfChain = FF_getLong( pxBuffer->pucBuffer, 0x0000 ) & 0x0FFFFFF8ul;

[htibosch]

Hello @cookpate, would you be so kind to copy the function contained in this ZIP file:

prvDetermineFatType_v4.zip

into the your copy of ff_ioman.c ?

@carlk3 and I went through my earlier solution prvDetermineFatType_v2.zip and agreed on the above prvDetermineFatType_v4.zip.

Thanks

[cookpate]

Happy Thanksgiving! I applied v4 to ff_ioman.c; thanks for the suggestions.

[cookpate]

I'll look into formatting this later. Need to docker into a Ubuntu 20 instance in order to get the correct version of Uncrustify...
clang-format is used now in other repos, and I believe the config used is backwards-compatible with Ubuntu 20's clang packages. So, maybe an issue should be opened here to update the workflows.

[Skptak]

/bot run formatting

[Skptak]

I'll look into formatting this later. Need to docker into a Ubuntu 20 instance in order to get the correct version of Uncrustify... clang-format is used now in other repos, and I believe the config used is backwards-compatible with Ubuntu 20's clang packages. So, maybe an issue should be opened here to update the workflows.

Unfortunately, Clang-format is not used in the majority of FreeRTOS repos.
However, #58 introduces the formatting bot and other CI-CD checks to the repo. It also contains spelling formatting, and broken link fixes.
I added your changes to the CMake file to that PR as well.

[htibosch]

ffconfigFAT_CHECK is a helpful option that helps determine/verify the FAT type, FAT12, FAT16, or FAT32.
It reads the first cluster address of the FAT table, which is supposed to have the value of end-of-chain.

I retested the latest version on a Xilinx Zynq, and it still works as expected.

Thanks for this PR