| Message ID | e34d1ac05d29aeeb982713a807345a0aaafc7fe0.1587670914.git.riteshh@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | WARN fibmap user in case of possible addr truncation | expand |
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 >
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>
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
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 >
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()?
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
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
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.
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); +
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
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(+)