netdev - Re: [PATCH net-next 1/4] sctp: check addr_size with sa_family_t size in __sctp_setsockopt

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: <20190723152449.GB8419@localhost.localdomain>
Date:   Tue, 23 Jul 2019 11:24:49 -0400
From:   Neil Horman <nhorman@...driver.com>
To:     Xin Long <lucien.xin@...il.com>
Cc:     network dev <netdev@...r.kernel.org>, linux-sctp@...r.kernel.org,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        davem@...emloft.net
Subject: Re: [PATCH net-next 1/4] sctp: check addr_size with sa_family_t size
 in __sctp_setsockopt_connectx

On Tue, Jul 23, 2019 at 01:37:57AM +0800, Xin Long wrote:
> Now __sctp_connect() is called by __sctp_setsockopt_connectx() and
> sctp_inet_connect(), the latter has done addr_size check with size
> of sa_family_t.
> 
> In the next patch to clean up __sctp_connect(), we will remove
> addr_size check with size of sa_family_t from __sctp_connect()
> for the 1st address.
> 
> So before doing that, __sctp_setsockopt_connectx() should do
> this check first, as sctp_inet_connect() does.
> 
> Signed-off-by: Xin Long <lucien.xin@...il.com>
> ---
>  net/sctp/socket.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index aa80cda..5f92e4a 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -1311,7 +1311,7 @@ static int __sctp_setsockopt_connectx(struct sock *sk,
>  	pr_debug("%s: sk:%p addrs:%p addrs_size:%d\n",
>  		 __func__, sk, addrs, addrs_size);
>  
> -	if (unlikely(addrs_size <= 0))
> +	if (unlikely(addrs_size < sizeof(sa_family_t)))
I don't think this is what you want to check for here.  sa_family_t is
an unsigned short, and addrs_size is the number of bytes in the addrs
array.  The addrs array should be at least the size of one struct
sockaddr (16 bytes iirc), and, if larger, should be a multiple of
sizeof(struct sockaddr)

Neil

>  		return -EINVAL;
>  
>  	kaddrs = memdup_user(addrs, addrs_size);
> -- 
> 2.1.0
> 
>