* [PATCH net] sctp: assign assoc_id earlier in __sctp_connect
@ 2016-11-03 19:03 Marcelo Ricardo Leitner
2016-11-04 10:45 ` Xin Long
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Marcelo Ricardo Leitner @ 2016-11-03 19:03 UTC (permalink / raw)
To: netdev
Cc: linux-sctp, Vlad Yasevich, Neil Horman, syzkaller, kcc, glider,
edumazet, dvyukov, andreyknvl
sctp_wait_for_connect() currently already holds the asoc to keep it
alive during the sleep, in case another thread release it. But Andrey
Konovalov and Dmitry Vyukov reported an use-after-free in such
situation.
Problem is that __sctp_connect() doesn't get a ref on the asoc and will
do a read on the asoc after calling sctp_wait_for_connect(), but by then
another thread may have closed it and the _put on sctp_wait_for_connect
will actually release it, causing the use-after-free.
Fix is, instead of doing the read after waiting for the connect, do it
before so, and avoid this issue as the socket is still locked by then.
There should be no issue on returning the asoc id in case of failure as
the application shouldn't trust on that number in such situations
anyway.
This issue doesn't exist in sctp_sendmsg() path.
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Reported-by: Andrey Konovalov <andreyknvl@google.com>
Tested-by: Andrey Konovalov <andreyknvl@google.com>
Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---
net/sctp/socket.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 6cdc61c21438aa9b6dbdad93e70759071a4d6789..be1d9bb98230c9d77f676949db773b2dacd801a4 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -1214,9 +1214,12 @@ static int __sctp_connect(struct sock *sk,
timeo = sock_sndtimeo(sk, f_flags & O_NONBLOCK);
- err = sctp_wait_for_connect(asoc, &timeo);
- if ((err == 0 || err == -EINPROGRESS) && assoc_id)
+ if (assoc_id)
*assoc_id = asoc->assoc_id;
+ err = sctp_wait_for_connect(asoc, &timeo);
+ /* Note: the asoc may be freed after the return of
+ * sctp_wait_for_connect.
+ */
/* Don't free association on exit. */
asoc = NULL;
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net] sctp: assign assoc_id earlier in __sctp_connect
2016-11-03 19:03 [PATCH net] sctp: assign assoc_id earlier in __sctp_connect Marcelo Ricardo Leitner
@ 2016-11-04 10:45 ` Xin Long
2016-11-04 10:55 ` David Laight
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Xin Long @ 2016-11-04 10:45 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: network dev, linux-sctp, Vlad Yasevich, Neil Horman, syzkaller,
Kostya Serebryany, Alexander Potapenko, Eric Dumazet,
Dmitry Vyukov, Andrey Konovalov
On Fri, Nov 4, 2016 at 3:03 AM, Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
> sctp_wait_for_connect() currently already holds the asoc to keep it
> alive during the sleep, in case another thread release it. But Andrey
> Konovalov and Dmitry Vyukov reported an use-after-free in such
> situation.
>
> Problem is that __sctp_connect() doesn't get a ref on the asoc and will
> do a read on the asoc after calling sctp_wait_for_connect(), but by then
> another thread may have closed it and the _put on sctp_wait_for_connect
> will actually release it, causing the use-after-free.
>
> Fix is, instead of doing the read after waiting for the connect, do it
> before so, and avoid this issue as the socket is still locked by then.
> There should be no issue on returning the asoc id in case of failure as
> the application shouldn't trust on that number in such situations
> anyway.
>
> This issue doesn't exist in sctp_sendmsg() path.
>
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Reported-by: Andrey Konovalov <andreyknvl@google.com>
> Tested-by: Andrey Konovalov <andreyknvl@google.com>
> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Reviewed-by: Xin Long <lucien.xin@gmail.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH net] sctp: assign assoc_id earlier in __sctp_connect
2016-11-03 19:03 [PATCH net] sctp: assign assoc_id earlier in __sctp_connect Marcelo Ricardo Leitner
2016-11-04 10:45 ` Xin Long
@ 2016-11-04 10:55 ` David Laight
2016-11-04 11:10 ` Marcelo Ricardo Leitner
2016-11-04 13:28 ` Neil Horman
2016-11-07 18:19 ` David Miller
3 siblings, 1 reply; 6+ messages in thread
From: David Laight @ 2016-11-04 10:55 UTC (permalink / raw)
To: 'Marcelo Ricardo Leitner', netdev@vger.kernel.org
Cc: linux-sctp@vger.kernel.org, Vlad Yasevich, Neil Horman,
syzkaller@googlegroups.com, kcc@google.com, glider@google.com,
edumazet@google.com, dvyukov@google.com, andreyknvl@google.com
From: Of Marcelo Ricardo Leitner
> Sent: 03 November 2016 19:04
> sctp_wait_for_connect() currently already holds the asoc to keep it
> alive during the sleep, in case another thread release it. But Andrey
> Konovalov and Dmitry Vyukov reported an use-after-free in such
> situation.
>
> Problem is that __sctp_connect() doesn't get a ref on the asoc and will
> do a read on the asoc after calling sctp_wait_for_connect(), but by then
> another thread may have closed it and the _put on sctp_wait_for_connect
> will actually release it, causing the use-after-free.
>
> Fix is, instead of doing the read after waiting for the connect, do it
> before so, and avoid this issue as the socket is still locked by then.
> There should be no issue on returning the asoc id in case of failure as
> the application shouldn't trust on that number in such situations
> anyway.
>
> This issue doesn't exist in sctp_sendmsg() path.
>
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Reported-by: Andrey Konovalov <andreyknvl@google.com>
> Tested-by: Andrey Konovalov <andreyknvl@google.com>
> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> ---
> net/sctp/socket.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 6cdc61c21438aa9b6dbdad93e70759071a4d6789..be1d9bb98230c9d77f676949db773b2dacd801a4 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -1214,9 +1214,12 @@ static int __sctp_connect(struct sock *sk,
>
> timeo = sock_sndtimeo(sk, f_flags & O_NONBLOCK);
>
> - err = sctp_wait_for_connect(asoc, &timeo);
> - if ((err == 0 || err == -EINPROGRESS) && assoc_id)
> + if (assoc_id)
> *assoc_id = asoc->assoc_id;
> + err = sctp_wait_for_connect(asoc, &timeo);
> + /* Note: the asoc may be freed after the return of
> + * sctp_wait_for_connect.
> + */
Is it worth ensuring that *assoc_id is NULL on error?
Maybe change the code to:
assoc_id_val = asoc->assoc_id;
rval = sctp_wait_for_connect(asoc, &timeo);
if (err != 0 && err != -EINPROGRESS)
assoc_id_val = 0;
if (assoc_id)
*assoc_id = assoc_id_val;
David
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] sctp: assign assoc_id earlier in __sctp_connect
2016-11-04 10:55 ` David Laight
@ 2016-11-04 11:10 ` Marcelo Ricardo Leitner
0 siblings, 0 replies; 6+ messages in thread
From: Marcelo Ricardo Leitner @ 2016-11-04 11:10 UTC (permalink / raw)
To: David Laight, netdev@vger.kernel.org
Cc: linux-sctp@vger.kernel.org, Vlad Yasevich, Neil Horman,
syzkaller@googlegroups.com, kcc@google.com, glider@google.com,
edumazet@google.com, dvyukov@google.com, andreyknvl@google.com
Em 04-11-2016 08:55, David Laight escreveu:
> From: Of Marcelo Ricardo Leitner
>> Sent: 03 November 2016 19:04
>> sctp_wait_for_connect() currently already holds the asoc to keep it
>> alive during the sleep, in case another thread release it. But Andrey
>> Konovalov and Dmitry Vyukov reported an use-after-free in such
>> situation.
>>
>> Problem is that __sctp_connect() doesn't get a ref on the asoc and will
>> do a read on the asoc after calling sctp_wait_for_connect(), but by then
>> another thread may have closed it and the _put on sctp_wait_for_connect
>> will actually release it, causing the use-after-free.
>>
>> Fix is, instead of doing the read after waiting for the connect, do it
>> before so, and avoid this issue as the socket is still locked by then.
>> There should be no issue on returning the asoc id in case of failure as
>> the application shouldn't trust on that number in such situations
>> anyway.
>>
>> This issue doesn't exist in sctp_sendmsg() path.
>>
>> Reported-by: Dmitry Vyukov <dvyukov@google.com>
>> Reported-by: Andrey Konovalov <andreyknvl@google.com>
>> Tested-by: Andrey Konovalov <andreyknvl@google.com>
>> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
>> ---
>> net/sctp/socket.c | 7 +++++--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>> index 6cdc61c21438aa9b6dbdad93e70759071a4d6789..be1d9bb98230c9d77f676949db773b2dacd801a4 100644
>> --- a/net/sctp/socket.c
>> +++ b/net/sctp/socket.c
>> @@ -1214,9 +1214,12 @@ static int __sctp_connect(struct sock *sk,
>>
>> timeo = sock_sndtimeo(sk, f_flags & O_NONBLOCK);
>>
>> - err = sctp_wait_for_connect(asoc, &timeo);
>> - if ((err == 0 || err == -EINPROGRESS) && assoc_id)
>> + if (assoc_id)
>> *assoc_id = asoc->assoc_id;
>> + err = sctp_wait_for_connect(asoc, &timeo);
>> + /* Note: the asoc may be freed after the return of
>> + * sctp_wait_for_connect.
>> + */
>
> Is it worth ensuring that *assoc_id is NULL on error?
I don't think so. An error was returned, the value shouldn't be trusted
anyway and it's not leaking any sort of critical data.
Note that original code doesn't touch assoc_id in case of error. It's
different than zeroing it out.
> Maybe change the code to:
> assoc_id_val = asoc->assoc_id;
> rval = sctp_wait_for_connect(asoc, &timeo);
> if (err != 0 && err != -EINPROGRESS)
> assoc_id_val = 0;
> if (assoc_id)
> *assoc_id = assoc_id_val;
Or just clear it in case of error..
if (assoc_id && (err != 0 && err != -EINPROGRESS))
*assoc_id = 0;
Amount of code is probably the same but avoids a temporary var.
Marcelo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] sctp: assign assoc_id earlier in __sctp_connect
2016-11-03 19:03 [PATCH net] sctp: assign assoc_id earlier in __sctp_connect Marcelo Ricardo Leitner
2016-11-04 10:45 ` Xin Long
2016-11-04 10:55 ` David Laight
@ 2016-11-04 13:28 ` Neil Horman
2016-11-07 18:19 ` David Miller
3 siblings, 0 replies; 6+ messages in thread
From: Neil Horman @ 2016-11-04 13:28 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: netdev, linux-sctp, Vlad Yasevich, syzkaller, kcc, glider,
edumazet, dvyukov, andreyknvl
On Thu, Nov 03, 2016 at 05:03:41PM -0200, Marcelo Ricardo Leitner wrote:
> sctp_wait_for_connect() currently already holds the asoc to keep it
> alive during the sleep, in case another thread release it. But Andrey
> Konovalov and Dmitry Vyukov reported an use-after-free in such
> situation.
>
> Problem is that __sctp_connect() doesn't get a ref on the asoc and will
> do a read on the asoc after calling sctp_wait_for_connect(), but by then
> another thread may have closed it and the _put on sctp_wait_for_connect
> will actually release it, causing the use-after-free.
>
> Fix is, instead of doing the read after waiting for the connect, do it
> before so, and avoid this issue as the socket is still locked by then.
> There should be no issue on returning the asoc id in case of failure as
> the application shouldn't trust on that number in such situations
> anyway.
>
> This issue doesn't exist in sctp_sendmsg() path.
>
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Reported-by: Andrey Konovalov <andreyknvl@google.com>
> Tested-by: Andrey Konovalov <andreyknvl@google.com>
> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> ---
> net/sctp/socket.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 6cdc61c21438aa9b6dbdad93e70759071a4d6789..be1d9bb98230c9d77f676949db773b2dacd801a4 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -1214,9 +1214,12 @@ static int __sctp_connect(struct sock *sk,
>
> timeo = sock_sndtimeo(sk, f_flags & O_NONBLOCK);
>
> - err = sctp_wait_for_connect(asoc, &timeo);
> - if ((err == 0 || err == -EINPROGRESS) && assoc_id)
> + if (assoc_id)
> *assoc_id = asoc->assoc_id;
> + err = sctp_wait_for_connect(asoc, &timeo);
> + /* Note: the asoc may be freed after the return of
> + * sctp_wait_for_connect.
> + */
>
> /* Don't free association on exit. */
> asoc = NULL;
> --
> 2.7.4
>
>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] sctp: assign assoc_id earlier in __sctp_connect
2016-11-03 19:03 [PATCH net] sctp: assign assoc_id earlier in __sctp_connect Marcelo Ricardo Leitner
` (2 preceding siblings ...)
2016-11-04 13:28 ` Neil Horman
@ 2016-11-07 18:19 ` David Miller
3 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2016-11-07 18:19 UTC (permalink / raw)
To: marcelo.leitner
Cc: netdev, linux-sctp, vyasevich, nhorman, syzkaller, kcc, glider,
edumazet, dvyukov, andreyknvl
From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: Thu, 3 Nov 2016 17:03:41 -0200
> sctp_wait_for_connect() currently already holds the asoc to keep it
> alive during the sleep, in case another thread release it. But Andrey
> Konovalov and Dmitry Vyukov reported an use-after-free in such
> situation.
>
> Problem is that __sctp_connect() doesn't get a ref on the asoc and will
> do a read on the asoc after calling sctp_wait_for_connect(), but by then
> another thread may have closed it and the _put on sctp_wait_for_connect
> will actually release it, causing the use-after-free.
>
> Fix is, instead of doing the read after waiting for the connect, do it
> before so, and avoid this issue as the socket is still locked by then.
> There should be no issue on returning the asoc id in case of failure as
> the application shouldn't trust on that number in such situations
> anyway.
>
> This issue doesn't exist in sctp_sendmsg() path.
>
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Reported-by: Andrey Konovalov <andreyknvl@google.com>
> Tested-by: Andrey Konovalov <andreyknvl@google.com>
> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Applied and queued up for -stable, thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-11-07 18:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-11-03 19:03 [PATCH net] sctp: assign assoc_id earlier in __sctp_connect Marcelo Ricardo Leitner
2016-11-04 10:45 ` Xin Long
2016-11-04 10:55 ` David Laight
2016-11-04 11:10 ` Marcelo Ricardo Leitner
2016-11-04 13:28 ` Neil Horman
2016-11-07 18:19 ` 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).