netdev - Re: [PATCH net] sctp: hold transport before accessing its asoc in sctp_transport_get

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:   Wed, 29 Aug 2018 07:35:32 -0400
From:   Neil Horman <nhorman@...driver.com>
To:     Xin Long <lucien.xin@...il.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: [PATCH net] sctp: hold transport before accessing its asoc in
 sctp_transport_get_next

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