netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net] sctp: walk the list of asoc safely
@ 2019-02-01 14:15 Greg Kroah-Hartman
  2019-02-01 14:20 ` Marcelo Ricardo Leitner
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Greg Kroah-Hartman @ 2019-02-01 14:15 UTC (permalink / raw)
  To: Vlad Yasevich, Neil Horman, Marcelo Ricardo Leitner, linux-sctp, netdev

In sctp_sendmesg(), when walking the list of endpoint associations, the
association can be dropped from the list, making the list corrupt.
Properly handle this by using list_for_each_entry_safe()

Fixes: 4910280503f3 ("sctp: add support for snd flag SCTP_SENDALL process in sendmsg")
Reported-by: Secunia Research <vuln@secunia.com>
Tested-by: Secunia Research <vuln@secunia.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index f93c3cf9e567..65d6d04546ae 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -2027,7 +2027,7 @@ static int sctp_sendmsg(struct sock *sk, struct msghdr *msg, size_t msg_len)
 	struct sctp_endpoint *ep = sctp_sk(sk)->ep;
 	struct sctp_transport *transport = NULL;
 	struct sctp_sndrcvinfo _sinfo, *sinfo;
-	struct sctp_association *asoc;
+	struct sctp_association *asoc, *tmp;
 	struct sctp_cmsgs cmsgs;
 	union sctp_addr *daddr;
 	bool new = false;
@@ -2053,7 +2053,7 @@ static int sctp_sendmsg(struct sock *sk, struct msghdr *msg, size_t msg_len)
 
 	/* SCTP_SENDALL process */
 	if ((sflags & SCTP_SENDALL) && sctp_style(sk, UDP)) {
-		list_for_each_entry(asoc, &ep->asocs, asocs) {
+		list_for_each_entry_safe(asoc, tmp, &ep->asocs, asocs) {
 			err = sctp_sendmsg_check_sflags(asoc, sflags, msg,
 							msg_len);
 			if (err == 0)

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH net] sctp: walk the list of asoc safely
  2019-02-01 14:15 [PATCH net] sctp: walk the list of asoc safely Greg Kroah-Hartman
@ 2019-02-01 14:20 ` Marcelo Ricardo Leitner
  2019-02-01 14:43   ` Greg Kroah-Hartman
  2019-02-01 15:55 ` Neil Horman
  2019-02-01 19:04 ` David Miller
  2 siblings, 1 reply; 6+ messages in thread
From: Marcelo Ricardo Leitner @ 2019-02-01 14:20 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: Vlad Yasevich, Neil Horman, linux-sctp, netdev, vuln

On Fri, Feb 01, 2019 at 03:15:22PM +0100, Greg Kroah-Hartman wrote:
> In sctp_sendmesg(), when walking the list of endpoint associations, the
> association can be dropped from the list, making the list corrupt.
> Properly handle this by using list_for_each_entry_safe()
> 
> Fixes: 4910280503f3 ("sctp: add support for snd flag SCTP_SENDALL process in sendmsg")
> Reported-by: Secunia Research <vuln@secunia.com>
> Tested-by: Secunia Research <vuln@secunia.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index f93c3cf9e567..65d6d04546ae 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -2027,7 +2027,7 @@ static int sctp_sendmsg(struct sock *sk, struct msghdr *msg, size_t msg_len)
>  	struct sctp_endpoint *ep = sctp_sk(sk)->ep;
>  	struct sctp_transport *transport = NULL;
>  	struct sctp_sndrcvinfo _sinfo, *sinfo;
> -	struct sctp_association *asoc;
> +	struct sctp_association *asoc, *tmp;
>  	struct sctp_cmsgs cmsgs;
>  	union sctp_addr *daddr;
>  	bool new = false;
> @@ -2053,7 +2053,7 @@ static int sctp_sendmsg(struct sock *sk, struct msghdr *msg, size_t msg_len)

Extending the context here by 1 line:
        lock_sock(sk);
>  
>  	/* SCTP_SENDALL process */
>  	if ((sflags & SCTP_SENDALL) && sctp_style(sk, UDP)) {
> -		list_for_each_entry(asoc, &ep->asocs, asocs) {
> +		list_for_each_entry_safe(asoc, tmp, &ep->asocs, asocs) {

With the socket being locked by here, how can an asoc be removed
while the list is being traversed? The socket lock should be
protecting from it.

>  			err = sctp_sendmsg_check_sflags(asoc, sflags, msg,
>  							msg_len);
>  			if (err == 0)

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH net] sctp: walk the list of asoc safely
  2019-02-01 14:20 ` Marcelo Ricardo Leitner
@ 2019-02-01 14:43   ` Greg Kroah-Hartman
  2019-02-01 14:58     ` Marcelo Ricardo Leitner
  0 siblings, 1 reply; 6+ messages in thread
From: Greg Kroah-Hartman @ 2019-02-01 14:43 UTC (permalink / raw)
  To: Marcelo Ricardo Leitner
  Cc: Vlad Yasevich, Neil Horman, linux-sctp, netdev, vuln

On Fri, Feb 01, 2019 at 12:20:37PM -0200, Marcelo Ricardo Leitner wrote:
> On Fri, Feb 01, 2019 at 03:15:22PM +0100, Greg Kroah-Hartman wrote:
> > In sctp_sendmesg(), when walking the list of endpoint associations, the
> > association can be dropped from the list, making the list corrupt.
> > Properly handle this by using list_for_each_entry_safe()
> > 
> > Fixes: 4910280503f3 ("sctp: add support for snd flag SCTP_SENDALL process in sendmsg")
> > Reported-by: Secunia Research <vuln@secunia.com>
> > Tested-by: Secunia Research <vuln@secunia.com>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > 
> > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > index f93c3cf9e567..65d6d04546ae 100644
> > --- a/net/sctp/socket.c
> > +++ b/net/sctp/socket.c
> > @@ -2027,7 +2027,7 @@ static int sctp_sendmsg(struct sock *sk, struct msghdr *msg, size_t msg_len)
> >  	struct sctp_endpoint *ep = sctp_sk(sk)->ep;
> >  	struct sctp_transport *transport = NULL;
> >  	struct sctp_sndrcvinfo _sinfo, *sinfo;
> > -	struct sctp_association *asoc;
> > +	struct sctp_association *asoc, *tmp;
> >  	struct sctp_cmsgs cmsgs;
> >  	union sctp_addr *daddr;
> >  	bool new = false;
> > @@ -2053,7 +2053,7 @@ static int sctp_sendmsg(struct sock *sk, struct msghdr *msg, size_t msg_len)
> 
> Extending the context here by 1 line:
>         lock_sock(sk);
> >  
> >  	/* SCTP_SENDALL process */
> >  	if ((sflags & SCTP_SENDALL) && sctp_style(sk, UDP)) {
> > -		list_for_each_entry(asoc, &ep->asocs, asocs) {
> > +		list_for_each_entry_safe(asoc, tmp, &ep->asocs, asocs) {
> 
> With the socket being locked by here, how can an asoc be removed
> while the list is being traversed? The socket lock should be
> protecting from it.

What about when the SCTP_ABORT flag is set with SCTP_SENDALL at the same
time?

:(


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH net] sctp: walk the list of asoc safely
  2019-02-01 14:43   ` Greg Kroah-Hartman
@ 2019-02-01 14:58     ` Marcelo Ricardo Leitner
  0 siblings, 0 replies; 6+ messages in thread
From: Marcelo Ricardo Leitner @ 2019-02-01 14:58 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: Vlad Yasevich, Neil Horman, linux-sctp, netdev, vuln

On Fri, Feb 01, 2019 at 03:43:59PM +0100, Greg Kroah-Hartman wrote:
> On Fri, Feb 01, 2019 at 12:20:37PM -0200, Marcelo Ricardo Leitner wrote:
> > On Fri, Feb 01, 2019 at 03:15:22PM +0100, Greg Kroah-Hartman wrote:
> > > In sctp_sendmesg(), when walking the list of endpoint associations, the
> > > association can be dropped from the list, making the list corrupt.
> > > Properly handle this by using list_for_each_entry_safe()
> > > 
> > > Fixes: 4910280503f3 ("sctp: add support for snd flag SCTP_SENDALL process in sendmsg")
> > > Reported-by: Secunia Research <vuln@secunia.com>
> > > Tested-by: Secunia Research <vuln@secunia.com>
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > 
> > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > > index f93c3cf9e567..65d6d04546ae 100644
> > > --- a/net/sctp/socket.c
> > > +++ b/net/sctp/socket.c
> > > @@ -2027,7 +2027,7 @@ static int sctp_sendmsg(struct sock *sk, struct msghdr *msg, size_t msg_len)
> > >  	struct sctp_endpoint *ep = sctp_sk(sk)->ep;
> > >  	struct sctp_transport *transport = NULL;
> > >  	struct sctp_sndrcvinfo _sinfo, *sinfo;
> > > -	struct sctp_association *asoc;
> > > +	struct sctp_association *asoc, *tmp;
> > >  	struct sctp_cmsgs cmsgs;
> > >  	union sctp_addr *daddr;
> > >  	bool new = false;
> > > @@ -2053,7 +2053,7 @@ static int sctp_sendmsg(struct sock *sk, struct msghdr *msg, size_t msg_len)
> > 
> > Extending the context here by 1 line:
> >         lock_sock(sk);
> > >  
> > >  	/* SCTP_SENDALL process */
> > >  	if ((sflags & SCTP_SENDALL) && sctp_style(sk, UDP)) {
> > > -		list_for_each_entry(asoc, &ep->asocs, asocs) {
> > > +		list_for_each_entry_safe(asoc, tmp, &ep->asocs, asocs) {
> > 
> > With the socket being locked by here, how can an asoc be removed
> > while the list is being traversed? The socket lock should be
> > protecting from it.
> 
> What about when the SCTP_ABORT flag is set with SCTP_SENDALL at the same
> time?
> 
> :(

Good point! Thanks.
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH net] sctp: walk the list of asoc safely
  2019-02-01 14:15 [PATCH net] sctp: walk the list of asoc safely Greg Kroah-Hartman
  2019-02-01 14:20 ` Marcelo Ricardo Leitner
@ 2019-02-01 15:55 ` Neil Horman
  2019-02-01 19:04 ` David Miller
  2 siblings, 0 replies; 6+ messages in thread
From: Neil Horman @ 2019-02-01 15:55 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Vlad Yasevich, Marcelo Ricardo Leitner, linux-sctp, netdev

On Fri, Feb 01, 2019 at 03:15:22PM +0100, Greg Kroah-Hartman wrote:
> In sctp_sendmesg(), when walking the list of endpoint associations, the
> association can be dropped from the list, making the list corrupt.
> Properly handle this by using list_for_each_entry_safe()
> 
> Fixes: 4910280503f3 ("sctp: add support for snd flag SCTP_SENDALL process in sendmsg")
> Reported-by: Secunia Research <vuln@secunia.com>
> Tested-by: Secunia Research <vuln@secunia.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index f93c3cf9e567..65d6d04546ae 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -2027,7 +2027,7 @@ static int sctp_sendmsg(struct sock *sk, struct msghdr *msg, size_t msg_len)
>  	struct sctp_endpoint *ep = sctp_sk(sk)->ep;
>  	struct sctp_transport *transport = NULL;
>  	struct sctp_sndrcvinfo _sinfo, *sinfo;
> -	struct sctp_association *asoc;
> +	struct sctp_association *asoc, *tmp;
>  	struct sctp_cmsgs cmsgs;
>  	union sctp_addr *daddr;
>  	bool new = false;
> @@ -2053,7 +2053,7 @@ static int sctp_sendmsg(struct sock *sk, struct msghdr *msg, size_t msg_len)
>  
>  	/* SCTP_SENDALL process */
>  	if ((sflags & SCTP_SENDALL) && sctp_style(sk, UDP)) {
> -		list_for_each_entry(asoc, &ep->asocs, asocs) {
> +		list_for_each_entry_safe(asoc, tmp, &ep->asocs, asocs) {
>  			err = sctp_sendmsg_check_sflags(asoc, sflags, msg,
>  							msg_len);
>  			if (err == 0)
> 
Acked-by: Neil Horman <nhorman@tuxdriver.com>


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH net] sctp: walk the list of asoc safely
  2019-02-01 14:15 [PATCH net] sctp: walk the list of asoc safely Greg Kroah-Hartman
  2019-02-01 14:20 ` Marcelo Ricardo Leitner
  2019-02-01 15:55 ` Neil Horman
@ 2019-02-01 19:04 ` David Miller
  2 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2019-02-01 19:04 UTC (permalink / raw)
  To: gregkh; +Cc: vyasevich, nhorman, marcelo.leitner, linux-sctp, netdev

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Date: Fri, 1 Feb 2019 15:15:22 +0100

> In sctp_sendmesg(), when walking the list of endpoint associations, the
> association can be dropped from the list, making the list corrupt.
> Properly handle this by using list_for_each_entry_safe()
> 
> Fixes: 4910280503f3 ("sctp: add support for snd flag SCTP_SENDALL process in sendmsg")
> Reported-by: Secunia Research <vuln@secunia.com>
> Tested-by: Secunia Research <vuln@secunia.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Applied and queued up for -stable.

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-02-01 19:04 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-01 14:15 [PATCH net] sctp: walk the list of asoc safely Greg Kroah-Hartman
2019-02-01 14:20 ` Marcelo Ricardo Leitner
2019-02-01 14:43   ` Greg Kroah-Hartman
2019-02-01 14:58     ` Marcelo Ricardo Leitner
2019-02-01 15:55 ` Neil Horman
2019-02-01 19:04 ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).