[PATCH] sunrpc - fixup user buffer overrun on 'transports' statistics

[PATCH] sunrpc - fixup user buffer overrun on 'transports' statistics

[Cyrill Gorcunov]

Vegard Nossum at Sat, 30 Aug 2008 20:44:22 +0200
------------------------------------------------
> I noticed that something weird is going on with /proc/sys/sunrpc/transports.
> This file is generated in net/sunrpc/sysctl.c, function proc_do_xprt(). When
> I "cat" this file, I get the expected output:
>
>    $ cat /proc/sys/sunrpc/transports
>    tcp 1048576
>    udp 32768

> But I think that it does not check the length of the buffer supplied by
> userspace to read(). With my original program, I found that the stack was
> being overwritten by the characters above, even when the length given to
> read() was just 1.

David Wagner <daw@cs.berkeley.edu> Sat, 30 Aug 2008 22:55:51 +0000 (UTC)
------------------------------------------------------------------------
>
> 4. Is proc_dostring() relevant here?
>

proc_do_xprt doesn't check for userside buffer size indeed so
we better to use proc_dostring.

Reported-by: Vegard Nossum <vegard.nossum@gmail.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
CC: David Wagner <daw@cs.berkeley.edu>
---

Please check, I don't have sunrpc on my machine built.

Index: linux-2.6.git/net/sunrpc/sysctl.c
===================================================================
--- linux-2.6.git.orig/net/sunrpc/sysctl.c	2008-07-20 11:40:14.000000000 +0400
+++ linux-2.6.git/net/sunrpc/sysctl.c	2008-08-31 13:22:16.000000000 +0400
@@ -39,6 +39,7 @@ EXPORT_SYMBOL_GPL(nlm_debug);
 
 static struct ctl_table_header *sunrpc_table_header;
 static ctl_table		sunrpc_table[];
+static char sunrpc_transport_stat[256];
 
 void
 rpc_register_sysctl(void)
@@ -56,30 +57,6 @@ rpc_unregister_sysctl(void)
 	}
 }
 
-static int proc_do_xprt(ctl_table *table, int write, struct file *file,
-			void __user *buffer, size_t *lenp, loff_t *ppos)
-{
-	char tmpbuf[256];
-	int len;
-	if ((*ppos && !write) || !*lenp) {
-		*lenp = 0;
-		return 0;
-	}
-	if (write)
-		return -EINVAL;
-	else {
-		len = svc_print_xprts(tmpbuf, sizeof(tmpbuf));
-		if (!access_ok(VERIFY_WRITE, buffer, len))
-			return -EFAULT;
-
-		if (__copy_to_user(buffer, tmpbuf, len))
-			return -EFAULT;
-	}
-	*lenp -= len;
-	*ppos += len;
-	return 0;
-}
-
 static int
 proc_dodebug(ctl_table *table, int write, struct file *file,
 				void __user *buffer, size_t *lenp, loff_t *ppos)
@@ -174,9 +151,11 @@ static ctl_table debug_table[] = {
 	},
 	{
 		.procname	= "transports",
-		.maxlen		= 256,
+		.data		= &sunrpc_transport_stat,
+		.maxlen		= sizeof(sunrpc_transport_stat),
 		.mode		= 0444,
-		.proc_handler	= &proc_do_xprt,
+		.proc_handler	= &proc_dostring,
+		.strategy	= &sysctl_string
 	},
 	{ .ctl_name = 0 }
 };

Re: [PATCH] sunrpc - fixup user buffer overrun on 'transports' statistics

[Cyrill Gorcunov]

[Cyrill Gorcunov - Sun, Aug 31, 2008 at 01:37:05PM +0400]
| Vegard Nossum at Sat, 30 Aug 2008 20:44:22 +0200
...

IT'S CRAP!!! SORRY!!!

		- Cyrill -