[PATCH net] sctp: hold transport before accessing its asoc in sctp_transport_get_next

[PATCH net] sctp: hold transport before accessing its asoc in sctp_transport_get_next

[Xin Long]

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

Re: [PATCH net] sctp: hold transport before accessing its asoc in sctp_transport_get_next

[Neil Horman]

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@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.

Neil

Re: [PATCH net] sctp: hold transport before accessing its asoc in sctp_transport_get_next

[Xin Long]

On Mon, Aug 27, 2018 at 9:08 PM Neil Horman <nhorman@tuxdriver.com> wrote:
>
> 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@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?

Re: [PATCH net] sctp: hold transport before accessing its asoc in sctp_transport_get_next

[Neil Horman]

On Wed, Aug 29, 2018 at 12:08:40AM +0800, Xin Long wrote:
> On Mon, Aug 27, 2018 at 9:08 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> >
> > 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@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?
> 
Sorry, wasn't suggesting that it had to be removed from sctp_for_each_trasnport,
its clearly used as both an input and output there.  All I was sugesting was
that, in sctp_transport_get_idx, the pos variable might no longer be needed
there specifically, as sctp_transprt_get_next should terminate the loop on its
own.  Or is there another purpose for that positional variable I am missing

Neil

Re: [PATCH net] sctp: hold transport before accessing its asoc in sctp_transport_get_next

[Xin Long]

On Wed, Aug 29, 2018 at 7:36 PM Neil Horman <nhorman@tuxdriver.com> wrote:
>
> On Wed, Aug 29, 2018 at 12:08:40AM +0800, Xin Long wrote:
> > On Mon, Aug 27, 2018 at 9:08 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > >
> > > 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@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?
> >
> Sorry, wasn't suggesting that it had to be removed from sctp_for_each_trasnport,
> its clearly used as both an input and output there.  All I was sugesting was
> that, in sctp_transport_get_idx, the pos variable might no longer be needed
> there specifically, as sctp_transprt_get_next should terminate the loop on its
> own.  Or is there another purpose for that positional variable I am missing
Yes, Neil, all transports/asocs could not be dumped once as one buffer/rtnl msg
is not big enough for them. when coming into proc/diag again, it needs to start
from the *next* one, and 'pos' is used to save its position.

Without 'pos', it would always start from the first one and dump the duplicate
ones in the next times. Make sense?

>
> Neil
>

Re: [PATCH net] sctp: hold transport before accessing its asoc in sctp_transport_get_next

[Neil Horman]

On Fri, Aug 31, 2018 at 03:09:05PM +0800, Xin Long wrote:
> On Wed, Aug 29, 2018 at 7:36 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> >
> > On Wed, Aug 29, 2018 at 12:08:40AM +0800, Xin Long wrote:
> > > On Mon, Aug 27, 2018 at 9:08 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > > >
> > > > 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@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?
> > >
> > Sorry, wasn't suggesting that it had to be removed from sctp_for_each_trasnport,
> > its clearly used as both an input and output there.  All I was sugesting was
> > that, in sctp_transport_get_idx, the pos variable might no longer be needed
> > there specifically, as sctp_transprt_get_next should terminate the loop on its
> > own.  Or is there another purpose for that positional variable I am missing
> Yes, Neil, all transports/asocs could not be dumped once as one buffer/rtnl msg
> is not big enough for them. when coming into proc/diag again, it needs to start
> from the *next* one, and 'pos' is used to save its position.
> 
> Without 'pos', it would always start from the first one and dump the duplicate
> ones in the next times. Make sense?
> 
You're missing what I'm trying to say.  I'm speaking specifically about
sctp_transport_get_idx here.  In that function, pos is passed by value, and has
no bearing on if sctp_transport_get_idx starts at the beginning of the list, or
not, thats in control of the iterator entirely here. If we remove pos from the
parameter list, at worst, the iterator continues until the end of the list,
which I think is fine.

No?
Neil

> >
> > Neil
> >
> 

Re: [PATCH net] sctp: hold transport before accessing its asoc in sctp_transport_get_next

[Neil Horman]

On Fri, Aug 31, 2018 at 08:03:23AM -0400, Neil Horman wrote:
> On Fri, Aug 31, 2018 at 03:09:05PM +0800, Xin Long wrote:
> > On Wed, Aug 29, 2018 at 7:36 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > >
> > > On Wed, Aug 29, 2018 at 12:08:40AM +0800, Xin Long wrote:
> > > > On Mon, Aug 27, 2018 at 9:08 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > > > >
> > > > > 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@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?
> > > >
> > > Sorry, wasn't suggesting that it had to be removed from sctp_for_each_trasnport,
> > > its clearly used as both an input and output there.  All I was sugesting was
> > > that, in sctp_transport_get_idx, the pos variable might no longer be needed
> > > there specifically, as sctp_transprt_get_next should terminate the loop on its
> > > own.  Or is there another purpose for that positional variable I am missing
> > Yes, Neil, all transports/asocs could not be dumped once as one buffer/rtnl msg
> > is not big enough for them. when coming into proc/diag again, it needs to start
> > from the *next* one, and 'pos' is used to save its position.
> > 
> > Without 'pos', it would always start from the first one and dump the duplicate
> > ones in the next times. Make sense?
> > 
> You're missing what I'm trying to say.  I'm speaking specifically about
> sctp_transport_get_idx here.  In that function, pos is passed by value, and has
> no bearing on if sctp_transport_get_idx starts at the beginning of the list, or
> not, thats in control of the iterator entirely here. If we remove pos from the
> parameter list, at worst, the iterator continues until the end of the list,
> which I think is fine.
> 
> No?
> Neil
> 

Sorry, slight correction here, I see what you were trying to say.  You're saying
that the one user of sctp_for_each_transport (sctp_diag_dump), uses the pos
variable to start at a point other than the head of the list, because the
netlink protocol that uses that allows you to index into it that way.  Sorry
about that.

That said, thats....odd.  Its certainly no in keeping with the other _for_each
methods the kernel has.  The for_each construct typically iterates over the
entire list regardless, and leaves filtering up to the caller.  I'd suggest we
do the same, by not requireing a positional argument, and instead checking that
positional argument in the callback.  I'll write a patch for it this week

Neil

> > >
> > > Neil
> > >
> > 
> 

Re: [PATCH net] sctp: hold transport before accessing its asoc in sctp_transport_get_next

[Marcelo Ricardo Leitner]

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

Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@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
> 

Re: [PATCH net] sctp: hold transport before accessing its asoc in sctp_transport_get_next

[David Miller]

From: Xin Long <lucien.xin@gmail.com>
Date: Mon, 27 Aug 2018 18:38:31 +0800

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

Applied and queued up for -stable.

[PATCH net] sctp: hold transport before accessing its asoc in sctp_hash_transport

[Xin Long]

In sctp_hash_transport, it dereferences a transport's asoc only under
rcu_read_lock. Without holding the transport, its asoc could be freed
already, which leads to a use-after-free panic.

A similar fix as Commit bab1be79a516 ("sctp: hold transport before
accessing its asoc in sctp_transport_get_next") is needed to hold
the transport before accessing its asoc in sctp_hash_transport.

Fixes: cd2b70875058 ("sctp: check duplicate node before inserting a new transport")
Reported-by: syzbot+0b05d8aa7cb185107483@syzkaller.appspotmail.com
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/input.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/sctp/input.c b/net/sctp/input.c
index 5c36a99..69584e9 100644
--- a/net/sctp/input.c
+++ b/net/sctp/input.c
@@ -896,11 +896,16 @@ int sctp_hash_transport(struct sctp_transport *t)
 	list = rhltable_lookup(&sctp_transport_hashtable, &arg,
 			       sctp_hash_params);
 
-	rhl_for_each_entry_rcu(transport, tmp, list, node)
+	rhl_for_each_entry_rcu(transport, tmp, list, node) {
+		if (!sctp_transport_hold(transport))
+			continue;
 		if (transport->asoc->ep = t->asoc->ep) {
+			sctp_transport_put(transport);
 			rcu_read_unlock();
 			return -EEXIST;
 		}
+		sctp_transport_put(transport);
+	}
 	rcu_read_unlock();
 
 	err = rhltable_insert_key(&sctp_transport_hashtable, &arg,
-- 
2.1.0

Re: [PATCH net] sctp: hold transport before accessing its asoc in sctp_hash_transport

[Neil Horman]

On Tue, Nov 20, 2018 at 07:09:16PM +0800, Xin Long wrote:
> In sctp_hash_transport, it dereferences a transport's asoc only under
> rcu_read_lock. Without holding the transport, its asoc could be freed
> already, which leads to a use-after-free panic.
> 
> A similar fix as Commit bab1be79a516 ("sctp: hold transport before
> accessing its asoc in sctp_transport_get_next") is needed to hold
> the transport before accessing its asoc in sctp_hash_transport.
> 
> Fixes: cd2b70875058 ("sctp: check duplicate node before inserting a new transport")
> Reported-by: syzbot+0b05d8aa7cb185107483@syzkaller.appspotmail.com
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/sctp/input.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/net/sctp/input.c b/net/sctp/input.c
> index 5c36a99..69584e9 100644
> --- a/net/sctp/input.c
> +++ b/net/sctp/input.c
> @@ -896,11 +896,16 @@ int sctp_hash_transport(struct sctp_transport *t)
>  	list = rhltable_lookup(&sctp_transport_hashtable, &arg,
>  			       sctp_hash_params);
>  
> -	rhl_for_each_entry_rcu(transport, tmp, list, node)
> +	rhl_for_each_entry_rcu(transport, tmp, list, node) {
> +		if (!sctp_transport_hold(transport))
> +			continue;
>  		if (transport->asoc->ep = t->asoc->ep) {
> +			sctp_transport_put(transport);
>  			rcu_read_unlock();
>  			return -EEXIST;
>  		}
> +		sctp_transport_put(transport);
> +	}
>  	rcu_read_unlock();
>  
>  	err = rhltable_insert_key(&sctp_transport_hashtable, &arg,
> -- 
> 2.1.0
> 
> 

something doesn't feel at all right about this.  If we are inserting a transport
to an association, it would seem to me that we should have at least one user of
the association (i.e. non-zero refcount).  As such it seems something is wrong
with the association refcount here.  At the very least, if there is a case where
an association is being removed while a transport is being added, the better
solution would be to ensure that sctp_association_destroy goes through a
quiescent point prior to unhashing transports from the list, to ensure that
there is no conflict with the add operation above.

Neil

Re: [PATCH net] sctp: hold transport before accessing its asoc in sctp_hash_transport

[Marcelo Ricardo Leitner]

On Tue, Nov 20, 2018 at 07:52:48AM -0500, Neil Horman wrote:
> On Tue, Nov 20, 2018 at 07:09:16PM +0800, Xin Long wrote:
> > In sctp_hash_transport, it dereferences a transport's asoc only under
> > rcu_read_lock. Without holding the transport, its asoc could be freed
> > already, which leads to a use-after-free panic.
> > 
> > A similar fix as Commit bab1be79a516 ("sctp: hold transport before
> > accessing its asoc in sctp_transport_get_next") is needed to hold
> > the transport before accessing its asoc in sctp_hash_transport.
> > 
> > Fixes: cd2b70875058 ("sctp: check duplicate node before inserting a new transport")
> > Reported-by: syzbot+0b05d8aa7cb185107483@syzkaller.appspotmail.com
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  net/sctp/input.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/sctp/input.c b/net/sctp/input.c
> > index 5c36a99..69584e9 100644
> > --- a/net/sctp/input.c
> > +++ b/net/sctp/input.c
> > @@ -896,11 +896,16 @@ int sctp_hash_transport(struct sctp_transport *t)
> >  	list = rhltable_lookup(&sctp_transport_hashtable, &arg,
> >  			       sctp_hash_params);
> >  
> > -	rhl_for_each_entry_rcu(transport, tmp, list, node)
> > +	rhl_for_each_entry_rcu(transport, tmp, list, node) {
> > +		if (!sctp_transport_hold(transport))
> > +			continue;
> >  		if (transport->asoc->ep = t->asoc->ep) {
> > +			sctp_transport_put(transport);
> >  			rcu_read_unlock();
> >  			return -EEXIST;
> >  		}
> > +		sctp_transport_put(transport);
> > +	}
> >  	rcu_read_unlock();
> >  
> >  	err = rhltable_insert_key(&sctp_transport_hashtable, &arg,
> > -- 
> > 2.1.0
> > 
> > 
> 
> something doesn't feel at all right about this.  If we are inserting a transport
> to an association, it would seem to me that we should have at least one user of
> the association (i.e. non-zero refcount).  As such it seems something is wrong
> with the association refcount here.  At the very least, if there is a case where
> an association is being removed while a transport is being added, the better
> solution would be to ensure that sctp_association_destroy goes through a
> quiescent point prior to unhashing transports from the list, to ensure that
> there is no conflict with the add operation above.

Consider that the rhl_for_each_entry_rcu() is traversing the global
rhashtable, and that it may operate on unrelated transports/asocs.
E.g., transport->asoc in the for() is potentially different from the
asoc under socket lock.

The core of the fix is at:
+		if (!sctp_transport_hold(transport))
+			continue;
If we can get a hold, the asoc will be available for dereferencing in
subsequent lines. Otherwise, move on.

With that, the patch makes sense to me.

Although I would prefer if we come up with a better way to do this
jump, or even avoid the jump. We are only comparing pointers here and
if we had asoc->ep cached on sctp_transport itself, we could avoid the
atomics here.

This change, in the next patch on sctp_epaddr_lookup_transport, will
hurt performance as that is called in datapath. Rhashtable will help
on keeping entry lists to a size, but still.

  Marcelo

Re: [PATCH net] sctp: hold transport before accessing its asoc in sctp_hash_transport

[Xin Long]

On Wed, Nov 21, 2018 at 9:46 AM Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
>
> On Tue, Nov 20, 2018 at 07:52:48AM -0500, Neil Horman wrote:
> > On Tue, Nov 20, 2018 at 07:09:16PM +0800, Xin Long wrote:
> > > In sctp_hash_transport, it dereferences a transport's asoc only under
> > > rcu_read_lock. Without holding the transport, its asoc could be freed
> > > already, which leads to a use-after-free panic.
> > >
> > > A similar fix as Commit bab1be79a516 ("sctp: hold transport before
> > > accessing its asoc in sctp_transport_get_next") is needed to hold
> > > the transport before accessing its asoc in sctp_hash_transport.
> > >
> > > Fixes: cd2b70875058 ("sctp: check duplicate node before inserting a new transport")
> > > Reported-by: syzbot+0b05d8aa7cb185107483@syzkaller.appspotmail.com
> > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > ---
> > >  net/sctp/input.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/net/sctp/input.c b/net/sctp/input.c
> > > index 5c36a99..69584e9 100644
> > > --- a/net/sctp/input.c
> > > +++ b/net/sctp/input.c
> > > @@ -896,11 +896,16 @@ int sctp_hash_transport(struct sctp_transport *t)
> > >     list = rhltable_lookup(&sctp_transport_hashtable, &arg,
> > >                            sctp_hash_params);
> > >
> > > -   rhl_for_each_entry_rcu(transport, tmp, list, node)
> > > +   rhl_for_each_entry_rcu(transport, tmp, list, node) {
> > > +           if (!sctp_transport_hold(transport))
> > > +                   continue;
> > >             if (transport->asoc->ep = t->asoc->ep) {
> > > +                   sctp_transport_put(transport);
> > >                     rcu_read_unlock();
> > >                     return -EEXIST;
> > >             }
> > > +           sctp_transport_put(transport);
> > > +   }
> > >     rcu_read_unlock();
> > >
> > >     err = rhltable_insert_key(&sctp_transport_hashtable, &arg,
> > > --
> > > 2.1.0
> > >
> > >
> >
> > something doesn't feel at all right about this.  If we are inserting a transport
> > to an association, it would seem to me that we should have at least one user of
> > the association (i.e. non-zero refcount).  As such it seems something is wrong
> > with the association refcount here.  At the very least, if there is a case where
> > an association is being removed while a transport is being added, the better
> > solution would be to ensure that sctp_association_destroy goes through a
> > quiescent point prior to unhashing transports from the list, to ensure that
> > there is no conflict with the add operation above.
Changing to do call_rcu(&transport->rcu, sctp_association_destroy) can
work for this case.
But it means asoc and socket (taking the port) will have to wait for a
grace period, which is not expected. We seemed to have talked about
this before, Marcelo?

>
> Consider that the rhl_for_each_entry_rcu() is traversing the global
> rhashtable, and that it may operate on unrelated transports/asocs.
> E.g., transport->asoc in the for() is potentially different from the
> asoc under socket lock.
>
> The core of the fix is at:
> +               if (!sctp_transport_hold(transport))
> +                       continue;
> If we can get a hold, the asoc will be available for dereferencing in
> subsequent lines. Otherwise, move on.
>
> With that, the patch makes sense to me.
>
> Although I would prefer if we come up with a better way to do this
> jump, or even avoid the jump. We are only comparing pointers here and,
> if we had asoc->ep cached on sctp_transport itself, we could avoid the
> atomics here.
Right, but it's another u64.

>
> This change, in the next patch on sctp_epaddr_lookup_transport, will
> hurt performance as that is called in datapath. Rhashtable will help
> on keeping entry lists to a size, but still.
This loop is not long normally, will only a few atomic operations hurt
a noticeable performance?

Re: [PATCH net] sctp: hold transport before accessing its asoc in sctp_hash_transport

[Marcelo Ricardo Leitner]

On Wed, Nov 21, 2018 at 03:47:33PM +0900, Xin Long wrote:
> On Wed, Nov 21, 2018 at 9:46 AM Marcelo Ricardo Leitner
> <marcelo.leitner@gmail.com> wrote:
> >
> > On Tue, Nov 20, 2018 at 07:52:48AM -0500, Neil Horman wrote:
> > > On Tue, Nov 20, 2018 at 07:09:16PM +0800, Xin Long wrote:
> > > > In sctp_hash_transport, it dereferences a transport's asoc only under
> > > > rcu_read_lock. Without holding the transport, its asoc could be freed
> > > > already, which leads to a use-after-free panic.
> > > >
> > > > A similar fix as Commit bab1be79a516 ("sctp: hold transport before
> > > > accessing its asoc in sctp_transport_get_next") is needed to hold
> > > > the transport before accessing its asoc in sctp_hash_transport.
> > > >
> > > > Fixes: cd2b70875058 ("sctp: check duplicate node before inserting a new transport")
> > > > Reported-by: syzbot+0b05d8aa7cb185107483@syzkaller.appspotmail.com
> > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > > ---
> > > >  net/sctp/input.c | 7 ++++++-
> > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/net/sctp/input.c b/net/sctp/input.c
> > > > index 5c36a99..69584e9 100644
> > > > --- a/net/sctp/input.c
> > > > +++ b/net/sctp/input.c
> > > > @@ -896,11 +896,16 @@ int sctp_hash_transport(struct sctp_transport *t)
> > > >     list = rhltable_lookup(&sctp_transport_hashtable, &arg,
> > > >                            sctp_hash_params);
> > > >
> > > > -   rhl_for_each_entry_rcu(transport, tmp, list, node)
> > > > +   rhl_for_each_entry_rcu(transport, tmp, list, node) {
> > > > +           if (!sctp_transport_hold(transport))
> > > > +                   continue;
> > > >             if (transport->asoc->ep = t->asoc->ep) {
> > > > +                   sctp_transport_put(transport);
> > > >                     rcu_read_unlock();
> > > >                     return -EEXIST;
> > > >             }
> > > > +           sctp_transport_put(transport);
> > > > +   }
> > > >     rcu_read_unlock();
> > > >
> > > >     err = rhltable_insert_key(&sctp_transport_hashtable, &arg,
> > > > --
> > > > 2.1.0
> > > >
> > > >
> > >
> > > something doesn't feel at all right about this.  If we are inserting a transport
> > > to an association, it would seem to me that we should have at least one user of
> > > the association (i.e. non-zero refcount).  As such it seems something is wrong
> > > with the association refcount here.  At the very least, if there is a case where
> > > an association is being removed while a transport is being added, the better
> > > solution would be to ensure that sctp_association_destroy goes through a
> > > quiescent point prior to unhashing transports from the list, to ensure that
> > > there is no conflict with the add operation above.
> Changing to do call_rcu(&transport->rcu, sctp_association_destroy) can
> work for this case.
> But it means asoc and socket (taking the port) will have to wait for a
> grace period, which is not expected. We seemed to have talked about
> this before, Marcelo?

Yes. This would cause it to linger longer and cause bind conflicts
meanwhile.
Note that we already have sctp_transport_destroy_rcu(), so this would
be a 2nd grace period.

> 
> >
> > Consider that the rhl_for_each_entry_rcu() is traversing the global
> > rhashtable, and that it may operate on unrelated transports/asocs.
> > E.g., transport->asoc in the for() is potentially different from the
> > asoc under socket lock.
> >
> > The core of the fix is at:
> > +               if (!sctp_transport_hold(transport))
> > +                       continue;
> > If we can get a hold, the asoc will be available for dereferencing in
> > subsequent lines. Otherwise, move on.
> >
> > With that, the patch makes sense to me.
> >
> > Although I would prefer if we come up with a better way to do this
> > jump, or even avoid the jump. We are only comparing pointers here and,
> > if we had asoc->ep cached on sctp_transport itself, we could avoid the
> > atomics here.
> Right, but it's another u64.

Strictly speaking, a pointer :-) (32bits, on 32bits archs)
But just an idea. It would cost one additional pointer per transport
but saves the atomics and also one extra dereference per iteration.

> 
> >
> > This change, in the next patch on sctp_epaddr_lookup_transport, will
> > hurt performance as that is called in datapath. Rhashtable will help
> > on keeping entry lists to a size, but still.
> This loop is not long normally, will only a few atomic operations hurt

Right.

> a noticeable performance?

I guess we can't know without actually testing this.

Re: [PATCH net] sctp: hold transport before accessing its asoc in sctp_hash_transport

[Xin Long]

On Thu, Nov 22, 2018 at 2:53 AM Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
>
> On Wed, Nov 21, 2018 at 03:47:33PM +0900, Xin Long wrote:
> > On Wed, Nov 21, 2018 at 9:46 AM Marcelo Ricardo Leitner
> > <marcelo.leitner@gmail.com> wrote:
> > >
> > > On Tue, Nov 20, 2018 at 07:52:48AM -0500, Neil Horman wrote:
> > > > On Tue, Nov 20, 2018 at 07:09:16PM +0800, Xin Long wrote:
> > > > > In sctp_hash_transport, it dereferences a transport's asoc only under
> > > > > rcu_read_lock. Without holding the transport, its asoc could be freed
> > > > > already, which leads to a use-after-free panic.
> > > > >
> > > > > A similar fix as Commit bab1be79a516 ("sctp: hold transport before
> > > > > accessing its asoc in sctp_transport_get_next") is needed to hold
> > > > > the transport before accessing its asoc in sctp_hash_transport.
> > > > >
> > > > > Fixes: cd2b70875058 ("sctp: check duplicate node before inserting a new transport")
> > > > > Reported-by: syzbot+0b05d8aa7cb185107483@syzkaller.appspotmail.com
> > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > > > ---
> > > > >  net/sctp/input.c | 7 ++++++-
> > > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/net/sctp/input.c b/net/sctp/input.c
> > > > > index 5c36a99..69584e9 100644
> > > > > --- a/net/sctp/input.c
> > > > > +++ b/net/sctp/input.c
> > > > > @@ -896,11 +896,16 @@ int sctp_hash_transport(struct sctp_transport *t)
> > > > >     list = rhltable_lookup(&sctp_transport_hashtable, &arg,
> > > > >                            sctp_hash_params);
> > > > >
> > > > > -   rhl_for_each_entry_rcu(transport, tmp, list, node)
> > > > > +   rhl_for_each_entry_rcu(transport, tmp, list, node) {
> > > > > +           if (!sctp_transport_hold(transport))
> > > > > +                   continue;
> > > > >             if (transport->asoc->ep = t->asoc->ep) {
> > > > > +                   sctp_transport_put(transport);
> > > > >                     rcu_read_unlock();
> > > > >                     return -EEXIST;
> > > > >             }
> > > > > +           sctp_transport_put(transport);
> > > > > +   }
> > > > >     rcu_read_unlock();
> > > > >
> > > > >     err = rhltable_insert_key(&sctp_transport_hashtable, &arg,
> > > > > --
> > > > > 2.1.0
> > > > >
> > > > >
> > > >
> > > > something doesn't feel at all right about this.  If we are inserting a transport
> > > > to an association, it would seem to me that we should have at least one user of
> > > > the association (i.e. non-zero refcount).  As such it seems something is wrong
> > > > with the association refcount here.  At the very least, if there is a case where
> > > > an association is being removed while a transport is being added, the better
> > > > solution would be to ensure that sctp_association_destroy goes through a
> > > > quiescent point prior to unhashing transports from the list, to ensure that
> > > > there is no conflict with the add operation above.
> > Changing to do call_rcu(&transport->rcu, sctp_association_destroy) can
> > work for this case.
> > But it means asoc and socket (taking the port) will have to wait for a
> > grace period, which is not expected. We seemed to have talked about
> > this before, Marcelo?
>
> Yes. This would cause it to linger longer and cause bind conflicts
> meanwhile.
> Note that we already have sctp_transport_destroy_rcu(), so this would
> be a 2nd grace period.
>
> >
> > >
> > > Consider that the rhl_for_each_entry_rcu() is traversing the global
> > > rhashtable, and that it may operate on unrelated transports/asocs.
> > > E.g., transport->asoc in the for() is potentially different from the
> > > asoc under socket lock.
> > >
> > > The core of the fix is at:
> > > +               if (!sctp_transport_hold(transport))
> > > +                       continue;
> > > If we can get a hold, the asoc will be available for dereferencing in
> > > subsequent lines. Otherwise, move on.
> > >
> > > With that, the patch makes sense to me.
> > >
> > > Although I would prefer if we come up with a better way to do this
> > > jump, or even avoid the jump. We are only comparing pointers here and,
> > > if we had asoc->ep cached on sctp_transport itself, we could avoid the
> > > atomics here.
> > Right, but it's another u64.
>
> Strictly speaking, a pointer :-) (32bits, on 32bits archs)
> But just an idea. It would cost one additional pointer per transport
> but saves the atomics and also one extra dereference per iteration.
>
> >
> > >
> > > This change, in the next patch on sctp_epaddr_lookup_transport, will
> > > hurt performance as that is called in datapath. Rhashtable will help
> > > on keeping entry lists to a size, but still.
> > This loop is not long normally, will only a few atomic operations hurt
>
> Right.
>
> > a noticeable performance?
>
> I guess we can't know without actually testing this.
I couldn't see a noticeable performance hurt in my testing with the
extra couples of atomic operations in datapath, which becomes very
light with rhlist. I will post v2 with some more changelog for this
patch and the other one in sctp_epaddr_lookup_transport().

Thanks.

Re: [PATCH net] sctp: hold transport before accessing its asoc in sctp_hash_transport

[Marcelo Ricardo Leitner]

On Wed, Nov 28, 2018 at 06:36:45PM +0900, Xin Long wrote:
> On Thu, Nov 22, 2018 at 2:53 AM Marcelo Ricardo Leitner
> <marcelo.leitner@gmail.com> wrote:
> >
> > On Wed, Nov 21, 2018 at 03:47:33PM +0900, Xin Long wrote:
> > > On Wed, Nov 21, 2018 at 9:46 AM Marcelo Ricardo Leitner
> > > <marcelo.leitner@gmail.com> wrote:
> > > >
> > > > On Tue, Nov 20, 2018 at 07:52:48AM -0500, Neil Horman wrote:
> > > > > On Tue, Nov 20, 2018 at 07:09:16PM +0800, Xin Long wrote:
> > > > > > In sctp_hash_transport, it dereferences a transport's asoc only under
> > > > > > rcu_read_lock. Without holding the transport, its asoc could be freed
> > > > > > already, which leads to a use-after-free panic.
> > > > > >
> > > > > > A similar fix as Commit bab1be79a516 ("sctp: hold transport before
> > > > > > accessing its asoc in sctp_transport_get_next") is needed to hold
> > > > > > the transport before accessing its asoc in sctp_hash_transport.
> > > > > >
> > > > > > Fixes: cd2b70875058 ("sctp: check duplicate node before inserting a new transport")
> > > > > > Reported-by: syzbot+0b05d8aa7cb185107483@syzkaller.appspotmail.com
> > > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > > > > ---
> > > > > >  net/sctp/input.c | 7 ++++++-
> > > > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/net/sctp/input.c b/net/sctp/input.c
> > > > > > index 5c36a99..69584e9 100644
> > > > > > --- a/net/sctp/input.c
> > > > > > +++ b/net/sctp/input.c
> > > > > > @@ -896,11 +896,16 @@ int sctp_hash_transport(struct sctp_transport *t)
> > > > > >     list = rhltable_lookup(&sctp_transport_hashtable, &arg,
> > > > > >                            sctp_hash_params);
> > > > > >
> > > > > > -   rhl_for_each_entry_rcu(transport, tmp, list, node)
> > > > > > +   rhl_for_each_entry_rcu(transport, tmp, list, node) {
> > > > > > +           if (!sctp_transport_hold(transport))
> > > > > > +                   continue;
> > > > > >             if (transport->asoc->ep = t->asoc->ep) {
> > > > > > +                   sctp_transport_put(transport);
> > > > > >                     rcu_read_unlock();
> > > > > >                     return -EEXIST;
> > > > > >             }
> > > > > > +           sctp_transport_put(transport);
> > > > > > +   }
> > > > > >     rcu_read_unlock();
> > > > > >
> > > > > >     err = rhltable_insert_key(&sctp_transport_hashtable, &arg,
> > > > > > --
> > > > > > 2.1.0
> > > > > >
> > > > > >
> > > > >
> > > > > something doesn't feel at all right about this.  If we are inserting a transport
> > > > > to an association, it would seem to me that we should have at least one user of
> > > > > the association (i.e. non-zero refcount).  As such it seems something is wrong
> > > > > with the association refcount here.  At the very least, if there is a case where
> > > > > an association is being removed while a transport is being added, the better
> > > > > solution would be to ensure that sctp_association_destroy goes through a
> > > > > quiescent point prior to unhashing transports from the list, to ensure that
> > > > > there is no conflict with the add operation above.
> > > Changing to do call_rcu(&transport->rcu, sctp_association_destroy) can
> > > work for this case.
> > > But it means asoc and socket (taking the port) will have to wait for a
> > > grace period, which is not expected. We seemed to have talked about
> > > this before, Marcelo?
> >
> > Yes. This would cause it to linger longer and cause bind conflicts
> > meanwhile.
> > Note that we already have sctp_transport_destroy_rcu(), so this would
> > be a 2nd grace period.
> >
> > >
> > > >
> > > > Consider that the rhl_for_each_entry_rcu() is traversing the global
> > > > rhashtable, and that it may operate on unrelated transports/asocs.
> > > > E.g., transport->asoc in the for() is potentially different from the
> > > > asoc under socket lock.
> > > >
> > > > The core of the fix is at:
> > > > +               if (!sctp_transport_hold(transport))
> > > > +                       continue;
> > > > If we can get a hold, the asoc will be available for dereferencing in
> > > > subsequent lines. Otherwise, move on.
> > > >
> > > > With that, the patch makes sense to me.
> > > >
> > > > Although I would prefer if we come up with a better way to do this
> > > > jump, or even avoid the jump. We are only comparing pointers here and,
> > > > if we had asoc->ep cached on sctp_transport itself, we could avoid the
> > > > atomics here.
> > > Right, but it's another u64.
> >
> > Strictly speaking, a pointer :-) (32bits, on 32bits archs)
> > But just an idea. It would cost one additional pointer per transport
> > but saves the atomics and also one extra dereference per iteration.
> >
> > >
> > > >
> > > > This change, in the next patch on sctp_epaddr_lookup_transport, will
> > > > hurt performance as that is called in datapath. Rhashtable will help
> > > > on keeping entry lists to a size, but still.
> > > This loop is not long normally, will only a few atomic operations hurt
> >
> > Right.
> >
> > > a noticeable performance?
> >
> > I guess we can't know without actually testing this.
> I couldn't see a noticeable performance hurt in my testing with the
> extra couples of atomic operations in datapath, which becomes very
> light with rhlist. I will post v2 with some more changelog for this
> patch and the other one in sctp_epaddr_lookup_transport().
> 
> Thanks.

Cool, thanks Xin.

FWIW, Xin and I talked offline about this and we couldn't get to a way
of doing it without adding at least another pointer in sctp_transport,
which wouldn't be justified considering the fact Xin mentions above.

Re: [PATCH net] sctp: hold transport before accessing its asoc in sctp_hash_transport

[Neil Horman]

On Tue, Nov 20, 2018 at 10:46:26PM -0200, Marcelo Ricardo Leitner wrote:
> On Tue, Nov 20, 2018 at 07:52:48AM -0500, Neil Horman wrote:
> > On Tue, Nov 20, 2018 at 07:09:16PM +0800, Xin Long wrote:
> > > In sctp_hash_transport, it dereferences a transport's asoc only under
> > > rcu_read_lock. Without holding the transport, its asoc could be freed
> > > already, which leads to a use-after-free panic.
> > > 
> > > A similar fix as Commit bab1be79a516 ("sctp: hold transport before
> > > accessing its asoc in sctp_transport_get_next") is needed to hold
> > > the transport before accessing its asoc in sctp_hash_transport.
> > > 
> > > Fixes: cd2b70875058 ("sctp: check duplicate node before inserting a new transport")
> > > Reported-by: syzbot+0b05d8aa7cb185107483@syzkaller.appspotmail.com
> > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > ---
> > >  net/sctp/input.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/net/sctp/input.c b/net/sctp/input.c
> > > index 5c36a99..69584e9 100644
> > > --- a/net/sctp/input.c
> > > +++ b/net/sctp/input.c
> > > @@ -896,11 +896,16 @@ int sctp_hash_transport(struct sctp_transport *t)
> > >  	list = rhltable_lookup(&sctp_transport_hashtable, &arg,
> > >  			       sctp_hash_params);
> > >  
> > > -	rhl_for_each_entry_rcu(transport, tmp, list, node)
> > > +	rhl_for_each_entry_rcu(transport, tmp, list, node) {
> > > +		if (!sctp_transport_hold(transport))
> > > +			continue;
> > >  		if (transport->asoc->ep = t->asoc->ep) {
> > > +			sctp_transport_put(transport);
> > >  			rcu_read_unlock();
> > >  			return -EEXIST;
> > >  		}
> > > +		sctp_transport_put(transport);
> > > +	}
> > >  	rcu_read_unlock();
> > >  
> > >  	err = rhltable_insert_key(&sctp_transport_hashtable, &arg,
> > > -- 
> > > 2.1.0
> > > 
> > > 
> > 
> > something doesn't feel at all right about this.  If we are inserting a transport
> > to an association, it would seem to me that we should have at least one user of
> > the association (i.e. non-zero refcount).  As such it seems something is wrong
> > with the association refcount here.  At the very least, if there is a case where
> > an association is being removed while a transport is being added, the better
> > solution would be to ensure that sctp_association_destroy goes through a
> > quiescent point prior to unhashing transports from the list, to ensure that
> > there is no conflict with the add operation above.
> 
> Consider that the rhl_for_each_entry_rcu() is traversing the global
> rhashtable, and that it may operate on unrelated transports/asocs.
> E.g., transport->asoc in the for() is potentially different from the
> asoc under socket lock.
> 
Ah, ok, we're comparing associations that are not related to the association
being searched for, that makes sense.

> The core of the fix is at:
> +		if (!sctp_transport_hold(transport))
> +			continue;
> If we can get a hold, the asoc will be available for dereferencing in
> subsequent lines. Otherwise, move on.
> 
> With that, the patch makes sense to me.
> 
Yes, I agree, but as you note below, this still seems like a lousy way to fix
the problem.

> Although I would prefer if we come up with a better way to do this
> jump, or even avoid the jump. We are only comparing pointers here and
> if we had asoc->ep cached on sctp_transport itself, we could avoid the
> atomics here.
> 
> This change, in the next patch on sctp_epaddr_lookup_transport, will
> hurt performance as that is called in datapath. Rhashtable will help
> on keeping entry lists to a size, but still.
> 
I still think the rcu_read_lock would be sufficient here, if we just ensured
that removals from the list occured after a quiescent point.  The lookup is in
the datapath, but adds/removes can have a little more latency added to them, and
if it removes the atomic operation from the fast path, I think thats a net win.

Neil

>   Marcelo
> 

Re: [PATCH net] sctp: hold transport before accessing its asoc in sctp_hash_transport

[Marcelo Ricardo Leitner]

On Wed, Nov 21, 2018 at 08:27:21AM -0500, Neil Horman wrote:
> On Tue, Nov 20, 2018 at 10:46:26PM -0200, Marcelo Ricardo Leitner wrote:
> > On Tue, Nov 20, 2018 at 07:52:48AM -0500, Neil Horman wrote:
> > > On Tue, Nov 20, 2018 at 07:09:16PM +0800, Xin Long wrote:
> > > > In sctp_hash_transport, it dereferences a transport's asoc only under
> > > > rcu_read_lock. Without holding the transport, its asoc could be freed
> > > > already, which leads to a use-after-free panic.
> > > > 
> > > > A similar fix as Commit bab1be79a516 ("sctp: hold transport before
> > > > accessing its asoc in sctp_transport_get_next") is needed to hold
> > > > the transport before accessing its asoc in sctp_hash_transport.
> > > > 
> > > > Fixes: cd2b70875058 ("sctp: check duplicate node before inserting a new transport")
> > > > Reported-by: syzbot+0b05d8aa7cb185107483@syzkaller.appspotmail.com
> > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > > ---
> > > >  net/sctp/input.c | 7 ++++++-
> > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/net/sctp/input.c b/net/sctp/input.c
> > > > index 5c36a99..69584e9 100644
> > > > --- a/net/sctp/input.c
> > > > +++ b/net/sctp/input.c
> > > > @@ -896,11 +896,16 @@ int sctp_hash_transport(struct sctp_transport *t)
> > > >  	list = rhltable_lookup(&sctp_transport_hashtable, &arg,
> > > >  			       sctp_hash_params);
> > > >  
> > > > -	rhl_for_each_entry_rcu(transport, tmp, list, node)
> > > > +	rhl_for_each_entry_rcu(transport, tmp, list, node) {
> > > > +		if (!sctp_transport_hold(transport))
> > > > +			continue;
> > > >  		if (transport->asoc->ep = t->asoc->ep) {
> > > > +			sctp_transport_put(transport);
> > > >  			rcu_read_unlock();
> > > >  			return -EEXIST;
> > > >  		}
> > > > +		sctp_transport_put(transport);
> > > > +	}
> > > >  	rcu_read_unlock();
> > > >  
> > > >  	err = rhltable_insert_key(&sctp_transport_hashtable, &arg,
> > > > -- 
> > > > 2.1.0
> > > > 
> > > > 
> > > 
> > > something doesn't feel at all right about this.  If we are inserting a transport
> > > to an association, it would seem to me that we should have at least one user of
> > > the association (i.e. non-zero refcount).  As such it seems something is wrong
> > > with the association refcount here.  At the very least, if there is a case where
> > > an association is being removed while a transport is being added, the better
> > > solution would be to ensure that sctp_association_destroy goes through a
> > > quiescent point prior to unhashing transports from the list, to ensure that
> > > there is no conflict with the add operation above.
> > 
> > Consider that the rhl_for_each_entry_rcu() is traversing the global
> > rhashtable, and that it may operate on unrelated transports/asocs.
> > E.g., transport->asoc in the for() is potentially different from the
> > asoc under socket lock.
> > 
> Ah, ok, we're comparing associations that are not related to the association
> being searched for, that makes sense.
> 
> > The core of the fix is at:
> > +		if (!sctp_transport_hold(transport))
> > +			continue;
> > If we can get a hold, the asoc will be available for dereferencing in
> > subsequent lines. Otherwise, move on.
> > 
> > With that, the patch makes sense to me.
> > 
> Yes, I agree, but as you note below, this still seems like a lousy way to fix
> the problem.
> 
> > Although I would prefer if we come up with a better way to do this
> > jump, or even avoid the jump. We are only comparing pointers here and
> > if we had asoc->ep cached on sctp_transport itself, we could avoid the
> > atomics here.
> > 
> > This change, in the next patch on sctp_epaddr_lookup_transport, will
> > hurt performance as that is called in datapath. Rhashtable will help
> > on keeping entry lists to a size, but still.
> > 
> I still think the rcu_read_lock would be sufficient here, if we just ensured
> that removals from the list occured after a quiescent point.  The lookup is in

I'm not sure I follow.

> the datapath, but adds/removes can have a little more latency added to them, and
> if it removes the atomic operation from the fast path, I think thats a net win.

Agree.

[PATCH net] sctp: hold transport before accessing its asoc in sctp_epaddr_lookup_transport

[Xin Long]

Without holding transport to dereference its asoc, a use after
free panic can be caused in sctp_epaddr_lookup_transport. Note
that a sock lock can't protect these transports that belong to
other socks.

A similar fix as Commit bab1be79a516 ("sctp: hold transport
before accessing its asoc in sctp_transport_get_next") is
needed to hold the transport before accessing its asoc in
sctp_epaddr_lookup_transport.

Fixes: 7fda702f9315 ("sctp: use new rhlist interface on sctp transport rhashtable")
Reported-by: syzbot+aad231d51b1923158444@syzkaller.appspotmail.com
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/input.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/sctp/input.c b/net/sctp/input.c
index 69584e9..c2c0816 100644
--- a/net/sctp/input.c
+++ b/net/sctp/input.c
@@ -972,9 +972,15 @@ struct sctp_transport *sctp_epaddr_lookup_transport(
 	list = rhltable_lookup(&sctp_transport_hashtable, &arg,
 			       sctp_hash_params);
 
-	rhl_for_each_entry_rcu(t, tmp, list, node)
-		if (ep = t->asoc->ep)
+	rhl_for_each_entry_rcu(t, tmp, list, node) {
+		if (!sctp_transport_hold(t))
+			continue;
+		if (ep = t->asoc->ep) {
+			sctp_transport_put(t);
 			return t;
+		}
+		sctp_transport_put(t);
+	}
 
 	return NULL;
 }
-- 
2.1.0

Re: [PATCH net] sctp: hold transport before accessing its asoc in sctp_epaddr_lookup_transport

[Neil Horman]

On Tue, Nov 20, 2018 at 07:12:10PM +0800, Xin Long wrote:
> Without holding transport to dereference its asoc, a use after
> free panic can be caused in sctp_epaddr_lookup_transport. Note
> that a sock lock can't protect these transports that belong to
> other socks.
> 
> A similar fix as Commit bab1be79a516 ("sctp: hold transport
> before accessing its asoc in sctp_transport_get_next") is
> needed to hold the transport before accessing its asoc in
> sctp_epaddr_lookup_transport.
> 
> Fixes: 7fda702f9315 ("sctp: use new rhlist interface on sctp transport rhashtable")
> Reported-by: syzbot+aad231d51b1923158444@syzkaller.appspotmail.com
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/sctp/input.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/net/sctp/input.c b/net/sctp/input.c
> index 69584e9..c2c0816 100644
> --- a/net/sctp/input.c
> +++ b/net/sctp/input.c
> @@ -972,9 +972,15 @@ struct sctp_transport *sctp_epaddr_lookup_transport(
>  	list = rhltable_lookup(&sctp_transport_hashtable, &arg,
>  			       sctp_hash_params);
>  
> -	rhl_for_each_entry_rcu(t, tmp, list, node)
> -		if (ep = t->asoc->ep)
> +	rhl_for_each_entry_rcu(t, tmp, list, node) {
> +		if (!sctp_transport_hold(t))
> +			continue;
> +		if (ep = t->asoc->ep) {
> +			sctp_transport_put(t);
>  			return t;
> +		}
> +		sctp_transport_put(t);
> +	}
>  
>  	return NULL;
>  }
> -- 
> 2.1.0
> 
> 
Same as the other patch, it seems like we shouldn't need to hold the transport
here as long as we either have a hold on the parent association already (which
we should), and or, properly quiesce the destruction of an assocation.

Neil