Minor net/core/sock.c security issue?

Minor net/core/sock.c security issue?

[Chris Evans]

Hi,

May be nothing, but it looks like SO_*BUF may have signedness issues (have
these been picked up by the Stanford tools and fixed in recent 2.4.x?)

    int val;
...
    case SO_SNDBUF:
      if (val > sysctl_wmem_max)
        val = sysctl_wmem_max;
      sk->sndbuf = max(val*2,2048);

If val is negative, then sk->sndbuf ends up negative. This is because the
arguments to max are passed as _unsigned_ ints. SO_RCVBUF has similar
issues. Maybe a nasty local user could use this to chew up memory?

Cheers
Chris

Re: Minor net/core/sock.c security issue?

[David S. Miller]

Chris Evans writes:
 >     int val;
 > ...
 >     case SO_SNDBUF:
 >       if (val > sysctl_wmem_max)
 >         val = sysctl_wmem_max;
 >       sk->sndbuf = max(val*2,2048);
 > 
 > If val is negative, then sk->sndbuf ends up negative. This is because the
 > arguments to max are passed as _unsigned_ ints. SO_RCVBUF has similar
 > issues. Maybe a nasty local user could use this to chew up memory?

Indeed, you have only hit the tip of the iceberg on the larger
problems lurking in this area.

In short, min/max usage is pretty broken.  And it is broken for
several reasons:

1) Signedness, what you have discovered.

2) Arg evaluation.

3) Multiple definitions

#3 is what really makes this look gross.  Watch this:

include/net/sock.h declares two inline functions, min and
max

net/core/sock.c defines "min" as a macro, overriding the
function in sock.h

egrep "define max" include/linux/*.h shows at least three
other headers which want to define their own max macro.

There is even commentary about this in include/linux/netfilter.h along
with Rusty's attempt to make reasonable macros.  I personally disagree
with keeping them as macros because of the arg multiple evaluation
issues.

I think the way to fix this is to either:

1) have standard inline functions with names that suggest the
   signedness, much like Rusty's netfilter macros.

2) Just open code all instances of min/max, there will be no
   mistaking what the code does in such a case.

In both cases, min/max simply die and nobody can therefore misuse them
anymore.

Later,
David S. Miller
davem@redhat.com

Re: Minor net/core/sock.c security issue?

[Albert D. Cahalan]

David S. Miller writes:

> In short, min/max usage is pretty broken.  And it is broken for
> several reasons:
>
> 1) Signedness, what you have discovered.
>
> 2) Arg evaluation.
>
> 3) Multiple definitions
...
> There is even commentary about this in include/linux/netfilter.h along
> with Rusty's attempt to make reasonable macros.  I personally disagree
> with keeping them as macros because of the arg multiple evaluation
> issues.
>
> I think the way to fix this is to either:
> 
> 1) have standard inline functions with names that suggest the
>    signedness, much like Rusty's netfilter macros.
>
> 2) Just open code all instances of min/max, there will be no
>    mistaking what the code does in such a case.
>
> In both cases, min/max simply die and nobody can therefore misuse them
> anymore.

The macros won't die. You could make global ones like this:

#define min(a,b) Never do this due to signed/unsigned issues.
#define max(a,b) Never do this due to signed/unsigned issues.
#define MIN(a,b) Never do this due to signed/unsigned issues.
#define MAX(a,b) Never do this due to signed/unsigned issues.

Long term, __builtin_min() and __builtin_max() would be nice.
I'm guessing the g++ <? and >? operators don't handle signs right,
but in case they do then that is an even better option.

Re: Minor net/core/sock.c security issue?

[David S. Miller]

Albert D. Cahalan writes:
 > Long term, __builtin_min() and __builtin_max() would be nice.

I would even avoid this, what is the signedness of their
arguments and return values?  The answer is: I don't care,
because I have to look it up.

And if I have to look it up, I know that most people _won't_ look it
up and will just guess or assume.  Most people are therefore likely to
misuse it.

Later,
David S. Miller
davem@redhat.com

Re: Minor net/core/sock.c security issue?

[Albert D. Cahalan]

David S. Miller writes:
> Albert D. Cahalan writes:

>> Long term, __builtin_min() and __builtin_max() would be nice.
>
> I would even avoid this, what is the signedness of their
> arguments and return values?  The answer is: I don't care,
> because I have to look it up.
> 
> And if I have to look it up, I know that most people _won't_ look it
> up and will just guess or assume.  Most people are therefore likely to
> misuse it.

The obvious answer is to enforce that the signedness of their
arguments and return values all match. Anything else won't compile.
This is safer than plain open code, because it forces the programmer
to fix any signedness mismatch.

The whole point of being built-in is that stuff like this can be
handled.

Possibly bad ideas:

The full range of signed/unsigned could be made to work, as if you
were using 33-bit or 65-bit signed math. It might even be sane to take
the return type from whatever is getting fed the return value. It
would be cool to use the exception tables if something goes out of
range, though maybe that would be too slow.

Re: Minor net/core/sock.c security issue?

[Alexey Kuznetsov]

Hello!

> 1) Signedness, what you have discovered.
> 
> 2) Arg evaluation.

Damn, I always assumed that min/max are macros and took into account #2,
which was painful sometimes. :-)

The fact that it is defined in sock.h (and the definition is truly
crazy, add #4: it is funny what happens on 64bit archs, when one of args
happens to be long) 


> 1) have standard inline functions with names that suggest the
>    signedness, much like Rusty's netfilter macros.

min/max are macros. I do not know how to make a valid inline
for it: cast to long has problems with unsigned longs, cast to unsigned long
have the same problems with signedness.


Alexey

Re: Minor net/core/sock.c security issue?

[David S. Miller]

Alexey Kuznetsov writes:
 > > 1) have standard inline functions with names that suggest the
 > >    signedness, much like Rusty's netfilter macros.
 > 
 > min/max are macros. I do not know how to make a valid inline
 > for it: cast to long has problems with unsigned longs, cast to unsigned long
 > have the same problems with signedness.

For the time being I've just killed that bogus min define
from sock.c and also open-coded the min/max usage in the
rest of sock.c

This solves the original report, but later I'd like to do
something more satisfactory here.

I mean, grep for "define [min|max]" in just the networking
sources right now, yuck!

Later,
David S. Miller
davem@redhat.com

Re: Minor net/core/sock.c security issue?

[kuznet]

Hello!

> For the time being I've just killed that bogus min define

It is not bogus. Bogus one is in sock.h.

Hundred times I discovered that min/max are not defined in some place,
but was lazy to search for header where they are defined. :-)


> I mean, grep for "define [min|max]" in just the networking
> sources right now, yuck!

grep better for min/max. Everywhere min/max are assumed to be shortcut
for (a<b?a:b).

And if you find a place, which relied on that silly inline in sock.h,
I will wonder at first and die of shame the next minute. :-)

Alexey