sctp: fix refcount bug in sctp_wfree

[ 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>

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 09cda66d0567ac5f1f8c8691a51b665d78553639..442780515760e290915e0f61f2671b5ec2ef7b6a 100644 (file)
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -175,29 +175,44 @@ static void sctp_clear_owner_w(struct sctp_chunk *chunk)
        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. */
@@ -8280,9 +8295,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_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.