| From foo@baz Fri 03 Apr 2020 11:04:16 AM CEST |
| From: Qiujun Huang <hqjagain@gmail.com> |
| Date: Fri, 27 Mar 2020 11:07:51 +0800 |
| Subject: sctp: fix refcount bug in sctp_wfree |
| |
| From: Qiujun Huang <hqjagain@gmail.com> |
| |
| [ Upstream commit 5c3e82fe159622e46e91458c1a6509c321a62820 ] |
| |
| We should iterate over the datamsgs to move |
| all chunks(skbs) to newsk. |
| |
| The following case cause the bug: |
| for the trouble SKB, it was in outq->transmitted list |
| |
| sctp_outq_sack |
| sctp_check_transmitted |
| SKB was moved to outq->sacked list |
| then throw away the sack queue |
| SKB was deleted from outq->sacked |
| (but it was held by datamsg at sctp_datamsg_to_asoc |
| So, sctp_wfree was not called here) |
| |
| then migrate happened |
| |
| sctp_for_each_tx_datachunk( |
| sctp_clear_owner_w); |
| sctp_assoc_migrate(); |
| sctp_for_each_tx_datachunk( |
| sctp_set_owner_w); |
| SKB was not in the outq, and was not changed to newsk |
| |
| finally |
| |
| __sctp_outq_teardown |
| sctp_chunk_put (for another skb) |
| sctp_datamsg_put |
| __kfree_skb(msg->frag_list) |
| sctp_wfree (for SKB) |
| SKB->sk was still oldsk (skb->sk != asoc->base.sk). |
| |
| Reported-and-tested-by: syzbot+cea71eec5d6de256d54d@syzkaller.appspotmail.com |
| Signed-off-by: Qiujun Huang <hqjagain@gmail.com> |
| Acked-by: Marcelo Ricardo Leitner <mleitner@redhat.com> |
| Signed-off-by: David S. Miller <davem@davemloft.net> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| --- |
| net/sctp/socket.c | 31 +++++++++++++++++++++++-------- |
| 1 file changed, 23 insertions(+), 8 deletions(-) |
| |
| --- a/net/sctp/socket.c |
| +++ b/net/sctp/socket.c |
| @@ -173,29 +173,44 @@ static void sctp_clear_owner_w(struct sc |
| skb_orphan(chunk->skb); |
| } |
| |
| +#define traverse_and_process() \ |
| +do { \ |
| + msg = chunk->msg; \ |
| + if (msg == prev_msg) \ |
| + continue; \ |
| + list_for_each_entry(c, &msg->chunks, frag_list) { \ |
| + if ((clear && asoc->base.sk == c->skb->sk) || \ |
| + (!clear && asoc->base.sk != c->skb->sk)) \ |
| + cb(c); \ |
| + } \ |
| + prev_msg = msg; \ |
| +} while (0) |
| + |
| static void sctp_for_each_tx_datachunk(struct sctp_association *asoc, |
| + bool clear, |
| void (*cb)(struct sctp_chunk *)) |
| |
| { |
| + struct sctp_datamsg *msg, *prev_msg = NULL; |
| struct sctp_outq *q = &asoc->outqueue; |
| + struct sctp_chunk *chunk, *c; |
| 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); |
| + traverse_and_process(); |
| |
| list_for_each_entry(chunk, &q->retransmit, transmitted_list) |
| - cb(chunk); |
| + traverse_and_process(); |
| |
| list_for_each_entry(chunk, &q->sacked, transmitted_list) |
| - cb(chunk); |
| + traverse_and_process(); |
| |
| list_for_each_entry(chunk, &q->abandoned, transmitted_list) |
| - cb(chunk); |
| + traverse_and_process(); |
| |
| list_for_each_entry(chunk, &q->out_chunk_list, list) |
| - cb(chunk); |
| + traverse_and_process(); |
| } |
| |
| /* Verify that this is a valid address. */ |
| @@ -7878,9 +7893,9 @@ static void sctp_sock_migrate(struct soc |
| * 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_for_each_tx_datachunk(assoc, true, sctp_clear_owner_w); |
| sctp_assoc_migrate(assoc, newsk); |
| - sctp_for_each_tx_datachunk(assoc, sctp_set_owner_w); |
| + sctp_for_each_tx_datachunk(assoc, false, sctp_set_owner_w); |
| |
| /* If the association on the newsk is already closed before accept() |
| * is called, set RCV_SHUTDOWN flag. |