From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> To: Xin Long <lucien.xin@gmail.com> Cc: network dev <netdev@vger.kernel.org>, linux-sctp@vger.kernel.org, davem@davemloft.net, Neil Horman <nhorman@tuxdriver.com> Subject: Re: [PATCH net] sctp: hold transport before accessing its asoc in sctp_transport_get_next Date: Mon, 27 Aug 2018 10:28:22 -0300 [thread overview] Message-ID: <20180827132822.GB4591@localhost.localdomain> (raw) In-Reply-To: <607dd2950d09fc83404d670a73099523087d4963.1535366311.git.lucien.xin@gmail.com> 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 >
WARNING: multiple messages have this Message-ID (diff)
From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> To: Xin Long <lucien.xin@gmail.com> Cc: network dev <netdev@vger.kernel.org>, linux-sctp@vger.kernel.org, davem@davemloft.net, Neil Horman <nhorman@tuxdriver.com> Subject: Re: [PATCH net] sctp: hold transport before accessing its asoc in sctp_transport_get_next Date: Mon, 27 Aug 2018 13:28:22 +0000 [thread overview] Message-ID: <20180827132822.GB4591@localhost.localdomain> (raw) In-Reply-To: <607dd2950d09fc83404d670a73099523087d4963.1535366311.git.lucien.xin@gmail.com> 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 >
next prev parent reply other threads:[~2018-08-27 17:15 UTC|newest] Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-08-27 10:38 [PATCH net] sctp: hold transport before accessing its asoc in sctp_transport_get_next Xin Long 2018-08-27 10:38 ` Xin Long 2018-08-27 13:08 ` Neil Horman 2018-08-27 13:08 ` Neil Horman 2018-08-28 16:08 ` Xin Long 2018-08-28 16:08 ` Xin Long 2018-08-29 11:35 ` Neil Horman 2018-08-29 11:35 ` Neil Horman 2018-08-31 7:09 ` Xin Long 2018-08-31 7:09 ` Xin Long 2018-08-31 12:03 ` Neil Horman 2018-08-31 12:03 ` Neil Horman 2018-09-03 13:03 ` Neil Horman 2018-09-03 13:03 ` Neil Horman 2018-08-27 13:28 ` Marcelo Ricardo Leitner [this message] 2018-08-27 13:28 ` Marcelo Ricardo Leitner 2018-08-27 22:13 ` David Miller 2018-08-27 22:13 ` David Miller 2018-11-20 11:09 [PATCH net] sctp: hold transport before accessing its asoc in sctp_hash_transport Xin Long 2018-11-20 11:09 ` Xin Long 2018-11-20 12:52 ` Neil Horman 2018-11-20 12:52 ` Neil Horman 2018-11-21 0:46 ` Marcelo Ricardo Leitner 2018-11-21 0:46 ` Marcelo Ricardo Leitner 2018-11-21 6:47 ` Xin Long 2018-11-21 6:47 ` Xin Long 2018-11-21 17:53 ` Marcelo Ricardo Leitner 2018-11-21 17:53 ` Marcelo Ricardo Leitner 2018-11-28 9:36 ` Xin Long 2018-11-28 9:36 ` Xin Long 2018-11-28 13:38 ` Marcelo Ricardo Leitner 2018-11-28 13:38 ` Marcelo Ricardo Leitner 2018-11-21 13:27 ` Neil Horman 2018-11-21 13:27 ` Neil Horman 2018-11-21 18:52 ` Marcelo Ricardo Leitner 2018-11-21 18:52 ` Marcelo Ricardo Leitner 2018-11-20 11:12 [PATCH net] sctp: hold transport before accessing its asoc in sctp_epaddr_lookup_transport Xin Long 2018-11-20 11:12 ` Xin Long 2018-11-20 14:00 ` Neil Horman 2018-11-20 14:00 ` Neil Horman
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20180827132822.GB4591@localhost.localdomain \ --to=marcelo.leitner@gmail.com \ --cc=davem@davemloft.net \ --cc=linux-sctp@vger.kernel.org \ --cc=lucien.xin@gmail.com \ --cc=netdev@vger.kernel.org \ --cc=nhorman@tuxdriver.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.