Messages in this thread Patch in this message | | | Date | Sun, 31 Aug 2008 14:08:06 +0400 | From | Cyrill Gorcunov <> | Subject | [PATCH] sunrpc - fixup userspace buffer possible overrun v2 |
| |
Vegard Nossum reported ---------------------- > 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 added (among other things) that copy_to_user could be probably used here.
The conclusion is that proc_do_xprt doesn't check for userside buffer size indeed so fix. Also set lenp to number of bytes were really written.
Reported-by: Vegard Nossum <vegard.nossum@gmail.com> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com> CC: David Wagner <daw@cs.berkeley.edu> ---
Please review.
Index: linux-2.6.git/net/sunrpc/sysctl.c =================================================================== --- linux-2.6.git.orig/net/sunrpc/sysctl.c 2008-08-31 13:43:46.000000000 +0400 +++ linux-2.6.git/net/sunrpc/sysctl.c 2008-08-31 13:58:14.000000000 +0400 @@ -60,23 +60,26 @@ static int proc_do_xprt(ctl_table *table void __user *buffer, size_t *lenp, loff_t *ppos) { char tmpbuf[256]; - int len; + size_t 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)) + if (len > *lenp) + len = *lenp; + if (copy_to_user(buffer, tmpbuf, len)) return -EFAULT; } - *lenp -= len; + + *lenp = len; *ppos += len; + return 0; }
| |