linux-kernel - Re: [PATCH] sunrpc - fixup userspace buffer possible overrun v3

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 31 Aug 2008 21:17:09 -0400
From:	"J. Bruce Fields" <bfields@...ldses.org>
To:	Cyrill Gorcunov <gorcunov@...il.com>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Vegard Nossum <vegard.nossum@...il.com>,
	Ingo Oeser <ioe-lkml@...eria.de>, neilb@...e.de
Subject: Re: [PATCH] sunrpc - fixup userspace buffer possible overrun v3

On Sun, Aug 31, 2008 at 07:25:49PM +0400, Cyrill Gorcunov wrote:
> 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.

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:

> +++ linux-2.6.git/net/sunrpc/sysctl.c	2008-08-31 19:15:57.000000000 +0400
> @@ -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;

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).


>  	else {

The previous return makes the "else" redundant.

>  		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;
> +		}

And it looks to me like the proc_sys_call_handler doesn't use *lenp when
ret is nonzero, so this "if" is unnecessary.

> +		return ret;
>  	}
> -	*lenp -= len;
> -	*ppos += len;
> +
>  	return 0;
>  }
>  

That gives the following:

--b.


commit 20d0daba667a355e7c76362633423ab569d2193d
Author: Cyrill Gorcunov <gorcunov@...il.com>
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.
    
    Reported-by: Vegard Nossum <vegard.nossum@...il.com>
    Signed-off-by: Cyrill Gorcunov <gorcunov@...il.com>
    CC: Ingo Oeser <ioe-lkml@...eria.de>
    Signed-off-by: J. Bruce Fields <bfields@...i.umich.edu>

diff --git a/net/sunrpc/sysctl.c b/net/sunrpc/sysctl.c
index 0f8c439..5231f7a 100644
--- a/net/sunrpc/sysctl.c
+++ b/net/sunrpc/sysctl.c
@@ -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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/