From mboxrd@z Thu Jan 1 00:00:00 1970 From: Neil Horman Subject: Re: [PATCH net] sctp: do not free asoc when it is already dead in sctp_sendmsg Date: Mon, 13 Nov 2017 09:46:53 -0500 Message-ID: <20171113144653.GA7876@hmswarspite.think-freely.org> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: network dev , linux-sctp@vger.kernel.org, davem@davemloft.net, Marcelo Ricardo Leitner , Dmitry Vyukov , syzkaller@googlegroups.com To: Xin Long Return-path: Received: from charlotte.tuxdriver.com ([70.61.120.58]:56953 "EHLO smtp.tuxdriver.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753287AbdKMOrY (ORCPT ); Mon, 13 Nov 2017 09:47:24 -0500 Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Mon, Nov 13, 2017 at 12:43:50PM +0800, Xin Long wrote: > Now in sctp_sendmsg sctp_wait_for_sndbuf could schedule out without > holding sock sk. It means the current asoc can be freed elsewhere, > like when receiving an abort packet. > > If the asoc is just created in sctp_sendmsg and sctp_wait_for_sndbuf > returns err, the asoc will be freed again due to new_asoc is not nil. > An use-after-free issue would be triggered by this. > > This patch is to fix it by setting new_asoc with nil if the asoc is > already dead when cpu schedules back, so that it will not be freed > again in sctp_sendmsg. > > Reported-by: Dmitry Vyukov > Signed-off-by: Xin Long > --- > net/sctp/socket.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > index 6f45d17..f575976 100644 > --- a/net/sctp/socket.c > +++ b/net/sctp/socket.c > @@ -83,8 +83,8 @@ > /* Forward declarations for internal helper functions. */ > static int sctp_writeable(struct sock *sk); > static void sctp_wfree(struct sk_buff *skb); > -static int sctp_wait_for_sndbuf(struct sctp_association *, long *timeo_p, > - size_t msg_len); > +static int sctp_wait_for_sndbuf(struct sctp_association *asoc, long *timeo_p, > + size_t msg_len, struct sctp_association **new); > static int sctp_wait_for_packet(struct sock *sk, int *err, long *timeo_p); > static int sctp_wait_for_connect(struct sctp_association *, long *timeo_p); > static int sctp_wait_for_accept(struct sock *sk, long timeo); > @@ -1962,7 +1962,7 @@ static int sctp_sendmsg(struct sock *sk, struct msghdr *msg, size_t msg_len) > > timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT); > if (!sctp_wspace(asoc)) { > - err = sctp_wait_for_sndbuf(asoc, &timeo, msg_len); > + err = sctp_wait_for_sndbuf(asoc, &timeo, msg_len, &new_asoc); > if (err) > goto out_free; > } > @@ -7822,7 +7822,7 @@ void sctp_sock_rfree(struct sk_buff *skb) > > /* Helper function to wait for space in the sndbuf. */ > static int sctp_wait_for_sndbuf(struct sctp_association *asoc, long *timeo_p, > - size_t msg_len) > + size_t msg_len, struct sctp_association **new) > { > struct sock *sk = asoc->base.sk; > int err = 0; > @@ -7839,10 +7839,13 @@ static int sctp_wait_for_sndbuf(struct sctp_association *asoc, long *timeo_p, > for (;;) { > prepare_to_wait_exclusive(&asoc->wait, &wait, > TASK_INTERRUPTIBLE); > + if (asoc->base.dead) { > + *new = NULL; > + goto do_error; > + } > if (!*timeo_p) > goto do_nonblock; > - if (sk->sk_err || asoc->state >= SCTP_STATE_SHUTDOWN_PENDING || > - asoc->base.dead) > + if (sk->sk_err || asoc->state >= SCTP_STATE_SHUTDOWN_PENDING) > goto do_error; > if (signal_pending(current)) > goto do_interrupted; > -- > 2.1.0 > > Why pass a pointer to a pointer into the wait function? It seems like you could just check the return code for err == -EPIPE and set the association to null in sctp_sendmsg. That would avoid passing another parameter, and cut down on some complexity here. Neil From mboxrd@z Thu Jan 1 00:00:00 1970 From: Neil Horman Date: Mon, 13 Nov 2017 14:46:53 +0000 Subject: Re: [PATCH net] sctp: do not free asoc when it is already dead in sctp_sendmsg Message-Id: <20171113144653.GA7876@hmswarspite.think-freely.org> List-Id: References: In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Xin Long Cc: network dev , linux-sctp@vger.kernel.org, davem@davemloft.net, Marcelo Ricardo Leitner , Dmitry Vyukov , syzkaller@googlegroups.com On Mon, Nov 13, 2017 at 12:43:50PM +0800, Xin Long wrote: > Now in sctp_sendmsg sctp_wait_for_sndbuf could schedule out without > holding sock sk. It means the current asoc can be freed elsewhere, > like when receiving an abort packet. > > If the asoc is just created in sctp_sendmsg and sctp_wait_for_sndbuf > returns err, the asoc will be freed again due to new_asoc is not nil. > An use-after-free issue would be triggered by this. > > This patch is to fix it by setting new_asoc with nil if the asoc is > already dead when cpu schedules back, so that it will not be freed > again in sctp_sendmsg. > > Reported-by: Dmitry Vyukov > Signed-off-by: Xin Long > --- > net/sctp/socket.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > index 6f45d17..f575976 100644 > --- a/net/sctp/socket.c > +++ b/net/sctp/socket.c > @@ -83,8 +83,8 @@ > /* Forward declarations for internal helper functions. */ > static int sctp_writeable(struct sock *sk); > static void sctp_wfree(struct sk_buff *skb); > -static int sctp_wait_for_sndbuf(struct sctp_association *, long *timeo_p, > - size_t msg_len); > +static int sctp_wait_for_sndbuf(struct sctp_association *asoc, long *timeo_p, > + size_t msg_len, struct sctp_association **new); > static int sctp_wait_for_packet(struct sock *sk, int *err, long *timeo_p); > static int sctp_wait_for_connect(struct sctp_association *, long *timeo_p); > static int sctp_wait_for_accept(struct sock *sk, long timeo); > @@ -1962,7 +1962,7 @@ static int sctp_sendmsg(struct sock *sk, struct msghdr *msg, size_t msg_len) > > timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT); > if (!sctp_wspace(asoc)) { > - err = sctp_wait_for_sndbuf(asoc, &timeo, msg_len); > + err = sctp_wait_for_sndbuf(asoc, &timeo, msg_len, &new_asoc); > if (err) > goto out_free; > } > @@ -7822,7 +7822,7 @@ void sctp_sock_rfree(struct sk_buff *skb) > > /* Helper function to wait for space in the sndbuf. */ > static int sctp_wait_for_sndbuf(struct sctp_association *asoc, long *timeo_p, > - size_t msg_len) > + size_t msg_len, struct sctp_association **new) > { > struct sock *sk = asoc->base.sk; > int err = 0; > @@ -7839,10 +7839,13 @@ static int sctp_wait_for_sndbuf(struct sctp_association *asoc, long *timeo_p, > for (;;) { > prepare_to_wait_exclusive(&asoc->wait, &wait, > TASK_INTERRUPTIBLE); > + if (asoc->base.dead) { > + *new = NULL; > + goto do_error; > + } > if (!*timeo_p) > goto do_nonblock; > - if (sk->sk_err || asoc->state >= SCTP_STATE_SHUTDOWN_PENDING || > - asoc->base.dead) > + if (sk->sk_err || asoc->state >= SCTP_STATE_SHUTDOWN_PENDING) > goto do_error; > if (signal_pending(current)) > goto do_interrupted; > -- > 2.1.0 > > Why pass a pointer to a pointer into the wait function? It seems like you could just check the return code for err = -EPIPE and set the association to null in sctp_sendmsg. That would avoid passing another parameter, and cut down on some complexity here. Neil