On Fri, Sep 15, 2017 at 11:02:21AM +0800, Xin Long wrote:
> Commit 86fdb3448cc1 ("sctp: ensure ep is not destroyed before doing the
> dump") tried to fix an use-after-free issue by checking !sctp_sk(sk)->ep
> with holding sock and sock lock.
>
> But Paolo noticed that endpoint could be destroyed in sctp_rcv without
> sock lock protection. It means the use-after-free issue still could be
> triggered when sctp_rcv put and destroy ep after sctp_sock_dump checks
> !ep, although it's pretty hard to reproduce.
>
> I could reproduce it by mdelay in sctp_rcv while msleep in sctp_close
> and sctp_sock_dump long time.
>
> This patch is to add another param cb_done to sctp_for_each_transport
> and dump ep->assocs with holding tsp after jumping out of transport's
> traversal in it to avoid this issue.
>
> It can also improve sctp diag dump to make it run faster, as no need
> to save sk into cb->args[5] and keep calling sctp_for_each_transport
> any more.
>
> This patch is also to use int * instead of int for the pos argument
> in sctp_for_each_transport, which could make postion increment only
> in sctp_for_each_transport and no need to keep changing cb->args[2]
> in sctp_sock_filter and sctp_sock_dump any more.
>
> Fixes: 86fdb3448cc1 ("sctp: ensure ep is not destroyed before doing the dump")
> Reported-by: Paolo Abeni <pab...@redhat.com>
> Signed-off-by: Xin Long <lucien....@gmail.com>
Acked-by: Marcelo Ricardo Leitner <marcelo.leit...@gmail.com>
> ---
> include/net/sctp/sctp.h | 3 ++-
> net/sctp/sctp_diag.c | 32 +++++++++-----------------------
> net/sctp/socket.c | 40 +++++++++++++++++++++++++---------------
> 3 files changed, 36 insertions(+), 39 deletions(-)
>
> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
> index 06b4f51..d7d8cba 100644
> --- a/include/net/sctp/sctp.h
> +++ b/include/net/sctp/sctp.h
> @@ -127,7 +127,8 @@ int sctp_transport_lookup_process(int (*cb)(struct
> sctp_transport *, void *),
> const union sctp_addr *laddr,
> const union sctp_addr *paddr, void *p);
> int sctp_for_each_transport(int (*cb)(struct sctp_transport *, void *),
> - struct net *net, int pos, void *p);
> + int (*cb_done)(struct sctp_transport *, void *),
> + struct net *net, int *pos, void *p);
> int sctp_for_each_endpoint(int (*cb)(struct sctp_endpoint *, void *), void
> *p);
> int sctp_get_sctp_info(struct sock *sk, struct sctp_association *asoc,
> struct sctp_info *info);
> diff --git a/net/sctp/sctp_diag.c b/net/sctp/sctp_diag.c
> index e99518e..7008a99 100644
> --- a/net/sctp/sctp_diag.c
> +++ b/net/sctp/sctp_diag.c
> @@ -279,9 +279,11 @@ static int sctp_tsp_dump_one(struct sctp_transport *tsp,
> void *p)
> return err;
> }
>
> -static int sctp_sock_dump(struct sock *sk, void *p)
> +static int sctp_sock_dump(struct sctp_transport *tsp, void *p)
> {
> + struct sctp_endpoint *ep = tsp->asoc->ep;
> struct sctp_comm_param *commp = p;
> + struct sock *sk = ep->base.sk;
> struct sk_buff *skb = commp->skb;
> struct netlink_callback *cb = commp->cb;
> const struct inet_diag_req_v2 *r = commp->r;
> @@ -289,9 +291,7 @@ static int sctp_sock_dump(struct sock *sk, void *p)
> int err = 0;
>
> lock_sock(sk);
> - if (!sctp_sk(sk)->ep)
> - goto release;
> - list_for_each_entry(assoc, &sctp_sk(sk)->ep->asocs, asocs) {
> + list_for_each_entry(assoc, &ep->asocs, asocs) {
> if (cb->args[4] < cb->args[1])
> goto next;
>
> @@ -327,40 +327,30 @@ static int sctp_sock_dump(struct sock *sk, void *p)
> cb->args[4]++;
> }
> cb->args[1] = 0;
> - cb->args[2]++;
> cb->args[3] = 0;
> cb->args[4] = 0;
> release:
> release_sock(sk);
> - sock_put(sk);
> return err;
> }
>
> -static int sctp_get_sock(struct sctp_transport *tsp, void *p)
> +static int sctp_sock_filter(struct sctp_transport *tsp, void *p)
> {
> struct sctp_endpoint *ep = tsp->asoc->ep;
> struct sctp_comm_param *commp = p;
> struct sock *sk = ep->base.sk;
> - struct netlink_callback *cb = commp->cb;
> const struct inet_diag_req_v2 *r = commp->r;
> struct sctp_association *assoc =
> list_entry(ep->asocs.next, struct sctp_association, asocs);
>
> /* find the ep only once through the transports by this condition */
> if (tsp->asoc != assoc)
> - goto out;
> + return 0;
>
> if (r->sdiag_family != AF_UNSPEC && sk->sk_family != r->sdiag_family)
> - goto out;
> -
> - sock_hold(sk);
> - cb->args[5] = (long)sk;
> + return 0;
>
> return 1;
> -
> -out:
> - cb->args[2]++;
> - return 0;
> }
>
> static int sctp_ep_dump(struct sctp_endpoint *ep, void *p)
> @@ -503,12 +493,8 @@ static void sctp_diag_dump(struct sk_buff *skb, struct
> netlink_callback *cb,
> if (!(idiag_states & ~(TCPF_LISTEN | TCPF_CLOSE)))
> goto done;
>
> -next:
> - cb->args[5] = 0;
> - sctp_for_each_transport(sctp_get_sock, net, cb->args[2], &commp);
> -
> - if (cb->args[5] && !sctp_sock_dump((struct sock *)cb->args[5], &commp))
> - goto next;
> + sctp_for_each_transport(sctp_sock_filter, sctp_sock_dump,
> + net, (int *)&cb->args[2], &commp);
>
> done:
> cb->args[1] = cb->args[4];
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 1b00a1e..d4730ad 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -4658,29 +4658,39 @@ int sctp_transport_lookup_process(int (*cb)(struct
> sctp_transport *, void *),
> EXPORT_SYMBOL_GPL(sctp_transport_lookup_process);
>
> int sctp_for_each_transport(int (*cb)(struct sctp_transport *, void *),
> - struct net *net, int pos, void *p) {
> + int (*cb_done)(struct sctp_transport *, void *),
> + struct net *net, int *pos, void *p) {
> struct rhashtable_iter hti;
> - void *obj;
> - int err;
> -
> - err = sctp_transport_walk_start(&hti);
> - if (err)
> - return err;
> + struct sctp_transport *tsp;
> + int ret;
>
> - obj = sctp_transport_get_idx(net, &hti, pos + 1);
> - for (; !IS_ERR_OR_NULL(obj); obj = sctp_transport_get_next(net, &hti)) {
> - struct sctp_transport *transport = obj;
> +again:
> + ret = sctp_transport_walk_start(&hti);
> + if (ret)
> + return ret;
>
> - if (!sctp_transport_hold(transport))
> + 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;
> - err = cb(transport, p);
> - sctp_transport_put(transport);
> - if (err)
> + ret = cb(tsp, p);
> + if (ret)
> break;
> + (*pos)++;
> + sctp_transport_put(tsp);
> }
> sctp_transport_walk_stop(&hti);
>
> - return err;
> + if (ret) {
> + if (cb_done && !cb_done(tsp, p)) {
> + (*pos)++;
> + sctp_transport_put(tsp);
> + goto again;
> + }
> + sctp_transport_put(tsp);
> + }
> +
> + return ret;
> }
> EXPORT_SYMBOL_GPL(sctp_for_each_transport);
>
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
