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
[e-mail]