linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/1] kernel/sysctl.c: avoid overflow
@ 2016-06-11  1:33 Heinrich Schuchardt
  2016-06-14 18:33 ` Kees Cook
  2016-06-14 20:19 ` Andrew Morton
  0 siblings, 2 replies; 8+ messages in thread
From: Heinrich Schuchardt @ 2016-06-11  1:33 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Arnaldo Carvalho de Melo, Kees Cook, Don Zickus, Al Viro,
	Dave Young, Hugh Dickins, Thomas Gleixner, Daniel Cashman,
	Willy Tarreau, Alexei Starovoitov, Eric W. Biederman,
	Ilya Dryomov, linux-kernel, Heinrich Schuchardt

An undetected overflow may occur in do_proc_dointvec_minmax_conv_param.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 kernel/sysctl.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 35f0dcb..a9e7be3 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2313,7 +2313,17 @@ static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
 {
 	struct do_proc_dointvec_minmax_conv_param *param = data;
 	if (write) {
-		int val = *negp ? -*lvalp : *lvalp;
+		int val;
+
+		if (*negp) {
+			if (*lvalp > (unsigned long) INT_MAX + 1)
+				return -EINVAL;
+			val = -*lvalp;
+		} else {
+			if (*lvalp > (unsigned long) INT_MAX)
+				return -EINVAL;
+			val = *lvalp;
+		}
 		if ((param->min && *param->min > val) ||
 		    (param->max && *param->max < val))
 			return -EINVAL;
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] kernel/sysctl.c: avoid overflow
  2016-06-11  1:33 [PATCH 1/1] kernel/sysctl.c: avoid overflow Heinrich Schuchardt
@ 2016-06-14 18:33 ` Kees Cook
  2016-06-14 20:19 ` Andrew Morton
  1 sibling, 0 replies; 8+ messages in thread
From: Kees Cook @ 2016-06-14 18:33 UTC (permalink / raw)
  To: Heinrich Schuchardt
  Cc: Andrew Morton, Arnaldo Carvalho de Melo, Don Zickus, Al Viro,
	Dave Young, Hugh Dickins, Thomas Gleixner, Daniel Cashman,
	Willy Tarreau, Alexei Starovoitov, Eric W. Biederman,
	Ilya Dryomov, LKML

On Fri, Jun 10, 2016 at 6:33 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> An undetected overflow may occur in do_proc_dointvec_minmax_conv_param.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

Acked-by: Kees Cook <keescook@chromium.org>

Thanks for the fix!

-Kees

> ---
>  kernel/sysctl.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 35f0dcb..a9e7be3 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -2313,7 +2313,17 @@ static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
>  {
>         struct do_proc_dointvec_minmax_conv_param *param = data;
>         if (write) {
> -               int val = *negp ? -*lvalp : *lvalp;
> +               int val;
> +
> +               if (*negp) {
> +                       if (*lvalp > (unsigned long) INT_MAX + 1)
> +                               return -EINVAL;
> +                       val = -*lvalp;
> +               } else {
> +                       if (*lvalp > (unsigned long) INT_MAX)
> +                               return -EINVAL;
> +                       val = *lvalp;
> +               }
>                 if ((param->min && *param->min > val) ||
>                     (param->max && *param->max < val))
>                         return -EINVAL;
> --
> 2.1.4
>



-- 
Kees Cook
Chrome OS & Brillo Security

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] kernel/sysctl.c: avoid overflow
  2016-06-11  1:33 [PATCH 1/1] kernel/sysctl.c: avoid overflow Heinrich Schuchardt
  2016-06-14 18:33 ` Kees Cook
@ 2016-06-14 20:19 ` Andrew Morton
  2016-06-14 20:41   ` Willy Tarreau
  2016-06-14 21:05   ` Kees Cook
  1 sibling, 2 replies; 8+ messages in thread
From: Andrew Morton @ 2016-06-14 20:19 UTC (permalink / raw)
  To: Heinrich Schuchardt
  Cc: Arnaldo Carvalho de Melo, Kees Cook, Don Zickus, Al Viro,
	Dave Young, Hugh Dickins, Thomas Gleixner, Daniel Cashman,
	Willy Tarreau, Alexei Starovoitov, Eric W. Biederman,
	Ilya Dryomov, linux-kernel

On Sat, 11 Jun 2016 03:33:08 +0200 Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:

> An undetected overflow may occur in do_proc_dointvec_minmax_conv_param.
> 
> ...
>
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -2313,7 +2313,17 @@ static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
>  {
>  	struct do_proc_dointvec_minmax_conv_param *param = data;
>  	if (write) {
> -		int val = *negp ? -*lvalp : *lvalp;
> +		int val;
> +
> +		if (*negp) {
> +			if (*lvalp > (unsigned long) INT_MAX + 1)
> +				return -EINVAL;
> +			val = -*lvalp;
> +		} else {
> +			if (*lvalp > (unsigned long) INT_MAX)
> +				return -EINVAL;
> +			val = *lvalp;
> +		}
>  		if ((param->min && *param->min > val) ||
>  		    (param->max && *param->max < val))
>  			return -EINVAL;

hm.

What happens if someone does

	echo -1 > /proc/foo

expecting to get 0xffffffff?  That's a reasonable shorthand, and if we
change that to spit out EINVAL then people's stuff may break.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] kernel/sysctl.c: avoid overflow
  2016-06-14 20:19 ` Andrew Morton
@ 2016-06-14 20:41   ` Willy Tarreau
  2016-06-15  8:33     ` Dave Young
  2016-06-14 21:05   ` Kees Cook
  1 sibling, 1 reply; 8+ messages in thread
From: Willy Tarreau @ 2016-06-14 20:41 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Heinrich Schuchardt, Arnaldo Carvalho de Melo, Kees Cook,
	Don Zickus, Al Viro, Dave Young, Hugh Dickins, Thomas Gleixner,
	Daniel Cashman, Alexei Starovoitov, Eric W. Biederman,
	Ilya Dryomov, linux-kernel

On Tue, Jun 14, 2016 at 01:19:06PM -0700, Andrew Morton wrote:
> On Sat, 11 Jun 2016 03:33:08 +0200 Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> 
> > An undetected overflow may occur in do_proc_dointvec_minmax_conv_param.
> > 
> > ...
> >
> > --- a/kernel/sysctl.c
> > +++ b/kernel/sysctl.c
> > @@ -2313,7 +2313,17 @@ static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
> >  {
> >  	struct do_proc_dointvec_minmax_conv_param *param = data;
> >  	if (write) {
> > -		int val = *negp ? -*lvalp : *lvalp;
> > +		int val;
> > +
> > +		if (*negp) {
> > +			if (*lvalp > (unsigned long) INT_MAX + 1)
> > +				return -EINVAL;
> > +			val = -*lvalp;
> > +		} else {
> > +			if (*lvalp > (unsigned long) INT_MAX)
> > +				return -EINVAL;
> > +			val = *lvalp;
> > +		}
> >  		if ((param->min && *param->min > val) ||
> >  		    (param->max && *param->max < val))
> >  			return -EINVAL;
> 
> hm.
> 
> What happens if someone does
> 
> 	echo -1 > /proc/foo
> 
> expecting to get 0xffffffff?  That's a reasonable shorthand, and if we
> change that to spit out EINVAL then people's stuff may break.

I'd go even further, I don't see anymore how it becomes possible
to actually *write* 0xffffffff at all! This function is used by
proc_dointvec_minmax() which is used with extra1=&zero and extra2
not set with some unsigned ints to allow the full range to be
configured (eg: dirty_expire_interval is the first I found by a
quick random look).

So for me this change is bogus.

Willy

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] kernel/sysctl.c: avoid overflow
  2016-06-14 20:19 ` Andrew Morton
  2016-06-14 20:41   ` Willy Tarreau
@ 2016-06-14 21:05   ` Kees Cook
  1 sibling, 0 replies; 8+ messages in thread
From: Kees Cook @ 2016-06-14 21:05 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Heinrich Schuchardt, Arnaldo Carvalho de Melo, Don Zickus,
	Al Viro, Dave Young, Hugh Dickins, Thomas Gleixner,
	Daniel Cashman, Willy Tarreau, Alexei Starovoitov,
	Eric W. Biederman, Ilya Dryomov, LKML

On Tue, Jun 14, 2016 at 1:19 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Sat, 11 Jun 2016 03:33:08 +0200 Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>> An undetected overflow may occur in do_proc_dointvec_minmax_conv_param.
>>
>> ...
>>
>> --- a/kernel/sysctl.c
>> +++ b/kernel/sysctl.c
>> @@ -2313,7 +2313,17 @@ static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
>>  {
>>       struct do_proc_dointvec_minmax_conv_param *param = data;
>>       if (write) {
>> -             int val = *negp ? -*lvalp : *lvalp;
>> +             int val;
>> +
>> +             if (*negp) {
>> +                     if (*lvalp > (unsigned long) INT_MAX + 1)
>> +                             return -EINVAL;
>> +                     val = -*lvalp;
>> +             } else {
>> +                     if (*lvalp > (unsigned long) INT_MAX)
>> +                             return -EINVAL;
>> +                     val = *lvalp;
>> +             }
>>               if ((param->min && *param->min > val) ||
>>                   (param->max && *param->max < val))
>>                       return -EINVAL;
>
> hm.
>
> What happens if someone does
>
>         echo -1 > /proc/foo
>
> expecting to get 0xffffffff?  That's a reasonable shorthand, and if we
> change that to spit out EINVAL then people's stuff may break.

If we expect the interface to allow overflows, we should at least add
comments to do_proc_dointvec_minmax_conv_param()...

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] kernel/sysctl.c: avoid overflow
  2016-06-14 20:41   ` Willy Tarreau
@ 2016-06-15  8:33     ` Dave Young
  2016-06-15  8:40       ` Willy Tarreau
  0 siblings, 1 reply; 8+ messages in thread
From: Dave Young @ 2016-06-15  8:33 UTC (permalink / raw)
  To: Willy Tarreau
  Cc: Andrew Morton, Heinrich Schuchardt, Arnaldo Carvalho de Melo,
	Kees Cook, Don Zickus, Al Viro, Hugh Dickins, Thomas Gleixner,
	Daniel Cashman, Alexei Starovoitov, Eric W. Biederman,
	Ilya Dryomov, linux-kernel

On 06/14/16 at 10:41pm, Willy Tarreau wrote:
> On Tue, Jun 14, 2016 at 01:19:06PM -0700, Andrew Morton wrote:
> > On Sat, 11 Jun 2016 03:33:08 +0200 Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > 
> > > An undetected overflow may occur in do_proc_dointvec_minmax_conv_param.
> > > 
> > > ...
> > >
> > > --- a/kernel/sysctl.c
> > > +++ b/kernel/sysctl.c
> > > @@ -2313,7 +2313,17 @@ static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
> > >  {
> > >  	struct do_proc_dointvec_minmax_conv_param *param = data;
> > >  	if (write) {
> > > -		int val = *negp ? -*lvalp : *lvalp;
> > > +		int val;
> > > +
> > > +		if (*negp) {
> > > +			if (*lvalp > (unsigned long) INT_MAX + 1)
> > > +				return -EINVAL;
> > > +			val = -*lvalp;
> > > +		} else {
> > > +			if (*lvalp > (unsigned long) INT_MAX)
> > > +				return -EINVAL;
> > > +			val = *lvalp;
> > > +		}
> > >  		if ((param->min && *param->min > val) ||
> > >  		    (param->max && *param->max < val))
> > >  			return -EINVAL;
> > 
> > hm.
> > 
> > What happens if someone does
> > 
> > 	echo -1 > /proc/foo
> > 
> > expecting to get 0xffffffff?  That's a reasonable shorthand, and if we
> > change that to spit out EINVAL then people's stuff may break.
> 
> I'd go even further, I don't see anymore how it becomes possible
> to actually *write* 0xffffffff at all! This function is used by
> proc_dointvec_minmax() which is used with extra1=&zero and extra2
> not set with some unsigned ints to allow the full range to be
> configured (eg: dirty_expire_interval is the first I found by a
> quick random look).

sysctl_writes_strict use extra1 = -1 and extra2 = 1

But I do not get why -1 does not work, 1 < (unsigned long) INT_MAX + 1
so val = -1, it is still right?

> 
> So for me this change is bogus.
> 
> Willy
> 

Thanks
Dave

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] kernel/sysctl.c: avoid overflow
  2016-06-15  8:33     ` Dave Young
@ 2016-06-15  8:40       ` Willy Tarreau
  2016-06-15  8:50         ` Dave Young
  0 siblings, 1 reply; 8+ messages in thread
From: Willy Tarreau @ 2016-06-15  8:40 UTC (permalink / raw)
  To: Dave Young
  Cc: Andrew Morton, Heinrich Schuchardt, Arnaldo Carvalho de Melo,
	Kees Cook, Don Zickus, Al Viro, Hugh Dickins, Thomas Gleixner,
	Daniel Cashman, Alexei Starovoitov, Eric W. Biederman,
	Ilya Dryomov, linux-kernel

On Wed, Jun 15, 2016 at 04:33:32PM +0800, Dave Young wrote:
> On 06/14/16 at 10:41pm, Willy Tarreau wrote:
> > On Tue, Jun 14, 2016 at 01:19:06PM -0700, Andrew Morton wrote:
> > > On Sat, 11 Jun 2016 03:33:08 +0200 Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > 
> > > > An undetected overflow may occur in do_proc_dointvec_minmax_conv_param.
> > > > 
> > > > ...
> > > >
> > > > --- a/kernel/sysctl.c
> > > > +++ b/kernel/sysctl.c
> > > > @@ -2313,7 +2313,17 @@ static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
> > > >  {
> > > >  	struct do_proc_dointvec_minmax_conv_param *param = data;
> > > >  	if (write) {
> > > > -		int val = *negp ? -*lvalp : *lvalp;
> > > > +		int val;
> > > > +
> > > > +		if (*negp) {
> > > > +			if (*lvalp > (unsigned long) INT_MAX + 1)
> > > > +				return -EINVAL;
> > > > +			val = -*lvalp;
> > > > +		} else {
> > > > +			if (*lvalp > (unsigned long) INT_MAX)
> > > > +				return -EINVAL;
> > > > +			val = *lvalp;
> > > > +		}
> > > >  		if ((param->min && *param->min > val) ||
> > > >  		    (param->max && *param->max < val))
> > > >  			return -EINVAL;
> > > 
> > > hm.
> > > 
> > > What happens if someone does
> > > 
> > > 	echo -1 > /proc/foo
> > > 
> > > expecting to get 0xffffffff?  That's a reasonable shorthand, and if we
> > > change that to spit out EINVAL then people's stuff may break.
> > 
> > I'd go even further, I don't see anymore how it becomes possible
> > to actually *write* 0xffffffff at all! This function is used by
> > proc_dointvec_minmax() which is used with extra1=&zero and extra2
> > not set with some unsigned ints to allow the full range to be
> > configured (eg: dirty_expire_interval is the first I found by a
> > quick random look).
> 
> sysctl_writes_strict use extra1 = -1 and extra2 = 1
> 
> But I do not get why -1 does not work, 1 < (unsigned long) INT_MAX + 1
> so val = -1, it is still right?

-1 should indeed work but 0xffffffff will definitely not.

Willy

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] kernel/sysctl.c: avoid overflow
  2016-06-15  8:40       ` Willy Tarreau
@ 2016-06-15  8:50         ` Dave Young
  0 siblings, 0 replies; 8+ messages in thread
From: Dave Young @ 2016-06-15  8:50 UTC (permalink / raw)
  To: Willy Tarreau
  Cc: Andrew Morton, Heinrich Schuchardt, Arnaldo Carvalho de Melo,
	Kees Cook, Don Zickus, Al Viro, Hugh Dickins, Thomas Gleixner,
	Daniel Cashman, Alexei Starovoitov, Eric W. Biederman,
	Ilya Dryomov, linux-kernel

On 06/15/16 at 10:40am, Willy Tarreau wrote:
> On Wed, Jun 15, 2016 at 04:33:32PM +0800, Dave Young wrote:
> > On 06/14/16 at 10:41pm, Willy Tarreau wrote:
> > > On Tue, Jun 14, 2016 at 01:19:06PM -0700, Andrew Morton wrote:
> > > > On Sat, 11 Jun 2016 03:33:08 +0200 Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > 
> > > > > An undetected overflow may occur in do_proc_dointvec_minmax_conv_param.
> > > > > 
> > > > > ...
> > > > >
> > > > > --- a/kernel/sysctl.c
> > > > > +++ b/kernel/sysctl.c
> > > > > @@ -2313,7 +2313,17 @@ static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
> > > > >  {
> > > > >  	struct do_proc_dointvec_minmax_conv_param *param = data;
> > > > >  	if (write) {
> > > > > -		int val = *negp ? -*lvalp : *lvalp;
> > > > > +		int val;
> > > > > +
> > > > > +		if (*negp) {
> > > > > +			if (*lvalp > (unsigned long) INT_MAX + 1)
> > > > > +				return -EINVAL;
> > > > > +			val = -*lvalp;
> > > > > +		} else {
> > > > > +			if (*lvalp > (unsigned long) INT_MAX)
> > > > > +				return -EINVAL;
> > > > > +			val = *lvalp;
> > > > > +		}
> > > > >  		if ((param->min && *param->min > val) ||
> > > > >  		    (param->max && *param->max < val))
> > > > >  			return -EINVAL;
> > > > 
> > > > hm.
> > > > 
> > > > What happens if someone does
> > > > 
> > > > 	echo -1 > /proc/foo
> > > > 
> > > > expecting to get 0xffffffff?  That's a reasonable shorthand, and if we
> > > > change that to spit out EINVAL then people's stuff may break.
> > > 
> > > I'd go even further, I don't see anymore how it becomes possible
> > > to actually *write* 0xffffffff at all! This function is used by
> > > proc_dointvec_minmax() which is used with extra1=&zero and extra2
> > > not set with some unsigned ints to allow the full range to be
> > > configured (eg: dirty_expire_interval is the first I found by a
> > > quick random look).
> > 
> > sysctl_writes_strict use extra1 = -1 and extra2 = 1
> > 
> > But I do not get why -1 does not work, 1 < (unsigned long) INT_MAX + 1
> > so val = -1, it is still right?
> 
> -1 should indeed work but 0xffffffff will definitely not.

Hmm, right, indeed.

Below echo works originally:
echo 0xffffffff > /proc/sys/kernel/sysctl_writes_strict

> 
> Willy

Thanks
Dave

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2016-06-15  8:50 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-11  1:33 [PATCH 1/1] kernel/sysctl.c: avoid overflow Heinrich Schuchardt
2016-06-14 18:33 ` Kees Cook
2016-06-14 20:19 ` Andrew Morton
2016-06-14 20:41   ` Willy Tarreau
2016-06-15  8:33     ` Dave Young
2016-06-15  8:40       ` Willy Tarreau
2016-06-15  8:50         ` Dave Young
2016-06-14 21:05   ` Kees Cook

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).