sunrpc - fixup userspace buffer possible overrun v3
Subsystems (3)
PATCH 1/1 sunrpc - fixup userspace buffer possible overrun v3
Vegard Nossum reported
----------------------
Quoted 11 lines
I noticed that something weird is going on with
/proc/sys/sunrpc/transports. This file is generated innet/sunrpc/sysctl.c, functionproc_do_xprt(). When I "cat" this file, I get the expected output:$ cat /proc/sys/sunrpc/transports tcp 1048576 udp 32768But 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 toread()was just 1.
David Wagner added (among other things) that copy_to_user could be probably used here.
Ingo Oeser suggested to use simple_read_from_buffer() here.
The conclusion is that proc_do_xprt doesn't check for userside buffer size indeed so fix this by using Ingo's suggestion.
---
net/sunrpc/sysctl.c +12 -8
@@ -60,23 +60,27 @@ 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; +ssize_t ret; +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 ; + ret =simple_read_from_buffer (buffer, *lenp, ppos, + tmpbuf, len); +if (ret >=0 ) { + *lenp = ret; + ret =0 ; + } +return ret; } - *lenp -= len; - *ppos += len; +return 0 ; }
Quoted 7 lines · Cyrill Gorcunov
David Wagner added (among other things) that
copy_to_usercould be probably used here.Ingo Oeser suggested to use
simple_read_from_buffer()here.The conclusion is that
proc_do_xprtdoesn't check for userside buffer size indeed so fix this by using Ingo's suggestion.
Thanks for fixing this! And apologies for not looking closer when this first went in....
It looks like we could cut out a little more code while we're there:
Quoted 6 lines
+++
linux-2.6.git/net/sunrpc/sysctl.c2008-08-31 19:15:57.000000000 +0400(quoted diff) +0
-1@@ -60,23 +60,27 @@ static int proc_do_xprt(ctl_table *table void __user *buffer, size_t *lenp, loff_t *ppos) { char tmpbuf[256]; - int len;
Review static int proc_do_xprt(ctl_table *table · line 72
Quoted 10 lines
(quoted diff) +4
-0+ size_t len; + ssize_t ret; + if ((*ppos && !write) || !*lenp) { *lenp = 0; return 0; } + if (write) return -EINVAL;
The only caller of .proc_handler appears to be fs/proc/proc_sysctl.c:proc_sys_call_handler(), which checks permissions before calling us, so we'll never hit this case (since "transports" is given mode 0444, and root isn't allowed to bypass this).
Review static int proc_do_xprt(ctl_table *table · line 73
Quoted 1 line
(quoted diff) +0
-0else {
The previous return makes the "else" redundant.
Review static int proc_do_xprt(ctl_table *table · line 80
Quoted 12 lines
(quoted diff) +6
-5len = svc_print_xprts(tmpbuf, sizeof(tmpbuf)); - if (!access_ok(VERIFY_WRITE, buffer, len)) - return -EFAULT; - - if (__copy_to_user(buffer, tmpbuf, len)) - return -EFAULT; + ret = simple_read_from_buffer(buffer, *lenp, ppos, + tmpbuf, len); + if (ret >= 0) { + *lenp = ret; + ret = 0; + }
And it looks to me like the proc_sys_call_handler doesn't use *lenp when ret is nonzero, so this "if" is unnecessary.
Review static int proc_do_xprt(ctl_table *table · line 86
Quoted 8 lines
(quoted diff) +2
-2+ return ret; } - *lenp -= len; - *ppos += len; + return 0; }
That gives the following:
--b.
commit 20d0daba667a355e7c76362633423ab569d2193d
Author: Cyrill Gorcunov <[email protected]> Date: Sun Aug 31 19:25:49 2008 +0400
sunrpc - fixup userspace buffer possible overrun v3
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.
Ingo Oeser suggested to use simple_read_from_buffer() here.
The conclusion is that proc_do_xprt doesn't check for userside buffer size indeed so fix this by using Ingo's suggestion.
net/sunrpc/sysctl.c +4 -14
@@ -60,24 +60,14 @@ 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; +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)) -return -EFAULT ; - } - *lenp -= len; - *ppos += len; -return 0 ; + len =svc_print_xprts (tmpbuf,sizeof (tmpbuf)); +return simple_read_from_buffer (buffer, *lenp, ppos, tmpbuf, len); } static int
[J. Bruce Fields - Sun, Aug 31, 2008 at 09:17:09PM -0400]
...
| | That gives the following: | | --b. |
...
Thanks Bruce
- Cyrill -