Re: buffer overflow in /proc/sys/sunrpc/transports

[daw@cs.berkeley.edu (David Wagner)]

Cyrill Gorcunov  wrote:
>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-30 23:05:30.000000000 +0400
>@@ -69,6 +69,8 @@ static int proc_do_xprt(ctl_table *table
> 		return -EINVAL;
> 	else {
> 		len = svc_print_xprts(tmpbuf, sizeof(tmpbuf));
>+		if (*lenp < len)
>+			return -EFAULT;
> 		if (!access_ok(VERIFY_WRITE, buffer, len))
> 			return -EFAULT;

1. Would it be better to use copy_to_user() rather than
access_ok() followed immediately by __copy_to_user()?

2. Is it OK to dereference *lenp directly?  Is lenp a pointer into user
memory or kernel memory?  If it points to user memory, why is it safe to
dereference it directly?  (What about TOCTTOU bugs?)  Should there be
some sparse annotations here to ensure the code is not dereferencing
user pointers directly?  Later on, proc_do_xprt() also dereferences
*lenp and *ppos directly.

3. 'len' is declared as a signed int.  len will be converted to size_t
before doing the comparison, so if len can ever be negative (e.g.,
svc_print_xprts() returns -1 because of an error), this patch will do
the wrong thing.  Looks like the current definition of svc_print_xprts()
won't ever do that, as that code currently stands, so at present this
is not a bug.  However from a security point of view there are benefits
to code whose correctness is 'locally obvious', all else being equal.
In particular this seems like a possible maintenance hazard.  Would it be
better to use type size_t for lengths like this that are never supposed
to be negative?

4. Is proc_dostring() relevant here?