[PATCH] fibmap: Reject negative addresses

[PATCH] fibmap: Reject negative addresses

[Carlos Maiolino]

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

Re: [PATCH] fibmap: Reject negative addresses

[Eric Sandeen]

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);
>  }
> 

Re: [PATCH] fibmap: Reject negative addresses

[Carlos Maiolino]

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