netdev - Re: [PATCH net-next] sctp: Pull the user copies out of the individual sockopt functions.

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]

Message-ID: <20200521001725.GW2491@localhost.localdomain>
Date:   Wed, 20 May 2020 21:17:25 -0300
From:   'Marcelo Ricardo Leitner' <marcelo.leitner@...il.com>
To:     David Laight <David.Laight@...lab.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-sctp@...r.kernel.org" <linux-sctp@...r.kernel.org>,
        Neil Horman <nhorman@...driver.com>
Subject: Re: [PATCH net-next] sctp: Pull the user copies out of the
 individual sockopt functions.

On Wed, May 20, 2020 at 03:08:13PM +0000, David Laight wrote:
...
> Only SCTP_SOCKOPT_CONNECTX3 contains an indirect pointer.
> It is also the only getsockopt() that wants to return a buffer
> and an error code. It is also definitely abusing getsockopt().
...
> @@ -1375,11 +1350,11 @@ struct compat_sctp_getaddrs_old {
>  #endif
>  
>  static int sctp_getsockopt_connectx3(struct sock *sk, int len,
> -				     char __user *optval,
> -				     int __user *optlen)
> +				     struct sctp_getaddrs_old *param,
> +				     int *optlen)
>  {
> -	struct sctp_getaddrs_old param;
>  	sctp_assoc_t assoc_id = 0;
> +	struct sockaddr *addrs;
>  	int err = 0;
>  
>  #ifdef CONFIG_COMPAT
> @@ -1388,29 +1363,28 @@ static int sctp_getsockopt_connectx3(struct sock *sk, int len,
>  
>  		if (len < sizeof(param32))
>  			return -EINVAL;
> -		if (copy_from_user(&param32, optval, sizeof(param32)))
> -			return -EFAULT;
> +		param32 = *(struct compat_sctp_getaddrs_old *)param;
>  
> -		param.assoc_id = param32.assoc_id;
> -		param.addr_num = param32.addr_num;
> -		param.addrs = compat_ptr(param32.addrs);
> +		param->assoc_id = param32.assoc_id;
> +		param->addr_num = param32.addr_num;
> +		param->addrs = compat_ptr(param32.addrs);
>  	} else
>  #endif
>  	{
> -		if (len < sizeof(param))
> +		if (len < sizeof(*param))
>  			return -EINVAL;
> -		if (copy_from_user(&param, optval, sizeof(param)))
> -			return -EFAULT;
>  	}
>  
> -	err = __sctp_setsockopt_connectx(sk, (struct sockaddr __user *)
> -					 param.addrs, param.addr_num,
> +	addrs = memdup_user(param->addrs, param->addr_num);

I'm staring at this for a while now but I don't get this memdup_user.
AFAICT, params->addrs is not __user anymore here, because
sctp_getsockopt() copied the whole thing already, no?
Also weird because it is being called from kernel_sctp_getsockopt(),
which now has no knowledge of __user buffers.
Maybe I didn't get something from the patch description.

> +	if (IS_ERR(addrs))
> +		return PTR_ERR(addrs);
> +
> +	err = __sctp_setsockopt_connectx(sk, addrs, param->addr_num,
>  					 &assoc_id);
> +	kfree(addrs);
>  	if (err == 0 || err == -EINPROGRESS) {
> -		if (copy_to_user(optval, &assoc_id, sizeof(assoc_id)))
> -			return -EFAULT;
> -		if (put_user(sizeof(assoc_id), optlen))
> -			return -EFAULT;
> +		*(sctp_assoc_t *)param = assoc_id;
> +		*optlen = sizeof(assoc_id);
>  	}
>  
>  	return err;