[PATCH 0/2] WARN fibmap user in case of possible addr truncation

[PATCH 0/2] WARN fibmap user in case of possible addr truncation

[Ritesh Harjani]

Currently an ioctl_fibmap() caller may get a truncated and wrong block map
address if the actual mapped physical block address is > INT_MAX.
If that is the case then lets:-
1. Add a warning,
2. Returns 0 in the block mapped address,
3. And also return -ERANGE error.
This is better than providing a wrong information to the user about the
block mapping which in the worst case could cause a FS corruption
(unknowingly by the caller since kernel didn't warn).
Currently these checks are in place only for the filesystems who uses iomap
based bmap interface. So let's do this right thing for all the fibmap()
callers by adding this logic in ioctl_fibmap() directly.

Patch-1 & Patch-2 commit msg may provide additional information.

Ritesh Harjani (2):
  fibmap: Warn and return an error in case of block > INT_MAX
  iomap: bmap: Remove the WARN and return the proper block address

 fs/ioctl.c        | 5 +++++
 fs/iomap/fiemap.c | 5 +----
 2 files changed, 6 insertions(+), 4 deletions(-)

-- 
2.21.0

[PATCH 1/2] fibmap: Warn and return an error in case of block > INT_MAX

[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 <riteshh@linux.ibm.com>
---
 fs/ioctl.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index f1d93263186c..3489f3a12c1d 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -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
-- 
2.21.0

Re: [PATCH 1/2] fibmap: Warn and return an error in case of block > INT_MAX

[Jan Kara]

On Fri 24-04-20 12:52:17, Ritesh Harjani wrote:
> 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 <riteshh@linux.ibm.com>

The patch looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ioctl.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index f1d93263186c..3489f3a12c1d 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -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
> -- 
> 2.21.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/2] fibmap: Warn and return an error in case of block > INT_MAX

[Christoph Hellwig]

On Fri, Apr 24, 2020 at 12:52:17PM +0530, Ritesh Harjani wrote:
> 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 <riteshh@linux.ibm.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 1/2] fibmap: Warn and return an error in case of block > INT_MAX

[Eric Biggers]

On Fri, Apr 24, 2020 at 12:52:17PM +0530, Ritesh Harjani wrote:
> 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 <riteshh@linux.ibm.com>
> ---
>  fs/ioctl.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index f1d93263186c..3489f3a12c1d 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -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");
> +	}
> +

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

Re: [PATCH 1/2] fibmap: Warn and return an error in case of block > INT_MAX

[Ritesh Harjani]

On 4/25/20 12:47 AM, Eric Biggers wrote:
> On Fri, Apr 24, 2020 at 12:52:17PM +0530, Ritesh Harjani wrote:
>> 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 <riteshh@linux.ibm.com>
>> ---
>>   fs/ioctl.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/fs/ioctl.c b/fs/ioctl.c
>> index f1d93263186c..3489f3a12c1d 100644
>> --- a/fs/ioctl.c
>> +++ b/fs/ioctl.c
>> @@ -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");
>> +	}
>> +
> 
> 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().

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");


> 
> - Eric
> 

Re: [PATCH 1/2] fibmap: Warn and return an error in case of block > INT_MAX

[Matthew Wilcox]

On Sat, Apr 25, 2020 at 04:24:24AM +0530, Ritesh Harjani wrote:
> 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()?

Re: [PATCH 1/2] fibmap: Warn and return an error in case of block > INT_MAX

[Darrick J. Wong]

On Fri, Apr 24, 2020 at 04:40:58PM -0700, Matthew Wilcox wrote:
> On Sat, Apr 25, 2020 at 04:24:24AM +0530, Ritesh Harjani wrote:
> > 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()?

Or WARN_ON_ONCE...

--D

Re: [PATCH 1/2] fibmap: Warn and return an error in case of block > INT_MAX

[Ritesh Harjani]

On 4/25/20 5:16 AM, Darrick J. Wong wrote:
> On Fri, Apr 24, 2020 at 04:40:58PM -0700, Matthew Wilcox wrote:
>> On Sat, Apr 25, 2020 at 04:24:24AM +0530, Ritesh Harjani wrote:
>>> 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()?
> 
> 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");

-ritesh

Re: [PATCH 1/2] fibmap: Warn and return an error in case of block > INT_MAX

[Dave Chinner]

On Sat, Apr 25, 2020 at 12:33:34PM +0530, Ritesh Harjani wrote:
> 
> 
> On 4/25/20 5:16 AM, Darrick J. Wong wrote:
> > On Fri, Apr 24, 2020 at 04:40:58PM -0700, Matthew Wilcox wrote:
> > > On Sat, Apr 25, 2020 at 04:24:24AM +0530, Ritesh Harjani wrote:
> > > > 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()?
> > 
> > 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");

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...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/2] fibmap: Warn and return an error in case of block > INT_MAX

[Ritesh Harjani]

On 4/27/20 6:34 AM, Dave Chinner wrote:
> On Sat, Apr 25, 2020 at 12:33:34PM +0530, Ritesh Harjani wrote:
>>
>>
>> On 4/25/20 5:16 AM, Darrick J. Wong wrote:
>>> On Fri, Apr 24, 2020 at 04:40:58PM -0700, Matthew Wilcox wrote:
>>>> On Sat, Apr 25, 2020 at 04:24:24AM +0530, Ritesh Harjani wrote:
>>>>> 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()?
>>>
>>> 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");
> 
> 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...
> 
Thanks for the suggestion. I will make it like below then.
Will send a v2 soon.

+		pr_warn_ratelimited("[%s/%d] FS (%s): would truncate fibmap result\n",
+				    current->comm, task_pid_nr(current),
+				    sb->s_id);
+

[PATCH 2/2] iomap: bmap: Remove the WARN and return the proper block address

[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 <riteshh@linux.ibm.com>
---
 fs/iomap/fiemap.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/fs/iomap/fiemap.c b/fs/iomap/fiemap.c
index bccf305ea9ce..d55e8f491a5e 100644
--- a/fs/iomap/fiemap.c
+++ b/fs/iomap/fiemap.c
@@ -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;
 }
-- 
2.21.0

Re: [PATCH 2/2] iomap: bmap: Remove the WARN and return the proper block address

[Jan Kara]

On Fri 24-04-20 12:52:18, Ritesh Harjani wrote:
> 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 <riteshh@linux.ibm.com>

Yes, I agree it's better to hadle the overflow in the ioctl than in the iomap
actor. The patch looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/iomap/fiemap.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/fs/iomap/fiemap.c b/fs/iomap/fiemap.c
> index bccf305ea9ce..d55e8f491a5e 100644
> --- a/fs/iomap/fiemap.c
> +++ b/fs/iomap/fiemap.c
> @@ -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;
>  }
> -- 
> 2.21.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/2] iomap: bmap: Remove the WARN and return the proper block address

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/2] iomap: bmap: Remove the WARN and return the proper block address

[Darrick J. Wong]

On Fri, Apr 24, 2020 at 12:52:18PM +0530, Ritesh Harjani wrote:
> 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 <riteshh@linux.ibm.com>
> ---
>  fs/iomap/fiemap.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/fs/iomap/fiemap.c b/fs/iomap/fiemap.c
> index bccf305ea9ce..d55e8f491a5e 100644
> --- a/fs/iomap/fiemap.c
> +++ b/fs/iomap/fiemap.c
> @@ -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");

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

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

Re: [PATCH 2/2] iomap: bmap: Remove the WARN and return the proper block address

[Ritesh Harjani]

On 4/24/20 11:18 PM, Darrick J. Wong wrote:
> On Fri, Apr 24, 2020 at 12:52:18PM +0530, Ritesh Harjani wrote:
>> 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 <riteshh@linux.ibm.com>
>> ---
>>   fs/iomap/fiemap.c | 5 +----
>>   1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/fs/iomap/fiemap.c b/fs/iomap/fiemap.c
>> index bccf305ea9ce..d55e8f491a5e 100644
>> --- a/fs/iomap/fiemap.c
>> +++ b/fs/iomap/fiemap.c
>> @@ -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");
> 
> 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.

Sure, let me combine the two in v2.

Thanks!!
-ritesh

> 
> --D
> 
>> -		else
>> -			*bno = addr;
>> +		*bno = addr;
>>   	}
>>   	return 0;
>>   }
>> -- 
>> 2.21.0
>>