[PATCH v2 0/5] fs: fat: calculate FAT type based on cluster count
From: Christian Taedcke christian.taedcke@weidmueller.com
This series fixes an issue where the FAT type (FAT12, FAT16) is not correctly detected, e.g. when the BPB field BS_FilSysType contains the valid value "FAT ".
This issue occures, for example, if a partition is formatted by swupdate using its diskformat handler. swupdate uses the FAT library from http://elm-chan.org/fsw/ff/ internally.
See https://groups.google.com/g/swupdate/c/7Yc3NupjXx8 for a discussion in the swupdate mailing list.
Please refer to the commit messages for more details.
1. Added bootsector checks
Most tests from https://www.win.tue.nl/~aeb/linux/fs/fat/fat-2.html are added in the latest commit. Only the tests VIII, IX and X are not implemented.
I also checked the Linux kernel code (v6.6) and did not find any checks on 'vistart->fs_type'. This is the reason why is skipped them here.
See section '3. Size comparisons' for the impact on the binary size.
2. Tests
I will try to implement some tests for the next version of this series.
3. Size comparisons
I executed bloat-o-meter from the Linux kernel for an arm64 target (config xilinx_zynqmp_mini_emmc0_defconfig):
Comparison of the binary spl/u-boot-spl between master (rev e17d174773e9ba9447596708e702b7382e47a6cf) and this patch series (including the added validity checks of the boot sector):
add/remove: 0/0 grow/shrink: 1/1 up/down: 100/-12 (88) Function old new delta read_bootsectandvi 308 408 +100 fat_itr_root 444 432 -12 Total: Before=67977, After=68065, chg +0.13%
When compare the size of the binary spl/u-boot-spl between master this series without the the validity checks of the boot sector (so i removed the last commit in this series):
add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-24 (-24) Function old new delta read_bootsectandvi 308 296 -12 fat_itr_root 444 432 -12 Total: Before=67977, After=67953, chg -0.04%
So the size of the spl on this arm64 target increases by 88 bytes for this series. When i remove the validity check the size decreases by 24 bytes.
Changes in v2: - Use get_unaligned_le16 instead of custom macro - Move code change in test/image/spl_load_fs.c into separate commit - Use get_unaligned_le16 instead of custom macro - Extract FAT32 logic from determine_fat_bits() into read_bootsectandvi() - Rename determine_fat_bits() to determine_legacy_fat_bits() - Keep one goto label instead of removing both - Move changing gotos from read_bootsectandvi() into separate commit - Add validity checks for boot sector
Christian Taedcke (5): fs: fat: use get_unaligned_le16 to convert u8[2] to u16 test: spl: Remove usage of FAT32_SIGN fs: fat: calculate FAT type based on cluster count fs: fat: simplify gotos from read_bootsectandvi fs: fat: add bootsector validity check
fs/fat/fat.c | 114 +++++++++++++++++++++++++++++---------- include/fat.h | 6 --- test/image/spl_load_fs.c | 2 +- 3 files changed, 88 insertions(+), 34 deletions(-)
From: Christian Taedcke christian.taedcke@weidmueller.com
This reduces code duplications.
Signed-off-by: Christian Taedcke christian.taedcke@weidmueller.com ---
Changes in v2: - Use get_unaligned_le16 instead of custom macro
fs/fat/fat.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/fat/fat.c b/fs/fat/fat.c index 8ff1fd0ec8..a3522340ef 100644 --- a/fs/fat/fat.c +++ b/fs/fat/fat.c @@ -18,6 +18,7 @@ #include <fs.h> #include <log.h> #include <asm/byteorder.h> +#include <asm/unaligned.h> #include <part.h> #include <malloc.h> #include <memalign.h> @@ -571,7 +572,7 @@ static int get_fs_info(fsdata *mydata) mydata->total_sect = bs.total_sect; } else { mydata->fatlength = bs.fat_length; - mydata->total_sect = (bs.sectors[1] << 8) + bs.sectors[0]; + mydata->total_sect = get_unaligned_le16(bs.sectors); if (!mydata->total_sect) mydata->total_sect = bs.total_sect; } @@ -583,7 +584,7 @@ static int get_fs_info(fsdata *mydata)
mydata->rootdir_sect = mydata->fat_sect + mydata->fatlength * bs.fats;
- mydata->sect_size = (bs.sector_size[1] << 8) + bs.sector_size[0]; + mydata->sect_size = get_unaligned_le16(bs.sector_size); mydata->clust_size = bs.cluster_size; if (mydata->sect_size != cur_part_info.blksz) { log_err("FAT sector size mismatch (fs=%u, dev=%lu)\n", @@ -607,8 +608,7 @@ static int get_fs_info(fsdata *mydata) (mydata->clust_size * 2); mydata->root_cluster = bs.root_cluster; } else { - mydata->rootdir_size = ((bs.dir_entries[1] * (int)256 + - bs.dir_entries[0]) * + mydata->rootdir_size = (get_unaligned_le16(bs.dir_entries) * sizeof(dir_entry)) / mydata->sect_size; mydata->data_begin = mydata->rootdir_sect +
From: Christian Taedcke christian.taedcke@weidmueller.com
FAT32_SIGN is removed in the following commits.
Signed-off-by: Christian Taedcke christian.taedcke@weidmueller.com ---
Changes in v2: - Move code change in test/image/spl_load_fs.c into separate commit
test/image/spl_load_fs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/test/image/spl_load_fs.c b/test/image/spl_load_fs.c index 297ab08a82..90e640b5de 100644 --- a/test/image/spl_load_fs.c +++ b/test/image/spl_load_fs.c @@ -220,7 +220,7 @@ static size_t create_fat(void *dst, size_t size, const char *filename, bs->root_cluster = cpu_to_le32(root_sector);
vi->ext_boot_sign = 0x29; - memcpy(vi->fs_type, FAT32_SIGN, sizeof(vi->fs_type)); + memcpy(vi->fs_type, "FAT32 ", sizeof(vi->fs_type));
memcpy(dst + 0x1fe, "\x55\xAA", 2);
From: Christian Taedcke christian.taedcke@weidmueller.com
This fixes an issue where the FAT type (FAT12, FAT16) is not correctly detected, e.g. when the BPB field BS_FilSysType contains the valid value "FAT ".
According to the FAT spec the field BS_FilSysType has only informational character and does not determine the FAT type.
The logic of this code is based on the linux kernel implementation from the file fs/fat/inode.c function fat_fill_super().
For details about FAT see http://elm-chan.org/docs/fat_e.html
Signed-off-by: Christian Taedcke christian.taedcke@weidmueller.com ---
Changes in v2: - Use get_unaligned_le16 instead of custom macro - Extract FAT32 logic from determine_fat_bits() into read_bootsectandvi() - Rename determine_fat_bits() to determine_legacy_fat_bits()
fs/fat/fat.c | 48 ++++++++++++++++++++++++++++-------------------- include/fat.h | 6 ------ 2 files changed, 28 insertions(+), 26 deletions(-)
diff --git a/fs/fat/fat.c b/fs/fat/fat.c index a3522340ef..c368c3b076 100644 --- a/fs/fat/fat.c +++ b/fs/fat/fat.c @@ -26,6 +26,9 @@ #include <linux/compiler.h> #include <linux/ctype.h>
+/* maximum number of clusters for FAT12 */ +#define MAX_FAT12 0xFF4 + /* * Convert a string to lowercase. Converts at most 'len' characters, * 'len' may be larger than the length of 'str' if 'str' is NULL @@ -484,6 +487,27 @@ static __u8 mkcksum(struct nameext *nameext) return ret; }
+/* + * Determine if the FAT type is FAT12 or FAT16 + * + * Based on fat_fill_super() from the Linux kernel's fs/fat/inode.c + */ +static int determine_legacy_fat_bits(const boot_sector *bs) +{ + u16 fat_start = bs->reserved; + u32 dir_start = fat_start + bs->fats * bs->fat_length; + u32 rootdir_sectors = get_unaligned_le16(bs->dir_entries) * + sizeof(dir_entry) / + get_unaligned_le16(bs->sector_size); + u32 data_start = dir_start + rootdir_sectors; + u16 sectors = get_unaligned_le16(bs->sectors); + u32 total_sectors = sectors ? sectors : bs->total_sect; + u32 total_clusters = (total_sectors - data_start) / + bs->cluster_size; + + return (total_clusters > MAX_FAT12) ? 16 : 12; +} + /* * Read boot sector and volume info from a FAT filesystem */ @@ -518,7 +542,7 @@ read_bootsectandvi(boot_sector *bs, volume_info *volinfo, int *fatsize) bs->total_sect = FAT2CPU32(bs->total_sect);
/* FAT32 entries */ - if (bs->fat_length == 0) { + if (!bs->fat_length && bs->fat32_length) { /* Assume FAT32 */ bs->fat32_length = FAT2CPU32(bs->fat32_length); bs->flags = FAT2CPU16(bs->flags); @@ -529,25 +553,10 @@ read_bootsectandvi(boot_sector *bs, volume_info *volinfo, int *fatsize) *fatsize = 32; } else { vistart = (volume_info *)&(bs->fat32_length); - *fatsize = 0; + *fatsize = determine_legacy_fat_bits(bs); } memcpy(volinfo, vistart, sizeof(volume_info)); - - if (*fatsize == 32) { - if (strncmp(FAT32_SIGN, vistart->fs_type, SIGNLEN) == 0) - goto exit; - } else { - if (strncmp(FAT12_SIGN, vistart->fs_type, SIGNLEN) == 0) { - *fatsize = 12; - goto exit; - } - if (strncmp(FAT16_SIGN, vistart->fs_type, SIGNLEN) == 0) { - *fatsize = 16; - goto exit; - } - } - - debug("Error: broken fs_type sign\n"); + goto exit; fail: ret = -1; exit: @@ -1157,9 +1166,8 @@ int file_fat_detectfs(void)
memcpy(vol_label, volinfo.volume_label, 11); vol_label[11] = '\0'; - volinfo.fs_type[5] = '\0';
- printf("Filesystem: %s "%s"\n", volinfo.fs_type, vol_label); + printf("Filesystem: FAT%d "%s"\n", fatsize, vol_label);
return 0; } diff --git a/include/fat.h b/include/fat.h index a9756fb4cd..3dce99a23c 100644 --- a/include/fat.h +++ b/include/fat.h @@ -34,12 +34,6 @@ struct disk_partition; /* Maximum number of entry for long file name according to spec */ #define MAX_LFN_SLOT 20
-/* Filesystem identifiers */ -#define FAT12_SIGN "FAT12 " -#define FAT16_SIGN "FAT16 " -#define FAT32_SIGN "FAT32 " -#define SIGNLEN 8 - /* File attributes */ #define ATTR_RO 1 #define ATTR_HIDDEN 2
From: Christian Taedcke christian.taedcke@weidmueller.com
This simplifies the code a little bit.
Signed-off-by: Christian Taedcke christian.taedcke@weidmueller.com ---
Changes in v2: - Keep one goto label instead of removing both - Move changing gotos from read_bootsectandvi() into separate commit
fs/fat/fat.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/fs/fat/fat.c b/fs/fat/fat.c index c368c3b076..77f225ccd8 100644 --- a/fs/fat/fat.c +++ b/fs/fat/fat.c @@ -531,7 +531,8 @@ read_bootsectandvi(boot_sector *bs, volume_info *volinfo, int *fatsize)
if (disk_read(0, 1, block) < 0) { debug("Error: reading block\n"); - goto fail; + ret = -1; + goto out_free; }
memcpy(bs, block, sizeof(boot_sector)); @@ -556,10 +557,8 @@ read_bootsectandvi(boot_sector *bs, volume_info *volinfo, int *fatsize) *fatsize = determine_legacy_fat_bits(bs); } memcpy(volinfo, vistart, sizeof(volume_info)); - goto exit; -fail: - ret = -1; -exit: + +out_free: free(block); return ret; }
From: Christian Taedcke christian.taedcke@weidmueller.com
The performed checks are similar to the checks performed by the Linux kernel in the function fat_read_bpb() in the file fs/fat/inode.c.
Signed-off-by: Christian Taedcke christian.taedcke@weidmueller.com ---
Changes in v2: - Add validity checks for boot sector
fs/fat/fat.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)
diff --git a/fs/fat/fat.c b/fs/fat/fat.c index 77f225ccd8..14e53cf2d5 100644 --- a/fs/fat/fat.c +++ b/fs/fat/fat.c @@ -25,6 +25,7 @@ #include <asm/cache.h> #include <linux/compiler.h> #include <linux/ctype.h> +#include <linux/log2.h>
/* maximum number of clusters for FAT12 */ #define MAX_FAT12 0xFF4 @@ -508,6 +509,52 @@ static int determine_legacy_fat_bits(const boot_sector *bs) return (total_clusters > MAX_FAT12) ? 16 : 12; }
+/* + * Determines if the boot sector's media field is valid + * + * Based on fat_valid_media() from Linux kernel's include/linux/msdos_fs.h + */ +static int fat_valid_media(u8 media) +{ + return media >= 0xf8 || media == 0xf0; +} + +/* + * Determines if the given boot sector is valid + * + * Based on fat_read_bpb() from the Linux kernel's fs/fat/inode.c + */ +static int is_bootsector_valid(const boot_sector *bs) +{ + u16 sector_size = get_unaligned_le16(bs->sector_size); + u16 dir_per_block = sector_size / sizeof(dir_entry); + + if (!bs->reserved) + return 0; + + if (!bs->fats) + return 0; + + if (!fat_valid_media(bs->media)) + return 0; + + if (!is_power_of_2(sector_size) || + sector_size < 512 || + sector_size > 4096) + return 0; + + if (!is_power_of_2(bs->cluster_size)) + return 0; + + if (!bs->fat_length && !bs->fat32_length) + return 0; + + if (get_unaligned_le16(bs->dir_entries) & (dir_per_block - 1)) + return 0; + + return 1; +} + /* * Read boot sector and volume info from a FAT filesystem */ @@ -542,6 +589,12 @@ read_bootsectandvi(boot_sector *bs, volume_info *volinfo, int *fatsize) bs->heads = FAT2CPU16(bs->heads); bs->total_sect = FAT2CPU32(bs->total_sect);
+ if (!is_bootsector_valid(bs)) { + debug("Error: bootsector is invalid\n"); + ret = -1; + goto out_free; + } + /* FAT32 entries */ if (!bs->fat_length && bs->fat32_length) { /* Assume FAT32 */
participants (1)
-
christian.taedcke-oss＠weidmueller.com