[PATCHv2] fibmap: Warn and return an error in case of block > INT_MAX

[PATCHv2] 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 pr_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.

Now 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 we have directly added this warning
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 iomap_bmap_actor().

Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
---
v1 -> v2: 
1. Improved error log msg.
2. Clubbed both iomap change and fibmap change into 1 patch.
3. Added Reviewed-by tags.

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

diff --git a/fs/ioctl.c b/fs/ioctl.c
index f1d93263186c..adc1e8178c43 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -55,6 +55,7 @@ EXPORT_SYMBOL(vfs_ioctl);
 static int ioctl_fibmap(struct file *filp, int __user *p)
 {
 	struct inode *inode = file_inode(filp);
+	struct super_block *sb = inode->i_sb;
 	int error, ur_block;
 	sector_t block;
 
@@ -71,6 +72,13 @@ static int ioctl_fibmap(struct file *filp, int __user *p)
 	block = ur_block;
 	error = bmap(inode, &block);
 
+	if (block > INT_MAX) {
+		error = -ERANGE;
+		pr_warn_ratelimited("[%s/%d] FS (%s): would truncate fibmap result\n",
+				    current->comm, task_pid_nr(current),
+				    sb->s_id);
+	}
+
 	if (error)
 		ur_block = 0;
 	else
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: [PATCHv2] fibmap: Warn and return an error in case of block > INT_MAX

[Christoph Hellwig]

On Tue, Apr 28, 2020 at 01:08:31PM +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 pr_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.
> 
> Now 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 we have directly added this warning
> 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 iomap_bmap_actor().
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Well, this changed quite a bit from the previous version, so I would
have dropped the Reviewed-by tags.

That being said this version still looks good to me:

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

Re: [PATCHv2] fibmap: Warn and return an error in case of block > INT_MAX

[Matthew Wilcox]

On Tue, Apr 28, 2020 at 01:08:31PM +0530, Ritesh Harjani wrote:
> @@ -71,6 +72,13 @@ static int ioctl_fibmap(struct file *filp, int __user *p)
>  	block = ur_block;
>  	error = bmap(inode, &block);
>  
> +	if (block > INT_MAX) {
> +		error = -ERANGE;
> +		pr_warn_ratelimited("[%s/%d] FS (%s): would truncate fibmap result\n",
> +				    current->comm, task_pid_nr(current),
> +				    sb->s_id);

Why is it useful to print the pid?
And why print the superblock when we could print the filename instead?
We have a struct file, so we can printk("%pD4", filp) to print the
last four components of the pathname.

Re: [PATCHv2] fibmap: Warn and return an error in case of block > INT_MAX

[Ritesh Harjani]

On 4/28/20 5:11 PM, Matthew Wilcox wrote:
> On Tue, Apr 28, 2020 at 01:08:31PM +0530, Ritesh Harjani wrote:
>> @@ -71,6 +72,13 @@ static int ioctl_fibmap(struct file *filp, int __user *p)
>>   	block = ur_block;
>>   	error = bmap(inode, &block);
>>   
>> +	if (block > INT_MAX) {
>> +		error = -ERANGE;
>> +		pr_warn_ratelimited("[%s/%d] FS (%s): would truncate fibmap result\n",
>> +				    current->comm, task_pid_nr(current),
>> +				    sb->s_id);
> 
> Why is it useful to print the pid?
For e.g. a case where you have pthreads. pid could be different there.

> And why print the superblock when we could print the filename instead? > We have a struct file, so we can printk("%pD4", filp) to print the
> last four components of the pathname.
> 

Sure, let me use below print msg then. This should cover everything now
If no objections, I could send this in v3. Pls, do let me know if
otherwise.

Since this has changed again from the 1st version, will drop all
Reviewed-by: and you could directly review v3 then.

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

About %pD from (Documentation/core-api/printk-formats.rst)
========================================================
<..>
	%pd{,2,3,4}
	%pD{,2,3,4}

For printing dentry name; if we race with :c:func:`d_move`, the name might
be a mix of old and new ones, but it won't oops.  %pd dentry is a safer
equivalent of %s dentry->d_name.name we used to use, %pd<n> prints ``n``
last components.  %pD does the same thing for struct file.



-ritesh