[PATCH] ipv4: avoid undefined behavior in do_ip_setsockopt()

(1<<optname) is undefined behavior in C with a negative optname or
optname larger than 31.  In those cases the result of the shift is
not necessarily zero (e.g., on x86).

This patch simplifies the code with a switch statement on optname.
It also allows the compiler to generate better code (e.g., using a
64-bit mask).

Signed-off-by: Xi Wang <xi.wang@gmail.com>
---
 net/ipv4/ip_sockglue.c |   35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
index 5eea4a8..14bbfcf 100644
--- a/net/ipv4/ip_sockglue.c
+++ b/net/ipv4/ip_sockglue.c

@@ -457,19 +457,28 @@ static int do_ip_setsockopt(struct sock *sk, int level,
 	struct inet_sock *inet = inet_sk(sk);
 	int val = 0, err;
 
-	if (((1<<optname) & ((1<<IP_PKTINFO) | (1<<IP_RECVTTL) |
-			     (1<<IP_RECVOPTS) | (1<<IP_RECVTOS) |
-			     (1<<IP_RETOPTS) | (1<<IP_TOS) |
-			     (1<<IP_TTL) | (1<<IP_HDRINCL) |
-			     (1<<IP_MTU_DISCOVER) | (1<<IP_RECVERR) |
-			     (1<<IP_ROUTER_ALERT) | (1<<IP_FREEBIND) |
-			     (1<<IP_PASSSEC) | (1<<IP_TRANSPARENT) |
-			     (1<<IP_MINTTL) | (1<<IP_NODEFRAG))) ||
-	    optname == IP_UNICAST_IF ||
-	    optname == IP_MULTICAST_TTL ||
-	    optname == IP_MULTICAST_ALL ||
-	    optname == IP_MULTICAST_LOOP ||
-	    optname == IP_RECVORIGDSTADDR) {
+	switch (optname) {
+	case IP_PKTINFO:
+	case IP_RECVTTL:
+	case IP_RECVOPTS:
+	case IP_RECVTOS:
+	case IP_RETOPTS:
+	case IP_TOS:
+	case IP_TTL:
+	case IP_HDRINCL:
+	case IP_MTU_DISCOVER:
+	case IP_RECVERR:
+	case IP_ROUTER_ALERT:
+	case IP_FREEBIND:
+	case IP_PASSSEC:
+	case IP_TRANSPARENT:
+	case IP_MINTTL:
+	case IP_NODEFRAG:
+	case IP_UNICAST_IF:
+	case IP_MULTICAST_TTL:
+	case IP_MULTICAST_ALL:
+	case IP_MULTICAST_LOOP:
+	case IP_RECVORIGDSTADDR:
 		if (optlen >= sizeof(int)) {
 			if (get_user(val, (int __user *) optval))
 				return -EFAULT;

-- 
1.7.10.4

This code is a fast bit test on purpose.  You're making the
code slower.

I'm not applying this patch.

On 11/11/12 5:02 PM, David Miller wrote:

This code is a fast bit test on purpose.  You're making the
code slower.

No, it's the opposite.  All modern compilers optimize switch cases
into a fast bit test.  The original "smarter" code, however, hinders
the compiler determining the mask for a fast bit test.  With this
patch, GCC is able to compute a better mask.  You can check the
assembly.

- xi

From: Xi Wang <xi.wang@gmail.com>
Date: Sun, 11 Nov 2012 17:18:55 -0500

On 11/11/12 5:02 PM, David Miller wrote:

quoted

This code is a fast bit test on purpose.  You're making the
code slower.

No, it's the opposite.  All modern compilers optimize switch cases
into a fast bit test.

Indeed, I even checked sparc64 with gcc-4.6 and it looks good.

Thanks for the clarification, I'll apply this, thanks.

quoted

quoted

This code is a fast bit test on purpose.  You're making the
code slower.

No, it's the opposite.  All modern compilers optimize switch cases
into a fast bit test.

'All modern' is probably an overstatement, 'recent gcc' might be valid.
 

Indeed, I even checked sparc64 with gcc-4.6 and it looks good.

Thanks for the clarification, I'll apply this, thanks.

The 'switch' version will have an extra conditional to detect
'out of range' values - even though we know they can't happen.
I'm not sure you can avoid that - even for an enum.

	David

On 11/12/12 8:54 AM, David Laight wrote:

'All modern' is probably an overstatement, 'recent gcc' might be valid.

I agree if you consider gcc 3.4 released 8 years ago as "recent gcc",
or if you use a compiler other than gcc/clang/icc to compile the kernel.

The 'switch' version will have an extra conditional to detect
'out of range' values - even though we know they can't happen.
I'm not sure you can avoid that - even for an enum.

This out-of-range check is exactly what this patch wanted to add:
optname is a syscall parameter, and we should reject invalid optname
values before doing (1<<optname).

- xi