On Mon, Aug 27, 2018 at 06:38:31PM +0800, Xin Long wrote:
> 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Xin Long <lucien.xin@xxxxxxxxx>
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@xxxxxxxxx>
> ---
> 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
>
