netdev - Re: [PATCH net] sctp: reset owner sk for data chunks on out queues when migrating a sock

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: <20171027215337.GA3675@localhost.localdomain>
Date:   Fri, 27 Oct 2017 19:53:37 -0200
From:   Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:     Xin Long <lucien.xin@...il.com>
Cc:     network dev <netdev@...r.kernel.org>, linux-sctp@...r.kernel.org,
        davem@...emloft.net, Neil Horman <nhorman@...driver.com>,
        Vlad Yasevich <vyasevich@...il.com>,
        Dmitry Vyukov <dvyukov@...gle.com>, chunwang@...hat.com,
        syzkaller@...glegroups.com
Subject: Re: [PATCH net] sctp: reset owner sk for data chunks on out queues
 when migrating a sock

On Sat, Oct 28, 2017 at 02:13:29AM +0800, Xin Long wrote:
> Now when migrating sock to another one in sctp_sock_migrate(), it only
> resets owner sk for the data in receive queues, not the chunks on out
> queues.
> 
> It would cause that data chunks length on the sock is not consistent
> with sk sk_wmem_alloc. When closing the sock or freeing these chunks,
> the old sk would never be freed, and the new sock may crash due to
> the overflow sk_wmem_alloc.
> 
> syzbot found this issue with this series:
> 
>   r0 = socket$inet_sctp()
>   sendto$inet(r0)
>   listen(r0)
>   accept4(r0)
>   close(r0)
> 
> Although listen() should have returned error when one TCP-style socket
> is in connecting (I may fix this one in another patch), it could also
> be reproduced by peeling off an assoc.
> 
> This issue is there since very beginning.
> 
> This patch is to reset owner sk for the chunks on out queues so that
> sk sk_wmem_alloc has correct value after accept one sock or peeloff
> an assoc to one sock.
> 
> Note that when resetting owner sk for chunks on outqueue, it has to
> sctp_clear_owner_w/skb_orphan chunks before changing assoc->base.sk
> first and then sctp_set_owner_w them after changing assoc->base.sk,
> due to that sctp_wfree and it's callees are using assoc->base.sk.
> 
> Reported-by: Dmitry Vyukov <dvyukov@...gle.com>
> Signed-off-by: Xin Long <lucien.xin@...il.com>

Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>

> ---
>  net/sctp/socket.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 17841ab..6f45d17 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -170,6 +170,36 @@ static inline void sctp_set_owner_w(struct sctp_chunk *chunk)
>  	sk_mem_charge(sk, chunk->skb->truesize);
>  }
>  
> +static void sctp_clear_owner_w(struct sctp_chunk *chunk)
> +{
> +	skb_orphan(chunk->skb);
> +}
> +
> +static void sctp_for_each_tx_datachunk(struct sctp_association *asoc,
> +				       void (*cb)(struct sctp_chunk *))
> +
> +{
> +	struct sctp_outq *q = &asoc->outqueue;
> +	struct sctp_transport *t;
> +	struct sctp_chunk *chunk;
> +
> +	list_for_each_entry(t, &asoc->peer.transport_addr_list, transports)
> +		list_for_each_entry(chunk, &t->transmitted, transmitted_list)
> +			cb(chunk);
> +
> +	list_for_each_entry(chunk, &q->retransmit, list)
> +		cb(chunk);
> +
> +	list_for_each_entry(chunk, &q->sacked, list)
> +		cb(chunk);
> +
> +	list_for_each_entry(chunk, &q->abandoned, list)
> +		cb(chunk);
> +
> +	list_for_each_entry(chunk, &q->out_chunk_list, list)
> +		cb(chunk);
> +}
> +
>  /* Verify that this is a valid address. */
>  static inline int sctp_verify_addr(struct sock *sk, union sctp_addr *addr,
>  				   int len)
> @@ -8212,7 +8242,9 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
>  	 * paths won't try to lock it and then oldsk.
>  	 */
>  	lock_sock_nested(newsk, SINGLE_DEPTH_NESTING);
> +	sctp_for_each_tx_datachunk(assoc, sctp_clear_owner_w);
>  	sctp_assoc_migrate(assoc, newsk);
> +	sctp_for_each_tx_datachunk(assoc, sctp_set_owner_w);
>  
>  	/* If the association on the newsk is already closed before accept()
>  	 * is called, set RCV_SHUTDOWN flag.
> -- 
> 2.1.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>