| 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
| ||
|
Date: Fri, 31 Aug 2018 08:03:23 -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 Fri, Aug 31, 2018 at 03:09:05PM +0800, Xin Long wrote:
> On Wed, Aug 29, 2018 at 7:36 PM Neil Horman <nhorman@...driver.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@...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
> 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
> >
>
Powered by blists - more mailing lists