netdev - Re: [PATCHv2 net] sctp: hold transport before accessing its asoc in sctp_epaddr_lookup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 30 Nov 2018 14:04:16 +0900
From:   Xin Long <lucien.xin@...il.com>
To:     Neil Horman <nhorman@...driver.com>
Cc:     network dev <netdev@...r.kernel.org>, linux-sctp@...r.kernel.org,
        davem <davem@...emloft.net>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Subject: Re: [PATCHv2 net] sctp: hold transport before accessing its asoc in sctp_epaddr_lookup_transport

On Fri, Nov 30, 2018 at 5:52 AM Neil Horman <nhorman@...driver.com> wrote:
>
> On Thu, Nov 29, 2018 at 02:44:07PM +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.
> >
> > Note that this extra atomic operation is on the datapath,
> > but as rhlist keeps the lists to a small size, it won't
> > see a noticeable performance hurt.
> >
> > v1->v2:
> >   - improve the changelog.
> >
> > Fixes: 7fda702f9315 ("sctp: use new rhlist interface on sctp transport rhashtable")
> > Reported-by: syzbot+aad231d51b1923158444@...kaller.appspotmail.com
> > Signed-off-by: Xin Long <lucien.xin@...il.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 5c36a99..ce7351c 100644
> > --- a/net/sctp/input.c
> > +++ b/net/sctp/input.c
> > @@ -967,9 +967,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;
> >  }
>
> Wait a second, what if we just added an rcu_head to the association structure
> and changed the kfree call in sctp_association_destroy to a kfree_rcu call
> instead?  That would force the actual freeing of the association to pass through
> a grace period, during which any in flight list traversal in
> sctp_epaddr_lookup_transport could complete safely.  Its another two pointers
We discussed this in last thread:
https://www.spinics.net/lists/netdev/msg535191.html

It will cause closed sk to linger longer.

> worth of space in the association, but I think that would be a worthwhile
> tradeoff for not having to do N atomic adds/puts every time you wanted to
> receive or send a frame.
N is not a big value, as rhlist itself keeps lists in a size.

>
> Neil
>
> > --
> > 2.1.0
> >
> >