ext4/overlayfs: fiemap related fixes

ext4 supports max number of logical blocks in a file to be 0xffffffff. (This is since ext4_extent's ee_block is __le32).
This means that EXT4_MAX_LOGICAL_BLOCK should be 0xfffffffe (starting from 0 logical offset). This patch fixes this.

The issue was seen when ext4 moved to iomap_fiemap API and when overlayfs was mounted on top of ext4. Since overlayfs was missing filemap_check_ranges(), so it could pass a arbitrary huge length which lead to overflow of map.m_len logic.

This patch fixes that.

Fixes d3b6f23f7167 ("ext4: move ext4_fiemap to use iomap framework")

Reported-by [email protected]

Signed-off-by Ritesh Harjani <[email protected]>

---
 fs/ext4/ext4.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/ext4/ext4.h +1 -1

index 91eb4381cae5..ad2dbf6e4924 100644
@@ -722,7 +722,7 @@ enum {
 #define EXT4_MAX_BLOCK_FILE_PHYS	0xFFFFFFFF
 
 /* Max logical block we can support */
-#define EXT4_MAX_LOGICAL_BLOCK		0xFFFFFFFF
+#define EXT4_MAX_LOGICAL_BLOCK		0xFFFFFFFE
 
 /*
  * Structure of an inode on the disk

Signature · 1 line

2.21.0

This renames the fiemap_check_ranges() copy of function within ext4/ioctl.c to become ext4_fiemap_check_ranges(). This is required so that we can finally get rid of this duplicate version.
Since overlayfs anyways need to use this in it's ovl_fiemap() function, so later patches make it available for use by others via EXPORT_SYMBOL.

Signed-off-by Ritesh Harjani <[email protected]>

---
 fs/ext4/ioctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

fs/ext4/ioctl.c +2 -2

index bfc1281fc4cb..76a2b5200ba3 100644
@@ -734,7 +734,7 @@ static void ext4_fill_fsxattr(struct inode *inode, struct fsxattr *fa)
 }
 
 /* copied from fs/ioctl.c */
-static int fiemap_check_ranges(struct super_block *sb,
+static int ext4_fiemap_check_ranges(struct super_block *sb,
 			       u64 start, u64 len, u64 *new_len)
 {
 	u64 maxbytes = (u64) sb->s_maxbytes;

@@ -775,7 +775,7 @@ static int ext4_ioctl_get_es_cache(struct file *filp, unsigned long arg)
 	if (fiemap.fm_extent_count > FIEMAP_MAX_EXTENTS)
 		return -EINVAL;
 
-	error = fiemap_check_ranges(sb, fiemap.fm_start, fiemap.fm_length,
+	error = ext4_fiemap_check_ranges(sb, fiemap.fm_start, fiemap.fm_length,
 				    &len);
 	if (error)
 		return error;

Signature · 1 line

2.21.0

1. fiemap_check_ranges() is needed by ovl_fiemap() to check for ranges before calling underlying inode's ->fiemap() call.
2. With this change even ext4 can use generic fiemap_check_ranges() instead of having a duplicate copy of it.

So make this EXPORT_SYMBOL for use by overlayfs.

Signed-off-by Ritesh Harjani <[email protected]>

---
 fs/ioctl.c         | 5 +++--
 include/linux/fs.h | 2 ++
 2 files changed, 5 insertions(+), 2 deletions(-)

fs/ioctl.c +3 -2

index 282d45be6f45..f1d93263186c 100644
@@ -166,8 +166,8 @@ int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags)
 }
 EXPORT_SYMBOL(fiemap_check_flags);
 
-static int fiemap_check_ranges(struct super_block *sb,
-			       u64 start, u64 len, u64 *new_len)
+int fiemap_check_ranges(struct super_block *sb, u64 start, u64 len,
+			u64 *new_len)
 {
 	u64 maxbytes = (u64) sb->s_maxbytes;
 

@@ -187,6 +187,7 @@ static int fiemap_check_ranges(struct super_block *sb,
 
 	return 0;
 }
+EXPORT_SYMBOL(fiemap_check_ranges);
 
 static int ioctl_fiemap(struct file *filp, struct fiemap __user *ufiemap)
 {

include/linux/fs.h +2 -0

index 4f6f59b4f22a..1ea70fe07618 100644
@@ -1759,6 +1759,8 @@ int fiemap_fill_next_extent(struct fiemap_extent_info *info, u64 logical,
 			    u64 phys, u64 len, u32 flags);
 int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags);
 
+int fiemap_check_ranges(struct super_block *sb, u64 start, u64 len,
+			u64 *new_len);
 /*
  * This is the "filldir" function type, used by readdir() to let
  * the kernel specify what kind of dirent layout it wants to have.

Signature · 1 line

2.21.0

Underlying fs may not be able to handle the length in fiemap beyond sb->s_maxbytes. So similar to how VFS ioctl does it, add fiemap_check_ranges() check in ovl_fiemap() as well before calling underlying fs i_op->fiemap() call.

Signed-off-by Ritesh Harjani <[email protected]>

---
 fs/overlayfs/inode.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

fs/overlayfs/inode.c +6 -1

index 79e8994e3bc1..9bcd2e96faad 100644
@@ -455,16 +455,21 @@ static int ovl_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 	int err;
 	struct inode *realinode = ovl_inode_real(inode);
 	const struct cred *old_cred;
+	u64 length;
 
 	if (!realinode->i_op->fiemap)
 		return -EOPNOTSUPP;
 
+	err = fiemap_check_ranges(realinode->i_sb, start, len, &length);
+	if (err)
+		return err;
+
 	old_cred = ovl_override_creds(inode->i_sb);
 
 	if (fieinfo->fi_flags & FIEMAP_FLAG_SYNC)
 		filemap_write_and_wait(realinode->i_mapping);
 
-	err = realinode->i_op->fiemap(realinode, fieinfo, start, len);
+	err = realinode->i_op->fiemap(realinode, fieinfo, start, length);
 	revert_creds(old_cred);
 
 	return err;

Signature · 1 line

2.21.0

Now that fiemap_check_ranges() is available for other filesystems to use, so get rid of ext4's private version.

Signed-off-by Ritesh Harjani <[email protected]>

---
 fs/ext4/ioctl.c | 25 +------------------------
 1 file changed, 1 insertion(+), 24 deletions(-)

fs/ext4/ioctl.c +1 -24

index 76a2b5200ba3..6a7d7e9027cd 100644
@@ -733,29 +733,6 @@ static void ext4_fill_fsxattr(struct inode *inode, struct fsxattr *fa)
 		fa->fsx_projid = from_kprojid(&init_user_ns, ei->i_projid);
 }
 
-/* copied from fs/ioctl.c */
-static int ext4_fiemap_check_ranges(struct super_block *sb,
-			       u64 start, u64 len, u64 *new_len)
-{
-	u64 maxbytes = (u64) sb->s_maxbytes;
-
-	*new_len = len;
-
-	if (len == 0)
-		return -EINVAL;
-
-	if (start > maxbytes)
-		return -EFBIG;
-
-	/*
-	 * Shrink request scope to what the fs can actually handle.
-	 */
-	if (len > maxbytes || (maxbytes - len) < start)
-		*new_len = maxbytes - start;
-
-	return 0;
-}
-
 /* So that the fiemap access checks can't overflow on 32 bit machines. */
 #define FIEMAP_MAX_EXTENTS	(UINT_MAX / sizeof(struct fiemap_extent))
 

@@ -775,7 +752,7 @@ static int ext4_ioctl_get_es_cache(struct file *filp, unsigned long arg)
 	if (fiemap.fm_extent_count > FIEMAP_MAX_EXTENTS)
 		return -EINVAL;
 
-	error = ext4_fiemap_check_ranges(sb, fiemap.fm_start, fiemap.fm_length,
+	error = fiemap_check_ranges(sb, fiemap.fm_start, fiemap.fm_length,
 				    &len);
 	if (error)
 		return error;

Signature · 1 line

2.21.0

I think the right fix is to move fiemap_check_ranges into all the ->fiemap

instances (we only have a few actual implementation minus the wrappers around iomap/generic).

Then add a version if iomap_fiemap that can pass
in maxbytes explicitly for ext4, similar to what we've done with various other generic helpers.

The idea of validating input against file systems specific paramaters before we call into the fs is just bound to cause problems.

Underlying fs may not be able to handle the length in fiemap beyond sb->s_maxbytes. So similar to how VFS ioctl does it, add fiemap_check_ranges() check in ovl_fiemap() as well before calling underlying fs i_op->fiemap() call.

Signed-off-by Ritesh Harjani <[email protected]>

---
 fs/overlayfs/inode.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

fs/overlayfs/inode.c +0 -0

index 79e8994e3bc1..9bcd2e96faad 100644

Hello Christoph,

Thanks for your review comments.

Quoted 1 line · Christoph Hellwig

I think the right fix is to move fiemap_check_ranges into all the ->fiemap

I do welcome your suggestion here. But I am not sure of what you are suggesting should be done as a 1st round of changes for the immediate reported problem.
So currently these patches take the same approach on overlayfs on how VFS does it. So as a fix to the overlayfs over ext4 reported problems in thread [1] & [2]. I think these patches are doing the right thing.

Also maybe I am biased in some way because as I see these are the right fixes with minimal changes only at places which does have a direct problem.

FWIW, I agree with you.
And seems like Jan does as well, since he ACKed all your patches. Current patches would be easier to backport to stable kernels.

Quoted 15 lines · Ritesh Harjani

Hello Christoph,

Thanks for your review comments.

Quoted 1 line · Christoph Hellwig

I think the right fix is to move fiemap_check_ranges into all the ->fiemap

I do welcome your suggestion here. But I am not sure of what you are suggesting should be done as a 1st round of changes for the immediate reported problem.
So currently these patches take the same approach on overlayfs on how VFS does it. So as a fix to the overlayfs over ext4 reported problems in thread [1] & [2]. I think these patches are doing the right thing.

Quoted 4 lines

Also maybe I am biased in some way because as I see these are the right fixes with minimal changes only at places which does have a direct problem.

FWIW, I agree with you.
And seems like Jan does as well, since he ACKed all your patches. Current patches would be easier to backport to stable kernels.

Plus, if we are going to cleanup the fiemap interface, need to look into FIEMAP_FLAG_SYNC handling.
Does it makes sense to handle this flag in vfs ioctl code and other flags by filesystem code?
See, iomap_fiemap() takes care of FIEMAP_FLAG_SYNC in addition to ioctl_fiemap(), so I would think that FIEMAP_FLAG_SYNC should probably be removed from ioctl_fiemap() and handled by generic_block_fiemap() and other filesystem specific implementation.

Quoted 3 lines · Amir Goldstein

FWIW, I agree with you.
And seems like Jan does as well, since he ACKed all your patches. Current patches would be easier to backport to stable kernels.

Honestly, the proper fix is pretty much trivial. I wrote it up this morning over coffee:

http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/fiemap-fix

Still needs more testing, though.

Quoted 6 lines · Christoph Hellwig

Quoted 3 lines · Amir Goldstein

FWIW, I agree with you.
And seems like Jan does as well, since he ACKed all your patches. Current patches would be easier to backport to stable kernels.

Quoted 6 lines

Honestly, the proper fix is pretty much trivial. I wrote it up this morning over coffee:

http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/fiemap-fix

Still needs more testing, though.

Very slick!

I still think Ritesh's patches are easier for backporting because they are mostly contained within the ext4/overlayfs subsystems and your patch can follow up as interface cleanup.

I would use as generic helper name generic_fiemap_checks() akin to generic_write_checks() and generic_remap_file_range_prep() => generic_remap_checks().

Quoted 3 lines · Amir Goldstein

FWIW, I agree with you.
And seems like Jan does as well, since he ACKed all your patches. Current patches would be easier to backport to stable kernels.

Honestly, the proper fix is pretty much trivial. I wrote it up this morning over coffee:

http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/fiemap-fix

Still needs more testing, though.

I would use as generic helper name generic_fiemap_checks() akin to generic_write_checks() and generic_remap_file_range_prep() => generic_remap_checks().

Quoted 9 lines · Christoph Hellwig

Quoted 3 lines · Amir Goldstein

FWIW, I agree with you.
And seems like Jan does as well, since he ACKed all your patches. Current patches would be easier to backport to stable kernels.

Honestly, the proper fix is pretty much trivial. I wrote it up this morning over coffee:

Quoted 3 lines

http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/fiemap-fix

Still needs more testing, though.

The "maxbytes" value should be passed in from the caller, since this may be different per inode (for ext4 at least).

Quoted 3 lines · Amir Goldstein

I would use as generic helper name generic_fiemap_checks() akin to generic_write_checks() and generic_remap_file_range_prep() => generic_remap_checks().

None of the other fiemap helpers use the redundant generic_ prefix.

Quoted 7 lines · Christoph Hellwig

Quoted 3 lines · Amir Goldstein

I would use as generic helper name generic_fiemap_checks() akin to generic_write_checks() and generic_remap_file_range_prep() => generic_remap_checks().

None of the other fiemap helpers use the redundant generic_ prefix.

Fine. I still don't like the name _validate() so much because it implies yes or no, not length truncating.

What's more, if we decide that FIEMAP_FLAG_SYNC handling should be done inside this generic helper, we would definitely need to rename it again. So how about going for something a bit more abstract like fiemap_prep() or whatever.

What is your take about FIEMAP_FLAG_SYNC handling btw?

Hello All,

Here are some changes, which as I understand, takes the right approach in fixing the offset/length bounds check problem reported in threads [1]-[2]. These warnings in iomap_apply/ext4 path are reported after ext4_fiemap() was moved to use iomap framework and when overlayfs is mounted on top of ext4. Though the issues were identified after ext4 moved to iomap framework, but these changes tries to fix the problem which are anyways present in current code irrespective of ext4 using iomap framework for fiemap or not.

Patch 1 & 4 commit msg may give more details of the problem.

Tests done
==========
1. Tested xfstest-suite with "-g quick" & "-overlay -g quick" configuration on a 4k blocksize on x86 & Power. There were no new failures reported due to these changes.
2. Tested syzcaller reported problem with this change. [1]
3. Tested below change which was reported by Murphy. [2]
The minimal reproducer is:
-------------------------------------
fallocate -l 256M test.img
mkfs.ext4 -Fq -b 4096 -I 256 test.img
mkdir -p test
mount -o loop test.img test || exit
pushd test
rm -rf l u w m
mkdir -p l u w m
mount -t overlay -o lowerdir=l,upperdir=u,workdir=w overlay m || exit xfs_io -f -c "pwrite 0 4096" -c "fiemap" m/tf
umount m
rm -rf l u w m
popd
umount -d test
rm -rf test test.img
-------------------------------------

Comments/feedback are much welcome!!

References
==========
[1]: https://lkml.org/lkml/2020/4/11/46
[2]: https://patchwork.ozlabs.org/project/linux-ext4/patch/[email protected]/

Ritesh Harjani (5):
ext4: Fix EXT4_MAX_LOGICAL_BLOCK macro
ext4: Rename fiemap_check_ranges() to make it ext4 specific
vfs: EXPORT_SYMBOL for fiemap_check_ranges()
overlayfs: Check for range bounds before calling i_op->fiemap()
ext4: Get rid of ext4_fiemap_check_ranges

 fs/ext4/ext4.h       |  2 +-
 fs/ext4/ioctl.c      | 23 -----------------------
 fs/ioctl.c           |  5 +++--
 fs/overlayfs/inode.c |  7 ++++++-
 include/linux/fs.h   |  2 ++
 5 files changed, 12 insertions(+), 27 deletions(-)

Signature · 1 line

2.21.0

Quoted 9 lines · Ritesh Harjani

Hello All,

Here are some changes, which as I understand, takes the right approach in fixing the offset/length bounds check problem reported in threads [1]-[2]. These warnings in iomap_apply/ext4 path are reported after ext4_fiemap() was moved to use iomap framework and when overlayfs is mounted on top of ext4. Though the issues were identified after ext4 moved to iomap framework, but these changes tries to fix the problem which are anyways present in current code irrespective of ext4 using iomap framework for fiemap or not.

Ping?

Hello Murphy,

Quoted 11 lines · Murphy Zhou

Quoted 9 lines · Ritesh Harjani

Hello All,

Here are some changes, which as I understand, takes the right approach in fixing the offset/length bounds check problem reported in threads [1]-[2]. These warnings in iomap_apply/ext4 path are reported after ext4_fiemap() was moved to use iomap framework and when overlayfs is mounted on top of ext4. Though the issues were identified after ext4 moved to iomap framework, but these changes tries to fix the problem which are anyways present in current code irrespective of ext4 using iomap framework for fiemap or not.

Quoted 1 line

Ping?

It's superseded by below mentioned patch series.
Please follow below thread.
https://lore.kernel.org/linux-ext4/[email protected]/T/#t

-ritesh