On Mon, Aug 27, 2018 at 9:08 PM Neil Horman [off-list ref] wrote:

On Mon, Aug 27, 2018 at 06:38:31PM +0800, Xin Long wrote:

quoted

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>
---
 net/sctp/proc.c   |  4 ----
 net/sctp/socket.c | 22 +++++++++++++++-------
 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/net/sctp/proc.c b/net/sctp/proc.c
index ef5c9a8..4d6f1c8 100644
--- 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 e96b15a..aa76586 100644
--- 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 @@ int sctp_for_each_transport(int (*cb)(struct sctp_transport *, void *),

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

2.1.0

Acked-by: Neil Horman <nhorman@tuxdriver.com>

Additionally, its not germaine to this particular fix, but why are we still
using that pos variable in sctp_transport_get_idx?  With the conversion to
rhashtables, it doesn't seem particularly useful anymore.

For proc, seems so, hti is saved into seq->private.
But for diag, "hti" in sctp_for_each_transport() is a local variable.
do you think where we can save it?