[PATCH] tun: make sure interface usage can not overflow

[PATCH] tun: make sure interface usage can not overflow

[Kees Cook]

This makes the size argument a const, since it is always populated by
the caller. Additionally double-checks to make sure the copy_from_user
can never overflow, keeping CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy:

   In function 'copy_from_user',
       inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1871:7:
       ... copy_from_user() buffer size is not provably correct

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/net/tun.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index acaaf6784179..a1f317cba206 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1855,7 +1855,7 @@ unlock:
 }
 
 static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
-			    unsigned long arg, int ifreq_len)
+			    unsigned long arg, const size_t ifreq_len)
 {
 	struct tun_file *tfile = file->private_data;
 	struct tun_struct *tun;
@@ -1869,6 +1869,7 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
 	int ret;
 
 	if (cmd == TUNSETIFF || cmd == TUNSETQUEUE || _IOC_TYPE(cmd) == 0x89) {
+		BUG_ON(ifreq_len > sizeof(ifr));
 		if (copy_from_user(&ifr, argp, ifreq_len))
 			return -EFAULT;
 	} else {
-- 
1.9.1


-- 
Kees Cook
Chrome OS Security

RE: [PATCH] tun: make sure interface usage can not overflow

[David Laight]

From: Kees Cook
> This makes the size argument a const, since it is always populated by
> the caller.

There is almost no point making parameters 'const.
('const foo *' makes sense).

> Additionally double-checks to make sure the copy_from_user
> can never overflow, keeping CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy:
> 
>    In function 'copy_from_user',
>        inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1871:7:
>        ... copy_from_user() buffer size is not provably correct

If 'ifreq_len' could be too big then you want to error the ioctl, not panic.
If it can't be too big you don't need the check.

	David

> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  drivers/net/tun.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index acaaf6784179..a1f317cba206 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1855,7 +1855,7 @@ unlock:
>  }
> 
>  static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
> -			    unsigned long arg, int ifreq_len)
> +			    unsigned long arg, const size_t ifreq_len)
>  {
>  	struct tun_file *tfile = file->private_data;
>  	struct tun_struct *tun;
> @@ -1869,6 +1869,7 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
>  	int ret;
> 
>  	if (cmd == TUNSETIFF || cmd == TUNSETQUEUE || _IOC_TYPE(cmd) == 0x89) {
> +		BUG_ON(ifreq_len > sizeof(ifr));
>  		if (copy_from_user(&ifr, argp, ifreq_len))
>  			return -EFAULT;
>  	} else {
> --
> 1.9.1
> 
> 
> --
> Kees Cook
> Chrome OS Security
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tun: make sure interface usage can not overflow

[Kees Cook]

On Mon, Sep 29, 2014 at 4:04 AM, David Laight <David.Laight@aculab.com> wrote:
> From: Kees Cook
>> This makes the size argument a const, since it is always populated by
>> the caller.
>
> There is almost no point making parameters 'const.
> ('const foo *' makes sense).
>
>> Additionally double-checks to make sure the copy_from_user
>> can never overflow, keeping CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy:
>>
>>    In function 'copy_from_user',
>>        inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1871:7:
>>        ... copy_from_user() buffer size is not provably correct
>
> If 'ifreq_len' could be too big then you want to error the ioctl, not panic.
> If it can't be too big you don't need the check.

The ifreq_len comes from the callers, and is the output of "sizeof"
which is const. Changing the function parameter to "const" means any
changes made in the future where the incoming value isn't const, the
compiler will throw a warning.

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH] tun: make sure interface usage can not overflow

[Hannes Frederic Sowa]

On Mo, 2014-09-29 at 12:41 -0700, Kees Cook wrote:
> On Mon, Sep 29, 2014 at 4:04 AM, David Laight <David.Laight@aculab.com> wrote:
> > From: Kees Cook
> >> This makes the size argument a const, since it is always populated by
> >> the caller.
> >
> > There is almost no point making parameters 'const.
> > ('const foo *' makes sense).
> >
> >> Additionally double-checks to make sure the copy_from_user
> >> can never overflow, keeping CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy:
> >>
> >>    In function 'copy_from_user',
> >>        inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1871:7:
> >>        ... copy_from_user() buffer size is not provably correct
> >
> > If 'ifreq_len' could be too big then you want to error the ioctl, not panic.
> > If it can't be too big you don't need the check.
> 
> The ifreq_len comes from the callers, and is the output of "sizeof"
> which is const. Changing the function parameter to "const" means any
> changes made in the future where the incoming value isn't const, the
> compiler will throw a warning.

Hmmm, I think you want something like BUILD_BUG_ON(!
__builtin_constant_p(var)). const in function argument only ensures that
the value cannot be modified in the function.

Bye,
Hannes

RE: [PATCH] tun: make sure interface usage can not overflow

[David Laight]

From: Hannes Frederic
> On Mo, 2014-09-29 at 12:41 -0700, Kees Cook wrote:
> > On Mon, Sep 29, 2014 at 4:04 AM, David Laight <David.Laight@aculab.com> wrote:
> > > From: Kees Cook
> > >> This makes the size argument a const, since it is always populated by
> > >> the caller.
> > >
> > > There is almost no point making parameters 'const.
> > > ('const foo *' makes sense).
> > >
> > >> Additionally double-checks to make sure the copy_from_user
> > >> can never overflow, keeping CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy:
> > >>
> > >>    In function 'copy_from_user',
> > >>        inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1871:7:
> > >>        ... copy_from_user() buffer size is not provably correct
> > >
> > > If 'ifreq_len' could be too big then you want to error the ioctl, not panic.
> > > If it can't be too big you don't need the check.
> >
> > The ifreq_len comes from the callers, and is the output of "sizeof"
> > which is const. Changing the function parameter to "const" means any
> > changes made in the future where the incoming value isn't const, the
> > compiler will throw a warning.
> 
> Hmmm, I think you want something like BUILD_BUG_ON(!
> __builtin_constant_p(var)). const in function argument only ensures that
> the value cannot be modified in the function.

You'd have to do something in the header file - nothing in the function
body can do that check.

I've not got the source handy, but in this case is there any need to
actually pass the size at all?
Is it always the same fixed constant??

	David

Re: [PATCH] tun: make sure interface usage can not overflow

[Hannes Frederic Sowa]

On Di, 2014-09-30 at 08:20 +0000, David Laight wrote:
> From: Hannes Frederic
> > On Mo, 2014-09-29 at 12:41 -0700, Kees Cook wrote:
> > > On Mon, Sep 29, 2014 at 4:04 AM, David Laight <David.Laight@aculab.com> wrote:
> > > > From: Kees Cook
> > > >> This makes the size argument a const, since it is always populated by
> > > >> the caller.
> > > >
> > > > There is almost no point making parameters 'const.
> > > > ('const foo *' makes sense).
> > > >
> > > >> Additionally double-checks to make sure the copy_from_user
> > > >> can never overflow, keeping CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy:
> > > >>
> > > >>    In function 'copy_from_user',
> > > >>        inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1871:7:
> > > >>        ... copy_from_user() buffer size is not provably correct
> > > >
> > > > If 'ifreq_len' could be too big then you want to error the ioctl, not panic.
> > > > If it can't be too big you don't need the check.
> > >
> > > The ifreq_len comes from the callers, and is the output of "sizeof"
> > > which is const. Changing the function parameter to "const" means any
> > > changes made in the future where the incoming value isn't const, the
> > > compiler will throw a warning.
> > 
> > Hmmm, I think you want something like BUILD_BUG_ON(!
> > __builtin_constant_p(var)). const in function argument only ensures that
> > the value cannot be modified in the function.
> 
> You'd have to do something in the header file - nothing in the function
> body can do that check.

Sure, it should work. You only need to make sure that gcc inlines the
function, so the value is constant (it is not enough that gcc knows the
value range, one specific constant is needed). So the simplest fix for
this is to specify __tun_chr_ioctl as __always_inline. ;)

Bye,
Hannes

RE: [PATCH] tun: make sure interface usage can not overflow

[David Laight]

From: Hannes Frederic 
> On Di, 2014-09-30 at 08:20 +0000, David Laight wrote:
> > From: Hannes Frederic
> > > On Mo, 2014-09-29 at 12:41 -0700, Kees Cook wrote:
> > > > On Mon, Sep 29, 2014 at 4:04 AM, David Laight <David.Laight@aculab.com> wrote:
> > > > > From: Kees Cook
> > > > >> This makes the size argument a const, since it is always populated by
> > > > >> the caller.
> > > > >
> > > > > There is almost no point making parameters 'const.
> > > > > ('const foo *' makes sense).
> > > > >
> > > > >> Additionally double-checks to make sure the copy_from_user
> > > > >> can never overflow, keeping CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy:
> > > > >>
> > > > >>    In function 'copy_from_user',
> > > > >>        inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1871:7:
> > > > >>        ... copy_from_user() buffer size is not provably correct
> > > > >
> > > > > If 'ifreq_len' could be too big then you want to error the ioctl, not panic.
> > > > > If it can't be too big you don't need the check.
> > > >
> > > > The ifreq_len comes from the callers, and is the output of "sizeof"
> > > > which is const. Changing the function parameter to "const" means any
> > > > changes made in the future where the incoming value isn't const, the
> > > > compiler will throw a warning.
> > >
> > > Hmmm, I think you want something like BUILD_BUG_ON(!
> > > __builtin_constant_p(var)). const in function argument only ensures that
> > > the value cannot be modified in the function.
> >
> > You'd have to do something in the header file - nothing in the function
> > body can do that check.
> 
> Sure, it should work. You only need to make sure that gcc inlines the
> function, so the value is constant (it is not enough that gcc knows the
> value range, one specific constant is needed). So the simplest fix for
> this is to specify __tun_chr_ioctl as __always_inline. ;)

You are joking aren't you???

Look at the code.

I'd suggest fixing whatever implements CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
to be less picky.

	David

Re: [PATCH] tun: make sure interface usage can not overflow

[Hannes Frederic Sowa]

On Di, 2014-09-30 at 11:18 +0000, David Laight wrote:
> From: Hannes Frederic 
> > On Di, 2014-09-30 at 08:20 +0000, David Laight wrote:
> > > From: Hannes Frederic
> > > > On Mo, 2014-09-29 at 12:41 -0700, Kees Cook wrote:
> > > > > On Mon, Sep 29, 2014 at 4:04 AM, David Laight <David.Laight@aculab.com> wrote:
> > > > > > From: Kees Cook
> > > > > >> This makes the size argument a const, since it is always populated by
> > > > > >> the caller.
> > > > > >
> > > > > > There is almost no point making parameters 'const.
> > > > > > ('const foo *' makes sense).
> > > > > >
> > > > > >> Additionally double-checks to make sure the copy_from_user
> > > > > >> can never overflow, keeping CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy:
> > > > > >>
> > > > > >>    In function 'copy_from_user',
> > > > > >>        inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1871:7:
> > > > > >>        ... copy_from_user() buffer size is not provably correct
> > > > > >
> > > > > > If 'ifreq_len' could be too big then you want to error the ioctl, not panic.
> > > > > > If it can't be too big you don't need the check.
> > > > >
> > > > > The ifreq_len comes from the callers, and is the output of "sizeof"
> > > > > which is const. Changing the function parameter to "const" means any
> > > > > changes made in the future where the incoming value isn't const, the
> > > > > compiler will throw a warning.
> > > >
> > > > Hmmm, I think you want something like BUILD_BUG_ON(!
> > > > __builtin_constant_p(var)). const in function argument only ensures that
> > > > the value cannot be modified in the function.
> > >
> > > You'd have to do something in the header file - nothing in the function
> > > body can do that check.
> > 
> > Sure, it should work. You only need to make sure that gcc inlines the
> > function, so the value is constant (it is not enough that gcc knows the
> > value range, one specific constant is needed). So the simplest fix for
> > this is to specify __tun_chr_ioctl as __always_inline. ;)
> 
> You are joking aren't you???

It would propagate the value from the compat and non-compat helper
functions to __tun_chr_ioctl and the __builtin_constant_p checks would
be true in the inlined copy_from/to_user, so no, I was not joking, but
because of my smiley I didn't considered this fix seriously.

> Look at the code.

I did.

> I'd suggest fixing whatever implements CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
> to be less picky.

This might involve gcc changes. E.g. on my gcc I don't get any errors.
But by looking at the code it might be plausible wrong estimations are
being made.

But somehow I really cannot follow the original bug report. Looks like
Kees' __builtin_object_size is broken?

I also checked my tun.i file, I really use gcc __builtin_object_size.

Strange...

Bye,
Hannes

Re: [PATCH] tun: make sure interface usage can not overflow

[Michael S. Tsirkin]

On Sun, Sep 28, 2014 at 04:27:53PM -0700, Kees Cook wrote:
> This makes the size argument a const, since it is always populated by
> the caller. Additionally double-checks to make sure the copy_from_user
> can never overflow, keeping CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy:
> 
>    In function 'copy_from_user',
>        inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1871:7:
>        ... copy_from_user() buffer size is not provably correct
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>

What exactly is the issue here?
__tun_chr_ioctl is called with sizeof(struct compat_ifreq)
or  sizeof (struct ifreq) as the last argument.

So this looks like a false positive, but
CONFIG_DEBUG_STRICT_USER_COPY_CHECKS machinery is supposed
to avoid false positives.

On which architecture is this?


> ---
>  drivers/net/tun.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index acaaf6784179..a1f317cba206 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1855,7 +1855,7 @@ unlock:
>  }
>  
>  static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
> -			    unsigned long arg, int ifreq_len)
> +			    unsigned long arg, const size_t ifreq_len)
>  {
>  	struct tun_file *tfile = file->private_data;
>  	struct tun_struct *tun;
> @@ -1869,6 +1869,7 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
>  	int ret;
>  
>  	if (cmd == TUNSETIFF || cmd == TUNSETQUEUE || _IOC_TYPE(cmd) == 0x89) {
> +		BUG_ON(ifreq_len > sizeof(ifr));
>  		if (copy_from_user(&ifr, argp, ifreq_len))
>  			return -EFAULT;
>  	} else {
> -- 
> 1.9.1
> 
> 
> -- 
> Kees Cook
> Chrome OS Security

Re: [PATCH] tun: make sure interface usage can not overflow

[Michael S. Tsirkin]

On Mon, Sep 29, 2014 at 02:48:49PM +0300, Michael S. Tsirkin wrote:
> On Sun, Sep 28, 2014 at 04:27:53PM -0700, Kees Cook wrote:
> > This makes the size argument a const, since it is always populated by
> > the caller. Additionally double-checks to make sure the copy_from_user
> > can never overflow, keeping CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy:
> > 
> >    In function 'copy_from_user',
> >        inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1871:7:
> >        ... copy_from_user() buffer size is not provably correct
> > 
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> 
> What exactly is the issue here?
> __tun_chr_ioctl is called with sizeof(struct compat_ifreq)
> or  sizeof (struct ifreq) as the last argument.
> 
> So this looks like a false positive, but
> CONFIG_DEBUG_STRICT_USER_COPY_CHECKS machinery is supposed
> to avoid false positives.
> 
> On which architecture is this?


Also - which kernel?
Does your kernel include: commit 3df7b41aa5e7797f391d0a41f8b0dce1fe366a09
    x86: Unify copy_from_user() size checking
?


 
> > ---
> >  drivers/net/tun.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > index acaaf6784179..a1f317cba206 100644
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> > @@ -1855,7 +1855,7 @@ unlock:
> >  }
> >  
> >  static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
> > -			    unsigned long arg, int ifreq_len)
> > +			    unsigned long arg, const size_t ifreq_len)
> >  {
> >  	struct tun_file *tfile = file->private_data;
> >  	struct tun_struct *tun;
> > @@ -1869,6 +1869,7 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
> >  	int ret;
> >  
> >  	if (cmd == TUNSETIFF || cmd == TUNSETQUEUE || _IOC_TYPE(cmd) == 0x89) {
> > +		BUG_ON(ifreq_len > sizeof(ifr));
> >  		if (copy_from_user(&ifr, argp, ifreq_len))
> >  			return -EFAULT;
> >  	} else {
> > -- 
> > 1.9.1
> > 
> > 
> > -- 
> > Kees Cook
> > Chrome OS Security

Re: [PATCH] tun: make sure interface usage can not overflow

[Kees Cook]

On Mon, Sep 29, 2014 at 4:48 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sun, Sep 28, 2014 at 04:27:53PM -0700, Kees Cook wrote:
>> This makes the size argument a const, since it is always populated by
>> the caller. Additionally double-checks to make sure the copy_from_user
>> can never overflow, keeping CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy:
>>
>>    In function 'copy_from_user',
>>        inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1871:7:
>>        ... copy_from_user() buffer size is not provably correct
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>
> What exactly is the issue here?
> __tun_chr_ioctl is called with sizeof(struct compat_ifreq)
> or  sizeof (struct ifreq) as the last argument.

Correct. There is no vulnerability here; I am attempting to both make
the code more defensive to future changes, and to keep
CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy.

> So this looks like a false positive, but
> CONFIG_DEBUG_STRICT_USER_COPY_CHECKS machinery is supposed
> to avoid false positives.

The support in GCC is currently a bit faulty, and it seems that it
didn't notice the two callers were static values, so instead, adding
an explicit test keeps it happy.

> On which architecture is this?

This is on x86, but with CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
correctly enabled (gcc after 4.6 broke its ability to correctly
optimize), which I've been playing with trying to get gcc working
again. I sent the patch because it seems like it's a reasonable
defensive change to make.

If you want to look more deeply, there's some background here:
https://code.google.com/p/chromium/issues/detail?id=371036

https://git.kernel.org/cgit/linux/kernel/git/kees/linux.git/commit/?h=gcc-bug&id=92dd7154932d8775a05dfd3de5564124c05a4150

Thanks,

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH] tun: make sure interface usage can not overflow

[Michael S. Tsirkin]

On Mon, Sep 29, 2014 at 12:48:47PM -0700, Kees Cook wrote:
> On Mon, Sep 29, 2014 at 4:48 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Sun, Sep 28, 2014 at 04:27:53PM -0700, Kees Cook wrote:
> >> This makes the size argument a const, since it is always populated by
> >> the caller. Additionally double-checks to make sure the copy_from_user
> >> can never overflow, keeping CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy:
> >>
> >>    In function 'copy_from_user',
> >>        inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1871:7:
> >>        ... copy_from_user() buffer size is not provably correct
> >>
> >> Signed-off-by: Kees Cook <keescook@chromium.org>
> >
> > What exactly is the issue here?
> > __tun_chr_ioctl is called with sizeof(struct compat_ifreq)
> > or  sizeof (struct ifreq) as the last argument.
> 
> Correct. There is no vulnerability here; I am attempting to both make
> the code more defensive to future changes, and to keep
> CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy.
> 
> > So this looks like a false positive, but
> > CONFIG_DEBUG_STRICT_USER_COPY_CHECKS machinery is supposed
> > to avoid false positives.
> 
> The support in GCC is currently a bit faulty, and it seems that it
> didn't notice the two callers were static values, so instead, adding
> an explicit test keeps it happy.
> 
> > On which architecture is this?
> 
> This is on x86, but with CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
> correctly enabled (gcc after 4.6 broke its ability to correctly
> optimize), which I've been playing with trying to get gcc working
> again. I sent the patch because it seems like it's a reasonable
> defensive change to make.

For the BUG_ON - I guess it's reasonable but we'll just break it again
by random code changes in the future. And when we do, I don't cherish
the thought of using trial and error to find a way to shut up the
warning again.
If gcc can't be fixed, something explicit like the uninitialized_var
is needed.

For const as an argument - is it also about gcc bugs?
We don't usually do this in kernel so if it's useful,
it has to be documented.

Finally, why are you switching the type from int to size_t?
Pls document the reason in the commit log.

> If you want to look more deeply, there's some background here:
> https://code.google.com/p/chromium/issues/detail?id=371036
> 
> https://git.kernel.org/cgit/linux/kernel/git/kees/linux.git/commit/?h=gcc-bug&id=92dd7154932d8775a05dfd3de5564124c05a4150
> 
> Thanks,
> 
> -Kees

I think what you are trying to do is really useful, but
I doubt making random code changes that happen to make a
specific gcc version happy is maintainable.

HTH
Thanks!

> -- 
> Kees Cook
> Chrome OS Security