* [PATCH] fibmap: Reject negative addresses @ 2019-03-18 16:10 Carlos Maiolino 2019-03-19 19:30 ` Eric Sandeen 0 siblings, 1 reply; 3+ messages in thread From: Carlos Maiolino @ 2019-03-18 16:10 UTC (permalink / raw) To: linux-fsdevel 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. Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> --- 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 */ + if (block < 0) + return -EINVAL; + res = mapping->a_ops->bmap(mapping, block); return put_user(res, p); } -- 2.20.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] fibmap: Reject negative addresses 2019-03-18 16:10 [PATCH] fibmap: Reject negative addresses Carlos Maiolino @ 2019-03-19 19:30 ` Eric Sandeen 2019-03-20 12:28 ` Carlos Maiolino 0 siblings, 1 reply; 3+ messages in thread From: Eric Sandeen @ 2019-03-19 19:30 UTC (permalink / raw) To: Carlos Maiolino, linux-fsdevel 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@redhat.com> > --- > 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); > } > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] fibmap: Reject negative addresses 2019-03-19 19:30 ` Eric Sandeen @ 2019-03-20 12:28 ` Carlos Maiolino 0 siblings, 0 replies; 3+ messages in thread From: Carlos Maiolino @ 2019-03-20 12:28 UTC (permalink / raw) To: Eric Sandeen; +Cc: linux-fsdevel 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@redhat.com> > > --- > > > 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-03-20 12:28 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-18 16:10 [PATCH] fibmap: Reject negative addresses Carlos Maiolino 2019-03-19 19:30 ` Eric Sandeen 2019-03-20 12:28 ` Carlos Maiolino
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).