On 3/18/19 11:10 AM, Carlos Maiolino wrote: > FIBMAP receives an integer from userspace which is then implicitly > converted into sector_t on ->bmap(). No check is made to ensure > userspace didn't send a negative address, which can confuse the ->bmap > interface, and return fuzzy addresses. Nitpick on summary & comments, I think this should be "negative logical block number?" because "address" often means something completely different. ;) what is a "fuzzy address?" What actually happens if a negative number gets through? > > Signed-off-by: Carlos Maiolino <cmaiolino@xxxxxxxxxx> > --- > I am not 100% sure if is there any reason for ->bmap interface to accept > negative values, from the top of my mind, I really can't see a reason, if there > is a reason why we don't check for negative addresses, I'd be happy to know, > otherwise we should reject it. > > Cheers > > fs/ioctl.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/fs/ioctl.c b/fs/ioctl.c > index fef3a6bf7c78..e3a01c43adb4 100644 > --- a/fs/ioctl.c > +++ b/fs/ioctl.c > @@ -64,6 +64,11 @@ static int ioctl_fibmap(struct file *filp, int __user *p) > res = get_user(block, p); > if (res) > return res; > + > + /* No reason to query a negative block addr */ /* Invalid to query a negative block number */ Actually, the next 2 lines of code are not hard to understand, so probably doesn't really even need a comment. As for the change, it seems fine, I guess we don't document what the user interface is supposed to be anywhere, sooo... go with the current signed int implementation, and disallow anything that doesn't fit, makes sense to me. -Eric > + if (block < 0) > + return -EINVAL; > + > res = mapping->a_ops->bmap(mapping, block); > return put_user(res, p); > } >
