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

[Cyrill Gorcunov <gorcunov@gmail.com>]

[Vegard Nossum - Sat, Aug 30, 2008 at 09:15:16PM +0200]
| On Sat, Aug 30, 2008 at 9:06 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| > [Vegard Nossum - Sat, Aug 30, 2008 at 08:44:22PM +0200]
| > | Hi,
| > |
| > | 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. So I have created a test program, see it at the bottom of
| > | this e-mail. Here is its output:
| > |
| > ...
| >
| > Indeed, maybe just add checking for user buffer length?
| > As proc_dodebug() in this file are doing. I don't think
| > the user would be happy with his stack burned :)
| >
| > Something like:
| > ---
| >
| > 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;
| >
| 
| Hm. I think this is wrong. Shouldn't we copy as many bytes as the user
| indicated?

Well, hard to say what user-space programmer is expecting from us.
I mean - maybe he (reader) wants only part of results not the whole
contents BUT by this way he never know what the whole conetnts would be
until trying to read more (ie to check if there no more data from
kernel side). What is preferred behaviour - i don't know :)

| 
| 
| Vegard
| 
| -- 
| "The animistic metaphor of the bug that maliciously sneaked in while
| the programmer was not looking is intellectually dishonest as it
| disguises that the error is the programmer's own creation."
| 	-- E. W. Dijkstra, EWD1036
| 

		- Cyrill -