Re: [PATCH net] sctp: fix an use-after-free issue in sctp_sock_dump

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

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 <pabeni@redhat.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>