Message ID | 58f0c64a3f2dbd363fb93371435f6bcaeeb7abe4.1588058868.git.riteshh@linux.ibm.com |
---|---|
State | New |
Headers | show |
Series | [PATCHv2] fibmap: Warn and return an error in case of block > INT_MAX | expand |
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>
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.
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
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; }