Compat32 setsockopt overzealous conversions

PATCH Applied David Woodhouse 2 participants 4 messages Sep 7, 2004 – Sep 10, 2004

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

David Woodhouse

author

2004-09-07 13:23 UTC #0 lore raw

PATCH 1/1 Compat32 setsockopt overzealous conversions

compat_sys_setsockopt() is a little overzealous about converting 32-bit stuff into 64-bit. It should match on level _and_ optname, not just optname. Currently it eats the IPV6_V6ONLY sockopt because its value
(26) happens to match SO_ATTACH_FILTER.

This makes it at least check 'level' for everything but IPT_SO_SET_REPLACE == IPT6_SO_SET_REPLACE, because that does seem to be the same in different levels. But do_netfilter_replace() is another can of worms entirely -- it doesn't actually work either, because some netfilter modules (like ipt_limit) include kernel-only bits which change size in the structure they share with userspace.

compat.c renamed from compat.c~ +4 -2

@@ -455,13 +455,15 @@
 asmlinkage long compat_sys_setsockopt(int fd, int level, int optname,
 				char __user *optval, int optlen)
 {
+	/* SO_SET_REPLACE seems to be the same in all levels */
 	if (optname == IPT_SO_SET_REPLACE)
 		return do_netfilter_replace(fd, level, optname,
 					    optval, optlen);
-	if (optname == SO_ATTACH_FILTER)
+	if (level == SOL_SOCKET && optname == SO_ATTACH_FILTER)
 		return do_set_attach_filter(fd, level, optname,
 					    optval, optlen);
-	if (optname == SO_RCVTIMEO || optname == SO_SNDTIMEO)
+	if (level == SOL_SOCKET &&
+	    (optname == SO_RCVTIMEO || optname == SO_SNDTIMEO))
 		return do_set_sock_timeout(fd, level, optname, optval, optlen);
 
 	return sys_setsockopt(fd, level, optname, optval, optlen);

Signature · 1 line

dwmw2

David S. Miller

maintainer Applied

2004-09-07 21:06 UTC #1 lore raw

Quoted 11 lines · On Tue, 07 Sep 2004 14:23:00 +0100 David Woodhouse <[email protected]> wrote:

compat_sys_setsockopt() is a little overzealous about converting 32-bit stuff into 64-bit. It should match on level _and_ optname, not just optname. Currently it eats the IPV6_V6ONLY sockopt because its value
(26) happens to match SO_ATTACH_FILTER.
This makes it at least check 'level' for everything but IPT_SO_SET_REPLACE == IPT6_SO_SET_REPLACE, because that does seem to be the same in different levels. But do_netfilter_replace() is another can of worms entirely -- it doesn't actually work either, because some netfilter modules (like ipt_limit) include kernel-only bits which change size in the structure they share with userspace.

Thanks, applied.

I know it's a pain in the ass, but could you cook up a 2.4.x version? Thanks.

David Woodhouse

author

2004-09-09 07:53 UTC #2 lore raw

Quoted 2 lines · David S. Miller

I know it's a pain in the ass, but could you cook up a 2.4.x version? Thanks.

Untested -- I feel dirty enough just for _looking_ at such ancient code, let alone compiling it for all the various platforms which each used to have their own version of the 32/64 compatibility stuff in those days.

I note mips64 and x86_64 have a 'do_set_icmpv6_filter' which the 2.6 compat_sys_setsockopt() lacks.

Back in the real world of 2.6, we REALLY do need to stop trying to do all this in a sockopt syscall wrapper, and instead pass down a 32/64 bit flag to the code which actually handles the sockopt -- although the optlen ought to be sufficient for the _majority_ of cases. Or maybe we should handle it like we do ioctls?

===== arch/mips64/kernel/linux32.c 1.12 vs edited =====

arch/mips64/kernel/linux32.c +1 -1

@@ -1568,7 +1568,7 @@
 asmlinkage int sys32_setsockopt(int fd, int level, int optname,
 				char *optval, int optlen)
 {
-	if (optname == SO_ATTACH_FILTER)
+	if (level == SOL_SOCKET && optname == SO_ATTACH_FILTER)
 		return do_set_attach_filter(fd, level, optname,
 					    optval, optlen);
 	if (level == SOL_ICMPV6 && optname == ICMPV6_FILTER)

===== arch/ppc64/kernel/sys_ppc32.c 1.8 vs edited =====

arch/ppc64/kernel/sys_ppc32.c +1 -1

@@ -3221,7 +3221,7 @@
 	
 	PPCDBG(PPCDBG_SYS32,"sys32_setsockopt - running - pid=%ld, comm=%s\n", current->pid, current->comm);
 
-	if (optname == SO_ATTACH_FILTER) {
+	if (level == SOL_SOCKET && optname == SO_ATTACH_FILTER) {
 		struct sock_fprog32 {
 			__u16 len;
 			__u32 filter;

===== arch/sparc64/kernel/sys_sparc32.c 1.33 vs edited =====

arch/sparc64/kernel/sys_sparc32.c +3 -2

@@ -2996,10 +2996,11 @@
 	if (optname == IPT_SO_SET_REPLACE)
 		return do_netfilter_replace(fd, level, optname,
 					    optval, optlen);
-	if (optname == SO_ATTACH_FILTER)
+	if (level == SOL_SOCKET && optname == SO_ATTACH_FILTER)
 		return do_set_attach_filter(fd, level, optname,
 					    optval, optlen);
-	if (optname == SO_RCVTIMEO || optname == SO_SNDTIMEO)
+	if (level == SOL_SOCKET && 
+	    (optname == SO_RCVTIMEO || optname == SO_SNDTIMEO))
 		return do_set_sock_timeout(fd, level, optname, optval, optlen);
 
 	return sys_setsockopt(fd, level, optname, optval, optlen);

===== arch/x86_64/ia32/socket32.c 1.4 vs edited =====

arch/x86_64/ia32/socket32.c +1 -1

@@ -555,7 +555,7 @@
 asmlinkage long sys32_setsockopt(int fd, int level, int optname,
 				char *optval, int optlen)
 {
-	if (optname == SO_ATTACH_FILTER)
+	if (level == SOL_SOCKET && optname == SO_ATTACH_FILTER)
 		return do_set_attach_filter(fd, level, optname,
 					    optval, optlen);
 	if (level == SOL_ICMPV6 && optname == ICMPV6_FILTER)

Signature · 1 line

dwmw2

David S. Miller

maintainer

2004-09-10 23:43 UTC #3 lore raw

On Thu, 09 Sep 2004 08:53:18 +0100
David Woodhouse <[email protected]> wrote:

2.4.x patch applied, thanks David.

Quoted 5 lines

Back in the real world of 2.6, we REALLY do need to stop trying to do all this in a sockopt syscall wrapper, and instead pass down a 32/64 bit flag to the code which actually handles the sockopt -- although the optlen ought to be sufficient for the _majority_ of cases. Or maybe we should handle it like we do ioctls?

That latter idea (doing it like ioctls) is an option, and in fact I like it since it would allow us to push the complicated netfilter translators into the netfilter code.