* 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