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.leit...@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+0b05d8aa7cb185107...@syzkaller.appspotmail.com
> > > > Signed-off-by: Xin Long <lucien....@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.
