On Tue, Mar 19, 2019 at 02:30:02PM -0500, Eric Sandeen wrote: > 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. ;) > Thanks, yeah, I meant block address, but I omitted the 'block' > what is a "fuzzy address?" What actually happens if a negative number > gets through? The type internally is a sector_t, which is a u64, so it will underflow the internal type, making the request look for block (2<<63)-1 (IIRC), which is most likely what the userspace doesn't want. Also, this affects iomap bmap interface, which has a WARN() in iomap_bmap_actor() triggered by any block address bigger than INT_MAX, which, makes sense giving the userspace limitation. I should have written a better description for the patch =/ I'll send a V2 with a better description, if people are ok with the code change, so, I'll just wait for a few more reviews before writing a V2. > > > > > 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 I much appreciate your review > > > + if (block < 0) > > + return -EINVAL; > > + > > res = mapping->a_ops->bmap(mapping, block); > > return put_user(res, p); > > } > > -- Carlos
