netdev - Re: [PATCH] decnet: incorrect optlen size

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:	Thu, 29 Jan 2009 08:58:58 +0000
From:	steve@...gwyn.com
To:	Roel Kluin <roel.kluin@...il.com>
Cc:	christine.caulfield@...glemail.com,
	linux-decnet-user@...ts.sourceforge.net, netdev@...r.kernel.org
Subject: Re: [PATCH] decnet: incorrect optlen size

Hi,

On Thu, Jan 29, 2009 at 09:21:15AM +0100, Roel Kluin wrote:
> Several functions with something like this occur:
> 
> int sock_set_foo(int optlen, ...)
> {
> 	struct food foo;
> 
> 	if (optlen < sizeof(foo))
> 		return -EINVAL;
> 
> 	if (copy_from_user(&foo, optval, sizeof(foo)))
> 		return -EFAULT;
> 	...
> }
> 
> see for instance:
> grep -C5 -E -R -n "copy_from_user\(&([a-zA-Z0-9]*), optval, sizeof\(\1\)\)" net
> 
> but in __dn_setsockopt, below, the checks are slightly different.
> Should maybe the changes below be apllied?
> 
> -------------->8----------------8<-----------------------
> fix size checks before copy_from_user
> 
> Signed-off-by: Roel Kluin <roel.kluin@...il.com>
> ---
> diff --git a/net/decnet/af_decnet.c b/net/decnet/af_decnet.c
> index cf0e184..45b9199 100644
> --- a/net/decnet/af_decnet.c
> +++ b/net/decnet/af_decnet.c
> @@ -1359,10 +1359,10 @@ static int __dn_setsockopt(struct socket *sock, int level,int optname, char __us
>  	if (optlen && !optval)
>  		return -EINVAL;
>  
> -	if (optlen > sizeof(u))
> +	if (optlen < sizeof(u))
>  		return -EINVAL;
>
I don't see that this makes sense... we want to ensure that the passed
length is less than the size of the union which we are going to use
as a buffer.
  
> -	if (copy_from_user(&u, optval, optlen))
> +	if (copy_from_user(&u, optval, sizeof(u)))
>  		return -EFAULT;
>
 ... and here we only want to copy the amount of data that has actually
been supplied, not the whole buffer size since in many cases the
amount of data is less than the total buffer size.

The only sensible addition that I can see, would be to zero out the
buffer before the copy to ensure that we don't land up using data
from the stack in the case that the supplied data is less than that
required for a particular command.

Steve.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html