sctp: hold transport before accessing its asoc in sctp_transport_get_next

As Marcelo noticed, in sctp_transport_get_next, it is iterating over
transports but then also accessing the association directly, without
checking any refcnts before that, which can cause an use-after-free
Read.

So fix it by holding transport before accessing the association. With
that, sctp_transport_hold calls can be removed in the later places.

Fixes: 626d16f50f39 ("sctp: export some apis or variables for sctp_diag and reuse some for proc")
Reported-by: syzbot+fe62a0c9aa6a85c6de16@syzkaller.appspotmail.com
Signed-off-by: Xin Long <lucien.xin@gmail.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/net/sctp/proc.c b/net/sctp/proc.c
index ef5c9a82d4e896b506271cb229b07ee570c917f3..4d6f1c8d66596033992fb708d6a6407d9ace0b16 100644 (file)
--- a/net/sctp/proc.c
+++ b/net/sctp/proc.c
@@ -264,8 +264,6 @@ static int sctp_assocs_seq_show(struct seq_file *seq, void *v)
        }
 
        transport = (struct sctp_transport *)v;
-       if (!sctp_transport_hold(transport))
-               return 0;
        assoc = transport->asoc;
        epb = &assoc->base;
        sk = epb->sk;
@@ -322,8 +320,6 @@ static int sctp_remaddr_seq_show(struct seq_file *seq, void *v)
        }
 
        transport = (struct sctp_transport *)v;
-       if (!sctp_transport_hold(transport))
-               return 0;
        assoc = transport->asoc;
 
        list_for_each_entry_rcu(tsp, &assoc->peer.transport_addr_list,

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index e96b15a66abaa481b64db1ebd34f75d59d724c3d..aa76586a1a1c8a243df93e6074be3681a767c18b 100644 (file)
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -5005,9 +5005,14 @@ struct sctp_transport *sctp_transport_get_next(struct net *net,
                        break;
                }
 
+               if (!sctp_transport_hold(t))
+                       continue;
+
                if (net_eq(sock_net(t->asoc->base.sk), net) &&
                    t->asoc->peer.primary_path == t)
                        break;
+
+               sctp_transport_put(t);
        }
 
        return t;
@@ -5017,13 +5022,18 @@ struct sctp_transport *sctp_transport_get_idx(struct net *net,
                                              struct rhashtable_iter *iter,
                                              int pos)
 {
-       void *obj = SEQ_START_TOKEN;
+       struct sctp_transport *t;
 
-       while (pos && (obj = sctp_transport_get_next(net, iter)) &&
-              !IS_ERR(obj))
-               pos--;
+       if (!pos)
+               return SEQ_START_TOKEN;
 
-       return obj;
+       while ((t = sctp_transport_get_next(net, iter)) && !IS_ERR(t)) {
+               if (!--pos)
+                       break;
+               sctp_transport_put(t);
+       }
+
+       return t;
 }
 
 int sctp_for_each_endpoint(int (*cb)(struct sctp_endpoint *, void *),
@@ -5082,8 +5092,6 @@ again:
 
        tsp = sctp_transport_get_idx(net, &hti, *pos + 1);
        for (; !IS_ERR_OR_NULL(tsp); tsp = sctp_transport_get_next(net, &hti)) {
-               if (!sctp_transport_hold(tsp))
-                       continue;
                ret = cb(tsp, p);
                if (ret)
                        break;