netdev - Re: [PATCH net] sctp: return error if the asoc has been peeled off in sctp_wait_for

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:   Mon, 15 Jan 2018 08:06:25 -0500
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,
        davem@...emloft.net,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Subject: Re: [PATCH net] sctp: return error if the asoc has been peeled off
 in sctp_wait_for_sndbuf

On Mon, Jan 15, 2018 at 05:01:36PM +0800, Xin Long wrote:
> After commit cea0cc80a677 ("sctp: use the right sk after waking up from
> wait_buf sleep"), it may change to lock another sk if the asoc has been
> peeled off in sctp_wait_for_sndbuf.
> 
> However, the asoc's new sk could be already closed elsewhere, as it's in
> the sendmsg context of the old sk that can't avoid the new sk's closing.
> If the sk's last one refcnt is held by this asoc, later on after putting
> this asoc, the new sk will be freed, while under it's own lock.
> 
> This patch is to revert that commit, but fix the old issue by returning
> error under the old sk's lock.
> 
> Fixes: cea0cc80a677 ("sctp: use the right sk after waking up from wait_buf sleep")
> Reported-by: syzbot+ac6ea7baa4432811eb50@...kaller.appspotmail.com
> Signed-off-by: Xin Long <lucien.xin@...il.com>
> ---
>  net/sctp/socket.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 15ae018..feb2ca6 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -85,7 +85,7 @@
>  static int sctp_writeable(struct sock *sk);
>  static void sctp_wfree(struct sk_buff *skb);
>  static int sctp_wait_for_sndbuf(struct sctp_association *asoc, long *timeo_p,
> -				size_t msg_len, struct sock **orig_sk);
> +				size_t msg_len);
>  static int sctp_wait_for_packet(struct sock *sk, int *err, long *timeo_p);
>  static int sctp_wait_for_connect(struct sctp_association *, long *timeo_p);
>  static int sctp_wait_for_accept(struct sock *sk, long timeo);
> @@ -1977,7 +1977,7 @@ static int sctp_sendmsg(struct sock *sk, struct msghdr *msg, size_t msg_len)
>  	timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
>  	if (!sctp_wspace(asoc)) {
>  		/* sk can be changed by peel off when waiting for buf. */
> -		err = sctp_wait_for_sndbuf(asoc, &timeo, msg_len, &sk);
> +		err = sctp_wait_for_sndbuf(asoc, &timeo, msg_len);
>  		if (err) {
>  			if (err == -ESRCH) {
>  				/* asoc is already dead. */
> @@ -8022,12 +8022,12 @@ void sctp_sock_rfree(struct sk_buff *skb)
>  
>  /* Helper function to wait for space in the sndbuf.  */
>  static int sctp_wait_for_sndbuf(struct sctp_association *asoc, long *timeo_p,
> -				size_t msg_len, struct sock **orig_sk)
> +				size_t msg_len)
>  {
>  	struct sock *sk = asoc->base.sk;
> -	int err = 0;
>  	long current_timeo = *timeo_p;
>  	DEFINE_WAIT(wait);
> +	int err = 0;
>  
>  	pr_debug("%s: asoc:%p, timeo:%ld, msg_len:%zu\n", __func__, asoc,
>  		 *timeo_p, msg_len);
> @@ -8056,17 +8056,13 @@ static int sctp_wait_for_sndbuf(struct sctp_association *asoc, long *timeo_p,
>  		release_sock(sk);
>  		current_timeo = schedule_timeout(current_timeo);
>  		lock_sock(sk);
> -		if (sk != asoc->base.sk) {
> -			release_sock(sk);
> -			sk = asoc->base.sk;
> -			lock_sock(sk);
> -		}
> +		if (sk != asoc->base.sk)
> +			goto do_error;
Is this a safe comparison to make (thinking in terms both of non-cache coherent
arches, or, more likely, of cases where the sock slab reuses an object leading
to the same pointer).  Would it be better to have a single point of freeing the
sock and use the SOCK_DEAD flag here?

Neil

>  
>  		*timeo_p = current_timeo;
>  	}
>  
>  out:
> -	*orig_sk = sk;
>  	finish_wait(&asoc->wait, &wait);
>  
>  	/* Release the association's refcnt.  */
> -- 
> 2.1.0
> 
>