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

[David Miller]

From: Xin Long 
Date: Fri, 15 Sep 2017 11:02:21 +0800

> 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 
> Signed-off-by: Xin Long 

Applied.

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

[Neil Horman]

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 
> Signed-off-by: Xin Long 
> ---
>  include/net/sctp/sctp.h |  3 ++-
>  net/sctp/sctp_diag.c| 32 +---
>  net/sctp/socket.c   | 40 +---
>  3 files changed, 36 insertions(+), 39 deletions(-)
> 
Acked-by: Neil Horman 

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

[Marcelo Ricardo Leitner]

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 
> Signed-off-by: Xin Long 

Acked-by: Marcelo Ricardo Leitner 

> ---
>  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, _sk(sk)->ep->asocs, asocs) {
> + list_for_each_entry(assoc, >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