netdev - Re: [PATCH V6 2/4] sctp: Add ip option support

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: <20180214203046.GB4625@localhost.localdomain>
Date:   Wed, 14 Feb 2018 18:30:46 -0200
From:   Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:     Richard Haines <richard_c_haines@...nternet.com>
Cc:     selinux@...ho.nsa.gov, netdev@...r.kernel.org,
        linux-sctp@...r.kernel.org, linux-security-module@...r.kernel.org,
        paul@...l-moore.com, vyasevich@...il.com, nhorman@...driver.com,
        sds@...ho.nsa.gov, eparis@...isplace.org, casey@...aufler-ca.com,
        james.l.morris@...cle.com
Subject: Re: [PATCH V6 2/4] sctp: Add ip option support

Hi,

On Tue, Feb 13, 2018 at 08:54:44PM +0000, Richard Haines wrote:
...
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index bf271f8..8307968 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -3138,6 +3138,7 @@ static int sctp_setsockopt_mappedv4(struct sock *sk, char __user *optval, unsign
>  static int sctp_setsockopt_maxseg(struct sock *sk, char __user *optval, unsigned int optlen)
>  {
>  	struct sctp_sock *sp = sctp_sk(sk);
> +	struct sctp_af *af = sp->pf->af;
>  	struct sctp_assoc_value params;
>  	struct sctp_association *asoc;
>  	int val;
> @@ -3159,10 +3160,13 @@ static int sctp_setsockopt_maxseg(struct sock *sk, char __user *optval, unsigned
>  		return -EINVAL;
>  	}
>  
> +	asoc = sctp_id2assoc(sk, params.assoc_id);
> +
>  	if (val) {
>  		int min_len, max_len;
>  
> -		min_len = SCTP_DEFAULT_MINSEGMENT - sp->pf->af->net_header_len;
> +		min_len = SCTP_DEFAULT_MINSEGMENT - af->net_header_len;
> +		min_len -= af->ip_options_len(asoc->base.sk);

We have an issue here. asoc may be NULL, in case the user supplied
asoc id is invalid. Then if it also specified the maxseg 'val', it
would trigger a NULL deref here.

You don't need to find the asoc, to then get to the socket. You can
use the sk function parameter, it's the same value here. Then you
don't need to move the sctp_id2assoc() call.

>  		min_len -= sizeof(struct sctphdr) +
>  			   sizeof(struct sctp_data_chunk);
>  
> @@ -3172,10 +3176,10 @@ static int sctp_setsockopt_maxseg(struct sock *sk, char __user *optval, unsigned
>  			return -EINVAL;
>  	}
>  
> -	asoc = sctp_id2assoc(sk, params.assoc_id);
>  	if (asoc) {
>  		if (val == 0) {
> -			val = asoc->pathmtu - sp->pf->af->net_header_len;
> +			val = asoc->pathmtu - af->net_header_len;
> +			val -= af->ip_options_len(asoc->base.sk);

You may use sk directly here too.

Other than these, LGTM.

>  			val -= sizeof(struct sctphdr) +
>  			       sctp_datachk_len(&asoc->stream);
>  		}
> @@ -5087,9 +5091,11 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
>  	sctp_copy_sock(sock->sk, sk, asoc);
>  
>  	/* Make peeled-off sockets more like 1-1 accepted sockets.
> -	 * Set the daddr and initialize id to something more random
> +	 * Set the daddr and initialize id to something more random and also
> +	 * copy over any ip options.
>  	 */
>  	sp->pf->to_sk_daddr(&asoc->peer.primary_addr, sk);
> +	sp->pf->copy_ip_options(sk, sock->sk);
>  
>  	/* Populate the fields of the newsk from the oldsk and migrate the
>  	 * asoc to the newsk.
> -- 
> 2.14.3
>