WARN fibmap user in case of possible addr truncation

We better warn the fibmap user and not return a truncated and therefore an incorrect block map address if the bmap() returned block address is greater than INT_MAX (since user supplied integer pointer).

It's better to WARN all user of ioctl_fibmap() and return a proper error code rather than silently letting a FS corruption happen if the user tries to fiddle around with the returned block map address.

We fix this by returning an error code of -ERANGE and returning 0 as the block mapping address in case if it is > INT_MAX.

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

---
 fs/ioctl.c | 5 +++++
 1 file changed, 5 insertions(+)

fs/ioctl.c +5 -0

index f1d93263186c..3489f3a12c1d 100644
@@ -71,6 +71,11 @@ static int ioctl_fibmap(struct file *filp, int __user *p)
 	block = ur_block;
 	error = bmap(inode, &block);
 
+	if (block > INT_MAX) {
+		error = -ERANGE;
+		WARN(1, "would truncate fibmap result\n");
+	}
+
 	if (error)
 		ur_block = 0;
 	else

Signature · 1 line

2.21.0

iomap_bmap() could be called from either of these two paths. Either when a user is calling an ioctl_fibmap() interface to get the block mapping address or by some filesystem via use of bmap() internal kernel API.
bmap() kernel API is well equipped with handling of u64 addresses.

WARN condition in iomap_bmap_actor() was mainly added to warn all the fibmap users. But now that in previous patch we have directly added this WARN condition for all fibmap users and also made sure to return 0 as block map address in case if addr > INT_MAX.
So we can now remove this logic from here.

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

---
 fs/iomap/fiemap.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

fs/iomap/fiemap.c +1 -4

index bccf305ea9ce..d55e8f491a5e 100644
@@ -117,10 +117,7 @@ iomap_bmap_actor(struct inode *inode, loff_t pos, loff_t length,
 
 	if (iomap->type == IOMAP_MAPPED) {
 		addr = (pos - iomap->offset + iomap->addr) >> inode->i_blkbits;
-		if (addr > INT_MAX)
-			WARN(1, "would truncate bmap result\n");
-		else
-			*bno = addr;
+		*bno = addr;
 	}
 	return 0;
 }

Signature · 1 line

2.21.0

We better warn the fibmap user and not return a truncated and therefore an incorrect block map address if the bmap() returned block address is greater than INT_MAX (since user supplied integer pointer).

It's better to WARN all user of ioctl_fibmap() and return a proper error code rather than silently letting a FS corruption happen if the user tries to fiddle around with the returned block map address.

We fix this by returning an error code of -ERANGE and returning 0 as the block mapping address in case if it is > INT_MAX.

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

iomap_bmap() could be called from either of these two paths. Either when a user is calling an ioctl_fibmap() interface to get the block mapping address or by some filesystem via use of bmap() internal kernel API.
bmap() kernel API is well equipped with handling of u64 addresses.

WARN condition in iomap_bmap_actor() was mainly added to warn all the fibmap users. But now that in previous patch we have directly added this WARN condition for all fibmap users and also made sure to return 0 as block map address in case if addr > INT_MAX.
So we can now remove this logic from here.

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

---
 fs/iomap/fiemap.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

fs/iomap/fiemap.c +0 -0

index bccf305ea9ce..d55e8f491a5e 100644

fs/iomap/fiemap.c +1 -2

-		else
-			*bno = addr;
+		*bno = addr;
 	}
 	return 0;
 }

Signature · 1 line

2.21.0

We better warn the fibmap user and not return a truncated and therefore an incorrect block map address if the bmap() returned block address is greater than INT_MAX (since user supplied integer pointer).

It's better to WARN all user of ioctl_fibmap() and return a proper error code rather than silently letting a FS corruption happen if the user tries to fiddle around with the returned block map address.

We fix this by returning an error code of -ERANGE and returning 0 as the block mapping address in case if it is > INT_MAX.

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

---
 fs/ioctl.c | 5 +++++
 1 file changed, 5 insertions(+)

fs/ioctl.c +0 -0

index f1d93263186c..3489f3a12c1d 100644

Quoted 23 lines · Ritesh Harjani

iomap_bmap() could be called from either of these two paths. Either when a user is calling an ioctl_fibmap() interface to get the block mapping address or by some filesystem via use of bmap() internal kernel API.
bmap() kernel API is well equipped with handling of u64 addresses.

WARN condition in iomap_bmap_actor() was mainly added to warn all the fibmap users. But now that in previous patch we have directly added this WARN condition for all fibmap users and also made sure to return 0 as block map address in case if addr > INT_MAX.
So we can now remove this logic from here.

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

---
  fs/iomap/fiemap.c | 5 +----
  1 file changed, 1 insertion(+), 4 deletions(-)

fs/iomap/fiemap.c +0 -0

index bccf305ea9ce..d55e8f491a5e 100644
@@ -117,10 +117,7 @@ iomap_bmap_actor(struct inode *inode, loff_t pos, loff_t length,
 

fs/iomap/fiemap.c iomap_bmap_actor(struct inode *inode, loff_t pos, loff_t length, · line 121

Quoted 4 lines

fs/iomap/fiemap.c +0 -2

  	if (iomap->type == IOMAP_MAPPED) {
  		addr = (pos - iomap->offset + iomap->addr) >> inode->i_blkbits;
-		if (addr > INT_MAX)
-			WARN(1, "would truncate bmap result\n");

Frankly I would've combined these two patches to make it more obvious that we're hoisting a FIBMAP constraint check from iomap into the ioctl handler.

--D

Quoted 9 lines

fs/iomap/fiemap.c +1 -2

-		else
-			*bno = addr;
+		*bno = addr;
  	}
  	return 0;
  }

Signature · 1 line

2.21.0

Quoted 24 lines · Ritesh Harjani

We better warn the fibmap user and not return a truncated and therefore an incorrect block map address if the bmap() returned block address is greater than INT_MAX (since user supplied integer pointer).

It's better to WARN all user of ioctl_fibmap() and return a proper error code rather than silently letting a FS corruption happen if the user tries to fiddle around with the returned block map address.

We fix this by returning an error code of -ERANGE and returning 0 as the block mapping address in case if it is > INT_MAX.

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

---
  fs/ioctl.c | 5 +++++
  1 file changed, 5 insertions(+)

fs/ioctl.c +0 -0

index f1d93263186c..3489f3a12c1d 100644
@@ -71,6 +71,11 @@ static int ioctl_fibmap(struct file *filp, int __user *p)
  	block = ur_block;
  	error = bmap(inode, &block);
 

fs/ioctl.c static int ioctl_fibmap(struct file *filp, int __user *p) · line 78

Quoted 5 lines

fs/ioctl.c +5 -0

+	if (block > INT_MAX) {
+		error = -ERANGE;
+		WARN(1, "would truncate fibmap result\n");
+	}
+

WARN() is only for kernel bugs. This case would be a userspace bug, not a kernel bug, right? If so, it should use pr_warn(), not WARN().

- Eric

Ok, I see.
Let me replace WARN() with below pr_warn() line then. If no objections, then will send this in a v2 with both patches combined as Darrick suggested. - (with Reviewed-by tags of Jan & Christoph).

pr_warn("fibmap: this would truncate fibmap result\n");

Quoted 6 lines · Ritesh Harjani

Ok, I see.
Let me replace WARN() with below pr_warn() line then. If no objections, then will send this in a v2 with both patches combined as Darrick suggested. - (with Reviewed-by tags of Jan & Christoph).

pr_warn("fibmap: this would truncate fibmap result\n");

We generally don't like userspace to be able to trigger kernel messages on demand, so they can't swamp the logfiles. printk_ratelimited()?

Quoted 6 lines · Matthew Wilcox

Quoted 5 lines · Ritesh Harjani

Ok, I see.
Let me replace WARN() with below pr_warn() line then. If no objections, then will send this in a v2 with both patches combined as Darrick suggested. - (with Reviewed-by tags of Jan & Christoph).

Quoted 4 lines

Quoted 1 line

pr_warn("fibmap: this would truncate fibmap result\n");

We generally don't like userspace to be able to trigger kernel messages on demand, so they can't swamp the logfiles. printk_ratelimited()?

Or WARN_ON_ONCE...

Quoted 7 lines · Darrick J. Wong

Quoted 6 lines · Matthew Wilcox

Quoted 5 lines · Ritesh Harjani

Ok, I see.
Let me replace WARN() with below pr_warn() line then. If no objections, then will send this in a v2 with both patches combined as Darrick suggested. - (with Reviewed-by tags of Jan & Christoph).

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

pr_warn("fibmap: this would truncate fibmap result\n");

We generally don't like userspace to be able to trigger kernel messages on demand, so they can't swamp the logfiles. printk_ratelimited()?

Or WARN_ON_ONCE...

So, Eric was mentioning WARN_** are mostly for kernel side of bugs. But this is mostly a API fault which affects user side and also to warn the user about the possible truncation in the block fibmap addr.
Also WARN_ON_ONCE, will be shown only once and won't be printed for every other file for which block addr > INT_MAX.

I think we could go with below. If ok, I could post this in v2.

pr_warn_ratelimited("fibmap: would truncate fibmap result\n");

Quoted 24 lines · Ritesh Harjani

Quoted 7 lines · Darrick J. Wong

Quoted 6 lines · Matthew Wilcox

Quoted 5 lines · Ritesh Harjani

Ok, I see.
Let me replace WARN() with below pr_warn() line then. If no objections, then will send this in a v2 with both patches combined as Darrick suggested. - (with Reviewed-by tags of Jan & Christoph).

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

pr_warn("fibmap: this would truncate fibmap result\n");

We generally don't like userspace to be able to trigger kernel messages on demand, so they can't swamp the logfiles. printk_ratelimited()?

Or WARN_ON_ONCE...

So, Eric was mentioning WARN_** are mostly for kernel side of bugs. But this is mostly a API fault which affects user side and also to warn the user about the possible truncation in the block fibmap addr.
Also WARN_ON_ONCE, will be shown only once and won't be printed for every other file for which block addr > INT_MAX.

Quoted 3 lines

I think we could go with below. If ok, I could post this in v2.

pr_warn_ratelimited("fibmap: would truncate fibmap result\n");

Please include the process ID, the superblock ID and the task name that is triggering this warning. Otherwise the administrator will have no clue what is generating it and so won't be able to fix it...