[PATCH v1 1/3] exfat: add volume limit bounds checks

[PATCH v1 1/3] exfat: add volume limit bounds checks

[David Timber]

If the user inadvertenly truncates an exFAT volume(mistakenly shrinks
the partition or simply dd'ing from a larger removable device to a
smaller one. eg: device marketed as having 8GB capatity < 8GiB), the
kernel exFAT obliviously mounts the volume and operates on it. No error
is reported to userspace unless the filesystem is accessed with O_SYNC
or O_DIRECT.

Off by one sector test:

	# truncate -s 1073741824 img
	# mkfs.exfat img
	# truncate -s 1073741312 img
	# mount -t exfat img ...

The existing filesystem implementations, prime examples being XFS and
ext*, refuse to mount the volume with such condition. Introduce the
checks similar checks in-place to exFAT.

Also, to prevent UB, add checks against exFAT volumes with maliciously
a crafted main boot sectors with the ClusterCount field equal to or
larger than (2^32 - 11) as per format spec.

Link: https://github.com/exfatprogs/exfatprogs/issues/353
Signed-off-by: David Timber <dxdt@dev.snart.me>
---
 fs/exfat/super.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 46 insertions(+), 2 deletions(-)

diff --git a/fs/exfat/super.c b/fs/exfat/super.c
index 95d87e2d7717..15e99766cb8c 100644
--- a/fs/exfat/super.c
+++ b/fs/exfat/super.c
@@ -431,6 +431,7 @@ static int exfat_read_boot_sector(struct super_block *sb)
 {
 	struct boot_sector *p_boot;
 	struct exfat_sb_info *sbi = EXFAT_SB(sb);
+	unsigned int nb_clusters;
 
 	/* set block size to read super block */
 	if (!sb_min_blocksize(sb, 512)) {
@@ -501,8 +502,17 @@ static int exfat_read_boot_sector(struct super_block *sb)
 	sbi->data_start_sector = le32_to_cpu(p_boot->clu_offset);
 	sbi->num_sectors = le64_to_cpu(p_boot->vol_length);
 	/* because the cluster index starts with 2 */
-	sbi->num_clusters = le32_to_cpu(p_boot->clu_count) +
-		EXFAT_RESERVED_CLUSTERS;
+	nb_clusters = le32_to_cpu(p_boot->clu_count);
+	/*
+	 * The inclusive comparison in the following check seems a bit off(quite
+	 * literally), but the exFAT format section 3.1.9 says
+	 * "lesser of the following". Aye, aye, captain.
+	 */
+	if (nb_clusters >= EXFAT_MAX_NUM_CLUSTER) {
+		exfat_err(sb, "bogus number of clusters : %u", nb_clusters);
+		return -EINVAL;
+	}
+	sbi->num_clusters = nb_clusters + EXFAT_RESERVED_CLUSTERS;
 
 	sbi->root_dir = le32_to_cpu(p_boot->root_cluster);
 	sbi->dentries_per_clu = 1 <<
@@ -591,6 +601,34 @@ static int exfat_verify_boot_region(struct super_block *sb)
 	return 0;
 }
 
+static inline int exfat_check_volume_sizes(struct super_block *sb)
+{
+	struct exfat_sb_info *sbi = EXFAT_SB(sb);
+	/* last sector of exFAT volume == end of last cluster */
+	const sector_t lc_end = exfat_cluster_to_sector(sbi, sbi->num_clusters);
+	/*
+	 * Do the calculations in bytes because the blocksize may have been
+	 * calibrated in exfat_calibrate_blocksize(), and bdev_nr_sectors()
+	 * always reports in 512-byte units.
+	 */
+	const unsigned long long vol_size = EXFAT_BLK_TO_B(sbi->num_sectors, sb);
+	const unsigned long long bdev_size = (unsigned long long)bdev_nr_bytes(sb->s_bdev);
+
+	if (sbi->num_sectors < (unsigned long long)lc_end) {
+		exfat_err(sb, "number of clusters out of volume length bounds : num_sectors=%llu, last_cluster_sector=%lld",
+			  sbi->num_sectors, lc_end);
+		return -EINVAL;
+	}
+
+	if (bdev_size < vol_size) {
+		exfat_err(sb, "volume length out of bounds : dev=%llu, vol=%lld",
+			  bdev_size, vol_size);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 /* mount the file system volume */
 static int __exfat_fill_super(struct super_block *sb,
 		struct exfat_chain *root_clu)
@@ -610,6 +648,12 @@ static int __exfat_fill_super(struct super_block *sb,
 		goto free_bh;
 	}
 
+	ret = exfat_check_volume_sizes(sb);
+	if (ret) {
+		exfat_warn(sb, "volume bounds check failed. Please run fsck");
+		goto free_bh;
+	}
+
 	/*
 	 * Call exfat_count_num_cluster() before searching for up-case and
 	 * bitmap directory entries to avoid infinite loop if they are missing
-- 
2.53.0.1.ga224b40d3f.dirty

[PATCH v1 2/3] exfat: print warning upon block size calibration

[David Timber]

If the block size specified in the exFAT volume boot sector is different
from the actual logical block size of the device, many implementations
including FUSE-exfat, macos and previous versions of Windows are not
able to mount the volume.

A possible scenario in which this can happen is when the user dd's the
volume in a 4K-blocksize device("Advanced Format") to a 512-blocksize
device. This is a design issue inherent to the exFAT format itself
which layouts the structures of exFAT volumes aligned to the sector
size rather than large byte sizes as seen with other modern file
systems.

Print a kind warning about this potential compatibility issue.

Link: https://github.com/exfatprogs/exfatprogs/issues/349
Signed-off-by: David Timber <dxdt@dev.snart.me>
---
 fs/exfat/super.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/exfat/super.c b/fs/exfat/super.c
index 15e99766cb8c..e0a80d8f5f80 100644
--- a/fs/exfat/super.c
+++ b/fs/exfat/super.c
@@ -409,6 +409,8 @@ static int exfat_calibrate_blocksize(struct super_block *sb, int logical_sect)
 	}
 
 	if (logical_sect > sb->s_blocksize) {
+		const unsigned long saved_bs = sb->s_blocksize;
+
 		brelse(sbi->boot_bh);
 		sbi->boot_bh = NULL;
 
@@ -423,6 +425,10 @@ static int exfat_calibrate_blocksize(struct super_block *sb, int logical_sect)
 				  sb->s_blocksize);
 			return -EIO;
 		}
+
+		exfat_warn(sb, "blocksize calibrated from device logical block size(%lu) to volume sector size(%d)!\n"
+			   "Other implementations may not be able to handle this volume.",
+			   saved_bs, logical_sect);
 	}
 	return 0;
 }
-- 
2.53.0.1.ga224b40d3f.dirty

[PATCH v1 3/3] exfat: fix memory leak (upcase table)

[David Timber]

Fix memory leak conditions due to exfat_free_upcase_table() not being
called.

Signed-off-by: David Timber <dxdt@dev.snart.me>
---
 fs/exfat/super.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/exfat/super.c b/fs/exfat/super.c
index e0a80d8f5f80..e9dc1e616fac 100644
--- a/fs/exfat/super.c
+++ b/fs/exfat/super.c
@@ -707,6 +707,7 @@ static int __exfat_fill_super(struct super_block *sb,
 	exfat_free_bitmap(sbi);
 free_bh:
 	brelse(sbi->boot_bh);
+	exfat_free_upcase_table(sbi);
 	return ret;
 }
 
@@ -802,6 +803,7 @@ static int exfat_get_tree(struct fs_context *fc)
 
 static void exfat_free_sbi(struct exfat_sb_info *sbi)
 {
+	exfat_free_upcase_table(sbi);
 	exfat_free_iocharset(sbi);
 	kfree(sbi);
 }
-- 
2.53.0.1.ga224b40d3f.dirty

Re: [PATCH v1 3/3] exfat: fix memory leak (upcase table)

[Yuezhang.Mo]

> Fix memory leak conditions due to exfat_free_upcase_table() not being
> called.
> 
> Signed-off-by: David Timber <dxdt@dev.snart.me>
> ---
>  fs/exfat/super.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/exfat/super.c b/fs/exfat/super.c
> index e0a80d8f5f80..e9dc1e616fac 100644
> --- a/fs/exfat/super.c
> +++ b/fs/exfat/super.c
> @@ -707,6 +707,7 @@ static int __exfat_fill_super(struct super_block *sb,
>         exfat_free_bitmap(sbi);
>  free_bh:
>         brelse(sbi->boot_bh);
> +       exfat_free_upcase_table(sbi);

Although kvfree() checks for NULL, adding a new goto label would be better.

>         return ret;
>  }
> 
> @@ -802,6 +803,7 @@ static int exfat_get_tree(struct fs_context *fc)
> 
>  static void exfat_free_sbi(struct exfat_sb_info *sbi)
>  {
> +       exfat_free_upcase_table(sbi);

exfat_free_upcase_table() has already been called in delayed_free(), so it is
not needed here.

>         exfat_free_iocharset(sbi);
>         kfree(sbi);
>  }
> --
> 2.53.0.1.ga224b40d3f.dirty
> 

Re: [PATCH v1 3/3] exfat: fix memory leak (upcase table)

[David Timber]

On 5/7/26 21:04, Yuezhang.Mo@sony.com wrote:
>> @@ -802,6 +803,7 @@ static int exfat_get_tree(struct fs_context *fc)
>>
>>  static void exfat_free_sbi(struct exfat_sb_info *sbi)
>>  {
>> +       exfat_free_upcase_table(sbi);
> exfat_free_upcase_table() has already been called in delayed_free(), so it is
> not needed here.
Upon further inspection, I realised why exfat_free_upcase_table() is
called in delayed_free() and I'm terrified. With the current
implementation, the upcase table mustn't be freed there. I apologise.

Everyone will agree that leaving stale objects should be avoided if
possible. Yes, there's lock dependency problem, but I think someone has
to fully understand it in order to remove delayed_free(). I'll put it on
my list and revisit this in the future.

The leak condition in fill_super() is legit and needs to be fixed, though.

Re: [PATCH v1 1/3] exfat: add volume limit bounds checks

[Yuezhang.Mo]

> If the user inadvertenly truncates an exFAT volume(mistakenly shrinks
> the partition or simply dd'ing from a larger removable device to a
> smaller one. eg: device marketed as having 8GB capatity < 8GiB), the
> kernel exFAT obliviously mounts the volume and operates on it. No error
> is reported to userspace unless the filesystem is accessed with O_SYNC
> or O_DIRECT.
> 
> Off by one sector test:
> 
>         # truncate -s 1073741824 img
>         # mkfs.exfat img
>         # truncate -s 1073741312 img
>         # mount -t exfat img ...
> 
> The existing filesystem implementations, prime examples being XFS and
> ext*, refuse to mount the volume with such condition. Introduce the
> checks similar checks in-place to exFAT.
> 
> Also, to prevent UB, add checks against exFAT volumes with maliciously
> a crafted main boot sectors with the ClusterCount field equal to or
> larger than (2^32 - 11) as per format spec.
> 
> Link: https://github.com/exfatprogs/exfatprogs/issues/353
> Signed-off-by: David Timber <dxdt@dev.snart.me>
> ---
>  fs/exfat/super.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 46 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/exfat/super.c b/fs/exfat/super.c
> index 95d87e2d7717..15e99766cb8c 100644
> --- a/fs/exfat/super.c
> +++ b/fs/exfat/super.c
> @@ -431,6 +431,7 @@ static int exfat_read_boot_sector(struct super_block *sb)
>  {
>         struct boot_sector *p_boot;
>         struct exfat_sb_info *sbi = EXFAT_SB(sb);
> +       unsigned int nb_clusters;
> 
>         /* set block size to read super block */
>         if (!sb_min_blocksize(sb, 512)) {
> @@ -501,8 +502,17 @@ static int exfat_read_boot_sector(struct super_block *sb)
>         sbi->data_start_sector = le32_to_cpu(p_boot->clu_offset);
>         sbi->num_sectors = le64_to_cpu(p_boot->vol_length);
>         /* because the cluster index starts with 2 */
> -       sbi->num_clusters = le32_to_cpu(p_boot->clu_count) +
> -               EXFAT_RESERVED_CLUSTERS;
> +       nb_clusters = le32_to_cpu(p_boot->clu_count);
> +       /*
> +        * The inclusive comparison in the following check seems a bit off(quite
> +        * literally), but the exFAT format section 3.1.9 says
> +        * "lesser of the following". Aye, aye, captain.
> +        */
> +       if (nb_clusters >= EXFAT_MAX_NUM_CLUSTER) {
> +               exfat_err(sb, "bogus number of clusters : %u", nb_clusters);
> +               return -EINVAL;
> +       }
> +       sbi->num_clusters = nb_clusters + EXFAT_RESERVED_CLUSTERS;
> 
>         sbi->root_dir = le32_to_cpu(p_boot->root_cluster);
>         sbi->dentries_per_clu = 1 <<
> @@ -591,6 +601,34 @@ static int exfat_verify_boot_region(struct super_block *sb)
>         return 0;
>  }
> 
> +static inline int exfat_check_volume_sizes(struct super_block *sb)
> +{
> +       struct exfat_sb_info *sbi = EXFAT_SB(sb);
> +       /* last sector of exFAT volume == end of last cluster */
> +       const sector_t lc_end = exfat_cluster_to_sector(sbi, sbi->num_clusters);
> +       /*
> +        * Do the calculations in bytes because the blocksize may have been
> +        * calibrated in exfat_calibrate_blocksize(), and bdev_nr_sectors()
> +        * always reports in 512-byte units.
> +        */
> +       const unsigned long long vol_size = EXFAT_BLK_TO_B(sbi->num_sectors, sb);
> +       const unsigned long long bdev_size = (unsigned long long)bdev_nr_bytes(sb->s_bdev);
> +
> +       if (sbi->num_sectors < (unsigned long long)lc_end) {
> +               exfat_err(sb, "number of clusters out of volume length bounds : num_sectors=%llu, last_cluster_sector=%lld",
> +                         sbi->num_sectors, lc_end);
> +               return -EINVAL;
> +       }
> +
> +       if (bdev_size < vol_size) {
> +               exfat_err(sb, "volume length out of bounds : dev=%llu, vol=%lld",
> +                         bdev_size, vol_size);
> +               return -EINVAL;
> +       }

I checked the code further and found that exfat_zeroed_cluster()
already has a similar check, but it is only for directories and
does not check the size of the block device.

I think we should also add a check here, similar to the check
in exfat_zeroed_cluster(), to check if the last cluster exceeds
the device size. This way, the check will apply to both files and
directories, and the check in exfat_zeroed_cluster() will no
longer be needed.

Re: [PATCH v1 1/3] exfat: add volume limit bounds checks

[David Timber]

On 5/7/26 21:04, Yuezhang.Mo@sony.com wrote:
> I checked the code further and found that exfat_zeroed_cluster()
> already has a similar check, but it is only for directories and
> does not check the size of the block device.
>
> I think we should also add a check here, similar to the check
> in exfat_zeroed_cluster(), to check if the last cluster exceeds
> the device size. This way, the check will apply to both files and
> directories, and the check in exfat_zeroed_cluster() will no
> longer be needed.
Valid points. Redundant checks hurt consistency.

The existing checks can be changed to WARN_ON() later on, but I think
the priority for now should be preventing silent data loss from users
inadvertenly mounting a truncated volume.

Let's see if there are more checks like these in the codebase.