Hi,
I've got the following error report while running the syzkaller fuzzer:
==================================================================
BUG: KASAN: use-after-free in __sctp_connect+0xabe/0xbf0 at addr
ffff88006b1dc610
Read of size 4 by task syz-executor/21837
CPU: 2 PID: 21837 Comm: syz-executor Not tainted 4.9.0-rc1+ #228
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
ffff8800393ef930 ffffffff81b474f4 ffff88003e80ed40 ffff88006b1dc568
ffff88006b1dd6a0 ffff88006b1dc560 ffff8800393ef958 ffffffff8150b33c
ffff8800393ef9e8 ffff88003e80ed40 ffff8800eb1dc610 ffff8800393ef9d8
Call Trace:
[< inline >] __dump_stack lib/dump_stack.c:15
[<ffffffff81b474f4>] dump_stack+0xb3/0x10f lib/dump_stack.c:51
[<ffffffff8150b33c>] kasan_object_err+0x1c/0x70 mm/kasan/report.c:156
[< inline >] print_address_description mm/kasan/report.c:194
[<ffffffff8150b5d7>] kasan_report_error+0x1f7/0x4d0 mm/kasan/report.c:283
[< inline >] kasan_report mm/kasan/report.c:303
[<ffffffff8150b96e>] __asan_report_load4_noabort+0x3e/0x40
mm/kasan/report.c:323
[<ffffffff8393457e>] __sctp_connect+0xabe/0xbf0 net/sctp/socket.c:1219
[<ffffffff83934832>] __sctp_setsockopt_connectx+0x182/0x1b0
net/sctp/socket.c:1329
[< inline >] sctp_setsockopt_connectx net/sctp/socket.c:1361
[<ffffffff8393c1d9>] sctp_setsockopt+0x1009/0x3d70 net/sctp/socket.c:3813
[<ffffffff82b770d6>] sock_common_setsockopt+0x96/0xd0 net/core/sock.c:2688
[< inline >] SYSC_setsockopt net/socket.c:1742
[<ffffffff82b740b4>] SyS_setsockopt+0x154/0x240 net/socket.c:1721
[<ffffffff83fc0141>] entry_SYSCALL_64_fastpath+0x1f/0xc2
Object at ffff88006b1dc568, in cache kmalloc-4096 size: 4096
Allocated:
PID = 21837
[ 270.449111] [<ffffffff8107e2d6>] save_stack_trace+0x16/0x20
arch/x86/kernel/stacktrace.c:57
[ 270.449111] [<ffffffff8150a6a6>] save_stack+0x46/0xd0 mm/kasan/kasan.c:495
[ 270.449111] [< inline >] set_track mm/kasan/kasan.c:507
[ 270.449111] [<ffffffff8150a91b>] kasan_kmalloc+0xab/0xe0
mm/kasan/kasan.c:598
[ 270.449111] [<ffffffff8150604c>] kmem_cache_alloc_trace+0xec/0x270
mm/slub.c:2735
[ 270.449111] [< inline >] kmalloc include/linux/slab.h:490
[ 270.449111] [< inline >] kzalloc include/linux/slab.h:636
[ 270.449111] [<ffffffff838f2b2f>] sctp_association_new+0x6f/0x1f50
net/sctp/associola.c:303
[ 270.449111] [<ffffffff8393402a>] __sctp_connect+0x56a/0xbf0
net/sctp/socket.c:1163
[ 270.449111] [<ffffffff83934832>]
__sctp_setsockopt_connectx+0x182/0x1b0 net/sctp/socket.c:1329
[ 270.449111] [< inline >] sctp_setsockopt_connectx
net/sctp/socket.c:1361
[ 270.449111] [<ffffffff8393c1d9>] sctp_setsockopt+0x1009/0x3d70
net/sctp/socket.c:3813
[ 270.449111] [<ffffffff82b770d6>] sock_common_setsockopt+0x96/0xd0
net/core/sock.c:2688
[ 270.449111] [< inline >] SYSC_setsockopt net/socket.c:1742
[ 270.449111] [<ffffffff82b740b4>] SyS_setsockopt+0x154/0x240
net/socket.c:1721
[ 270.449111] [<ffffffff83fc0141>] entry_SYSCALL_64_fastpath+0x1f/0xc2
Freed:
PID = 21837
[ 270.449111] [<ffffffff8107e2d6>] save_stack_trace+0x16/0x20
arch/x86/kernel/stacktrace.c:57
[ 270.449111] [<ffffffff8150a6a6>] save_stack+0x46/0xd0 mm/kasan/kasan.c:495
[ 270.449111] [< inline >] set_track mm/kasan/kasan.c:507
[ 270.449111] [<ffffffff8150af03>] kasan_slab_free+0x73/0xc0
mm/kasan/kasan.c:571
[ 270.449111] [< inline >] slab_free_hook mm/slub.c:1352
[ 270.449111] [< inline >] slab_free_freelist_hook mm/slub.c:1374
[ 270.449111] [< inline >] slab_free mm/slub.c:2951
[ 270.449111] [<ffffffff815073e8>] kfree+0xe8/0x2b0 mm/slub.c:3871
[ 270.449111] [< inline >] sctp_association_destroy
net/sctp/associola.c:426
[ 270.449111] [<ffffffff838f56e5>] sctp_association_put+0x155/0x250
net/sctp/associola.c:866
[ 270.449111] [<ffffffff8392e213>] sctp_wait_for_connect+0x313/0x460
net/sctp/socket.c:7544
[ 270.449111] [<ffffffff8393443b>] __sctp_connect+0x97b/0xbf0
net/sctp/socket.c:1217
[ 270.449111] [<ffffffff83934832>]
__sctp_setsockopt_connectx+0x182/0x1b0 net/sctp/socket.c:1329
[ 270.449111] [< inline >] sctp_setsockopt_connectx
net/sctp/socket.c:1361
[ 270.449111] [<ffffffff8393c1d9>] sctp_setsockopt+0x1009/0x3d70
net/sctp/socket.c:3813
[ 270.449111] [<ffffffff82b770d6>] sock_common_setsockopt+0x96/0xd0
net/core/sock.c:2688
[ 270.449111] [< inline >] SYSC_setsockopt net/socket.c:1742
[ 270.449111] [<ffffffff82b740b4>] SyS_setsockopt+0x154/0x240
net/socket.c:1721
[ 270.449111] [<ffffffff83fc0141>] entry_SYSCALL_64_fastpath+0x1f/0xc2
Memory state around the buggy address:
ffff88006b1dc500: fc fc fc fc fc fc fc fc fc fc fc fc fc fb fb fb
ffff88006b1dc580: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>ffff88006b1dc600: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
^
ffff88006b1dc680: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff88006b1dc700: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================
sctp_wait_for_connect ends up freeing asoc, which is later accessed to
read asoc->assoc_id.
On commit 1a1891d762d6e64daf07b5be4817e3fbb29e3c59 (Oct 18).
On Wed, Oct 19, 2016 at 02:25:24PM +0200, Andrey Konovalov wrote:
> Hi,
>
> I've got the following error report while running the syzkaller fuzzer:
>
> ==================================================================
> BUG: KASAN: use-after-free in __sctp_connect+0xabe/0xbf0 at addr
> ffff88006b1dc610
Seems this is the same that Dmitry Vyukov had reported back in Jan 13th.
So far I couldn't identify the reason.
"Good" to know it's still there, thanks for reporting it.
On Wed, Oct 19, 2016 at 6:57 PM, Marcelo Ricardo Leitner
<[email protected]> wrote:
> On Wed, Oct 19, 2016 at 02:25:24PM +0200, Andrey Konovalov wrote:
>> Hi,
>>
>> I've got the following error report while running the syzkaller fuzzer:
>>
>> ==================================================================
>> BUG: KASAN: use-after-free in __sctp_connect+0xabe/0xbf0 at addr
>> ffff88006b1dc610
>
> Seems this is the same that Dmitry Vyukov had reported back in Jan 13th.
> So far I couldn't identify the reason.
> "Good" to know it's still there, thanks for reporting it.
Hi Marcelo,
I've attached a reproducer that might help to figure out the reason.
It triggers the UAF for me in ~10 seconds of running as:
$ gcc -lpthread sctp-connect-uaf-poc.c
$ while true; do ./a.out; done
You need to have KASAN enabled.
>
On Wed, Nov 2, 2016 at 11:42 PM, Andrey Konovalov <[email protected]> wrote:
> On Wed, Oct 19, 2016 at 6:57 PM, Marcelo Ricardo Leitner
> <[email protected]> wrote:
>> On Wed, Oct 19, 2016 at 02:25:24PM +0200, Andrey Konovalov wrote:
>>> Hi,
>>>
>>> I've got the following error report while running the syzkaller fuzzer:
>>>
>>> ==================================================================
>>> BUG: KASAN: use-after-free in __sctp_connect+0xabe/0xbf0 at addr
>>> ffff88006b1dc610
>>
>> Seems this is the same that Dmitry Vyukov had reported back in Jan 13th.
>> So far I couldn't identify the reason.
>> "Good" to know it's still there, thanks for reporting it.
Hi Marcelo,
So I've looked at the code.
As far as I understand, the problem is a race condition between
setsockopt(SCTP_SOCKOPT_CONNECTX) and shutdown on an sctp socket.
setsockopt() calls sctp_wait_for_connect(), which exits the for loop
on the sk->sk_shutdown & RCV_SHUTDOWN if clause, and then frees asoc
with sctp_association_put() and returns err = 0.
Then __sctp_connect() checks that err == 0 and reads asoc->assoc_id
from the freed asoc.
On Thu, Nov 03, 2016 at 06:11:01PM +0100, Andrey Konovalov wrote:
> On Wed, Nov 2, 2016 at 11:42 PM, Andrey Konovalov <[email protected]> wrote:
> > On Wed, Oct 19, 2016 at 6:57 PM, Marcelo Ricardo Leitner
> > <[email protected]> wrote:
> >> On Wed, Oct 19, 2016 at 02:25:24PM +0200, Andrey Konovalov wrote:
> >>> Hi,
> >>>
> >>> I've got the following error report while running the syzkaller fuzzer:
> >>>
> >>> ==================================================================
> >>> BUG: KASAN: use-after-free in __sctp_connect+0xabe/0xbf0 at addr
> >>> ffff88006b1dc610
> >>
> >> Seems this is the same that Dmitry Vyukov had reported back in Jan 13th.
> >> So far I couldn't identify the reason.
> >> "Good" to know it's still there, thanks for reporting it.
>
> Hi Marcelo,
>
Hi
> So I've looked at the code.
> As far as I understand, the problem is a race condition between
> setsockopt(SCTP_SOCKOPT_CONNECTX) and shutdown on an sctp socket.
> setsockopt() calls sctp_wait_for_connect(), which exits the for loop
> on the sk->sk_shutdown & RCV_SHUTDOWN if clause, and then frees asoc
> with sctp_association_put() and returns err = 0.
> Then __sctp_connect() checks that err == 0 and reads asoc->assoc_id
> from the freed asoc.
Suddenly this seems familiar. Your description makes sense, thanks for
looking deeper into this, Andrey.
This fix should do it, can you please try it? I'll post it properly
if it works.
wait_for_connect is only used in two places, we can move the ref to a
broader scope and cover that read too, instead of holding another ref.
sendmsg path won't read anything from the asoc after waiting, so this
should be enough for it too.
---8<---
commit 7f7ba9b4fb834a61ab097dfd7c1f267e6a6d70a8
Author: Marcelo Ricardo Leitner <[email protected]>
Date: Thu Nov 3 15:47:45 2016 -0200
sctp: hold the asoc longer when associating
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 9fbb6feb8c27..aac271571930 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -1214,9 +1214,11 @@ static int __sctp_connect(struct sock *sk,
timeo = sock_sndtimeo(sk, f_flags & O_NONBLOCK);
+ sctp_association_hold(asoc);
err = sctp_wait_for_connect(asoc, &timeo);
if ((err == 0 || err == -EINPROGRESS) && assoc_id)
*assoc_id = asoc->assoc_id;
+ sctp_association_put(asoc);
/* Don't free association on exit. */
asoc = NULL;
@@ -1985,7 +1987,9 @@ static int sctp_sendmsg(struct sock *sk, struct msghdr *msg, size_t msg_len)
if (unlikely(wait_connect)) {
timeo = sock_sndtimeo(sk, msg_flags & MSG_DONTWAIT);
+ sctp_association_hold(asoc);
sctp_wait_for_connect(asoc, &timeo);
+ sctp_association_put(asoc);
}
/* If we are already past ASSOCIATE, the lower
@@ -7501,6 +7505,7 @@ static int sctp_writeable(struct sock *sk)
/* Wait for an association to go into ESTABLISHED state. If timeout is 0,
* returns immediately with EINPROGRESS.
+ * Note: caller must hold a ref on asoc before calling this function.
*/
static int sctp_wait_for_connect(struct sctp_association *asoc, long *timeo_p)
{
@@ -7511,9 +7516,6 @@ static int sctp_wait_for_connect(struct sctp_association *asoc, long *timeo_p)
pr_debug("%s: asoc:%p, timeo:%ld\n", __func__, asoc, *timeo_p);
- /* Increment the association's refcnt. */
- sctp_association_hold(asoc);
-
for (;;) {
prepare_to_wait_exclusive(&asoc->wait, &wait,
TASK_INTERRUPTIBLE);
@@ -7543,9 +7545,6 @@ static int sctp_wait_for_connect(struct sctp_association *asoc, long *timeo_p)
out:
finish_wait(&asoc->wait, &wait);
- /* Release the association's refcnt. */
- sctp_association_put(asoc);
-
return err;
do_error:
On Thu, Nov 3, 2016 at 6:52 PM, Marcelo Ricardo Leitner
<[email protected]> wrote:
> On Thu, Nov 03, 2016 at 06:11:01PM +0100, Andrey Konovalov wrote:
>> On Wed, Nov 2, 2016 at 11:42 PM, Andrey Konovalov <[email protected]> wrote:
>> > On Wed, Oct 19, 2016 at 6:57 PM, Marcelo Ricardo Leitner
>> > <[email protected]> wrote:
>> >> On Wed, Oct 19, 2016 at 02:25:24PM +0200, Andrey Konovalov wrote:
>> >>> Hi,
>> >>>
>> >>> I've got the following error report while running the syzkaller fuzzer:
>> >>>
>> >>> ==================================================================
>> >>> BUG: KASAN: use-after-free in __sctp_connect+0xabe/0xbf0 at addr
>> >>> ffff88006b1dc610
>> >>
>> >> Seems this is the same that Dmitry Vyukov had reported back in Jan 13th.
>> >> So far I couldn't identify the reason.
>> >> "Good" to know it's still there, thanks for reporting it.
>>
>> Hi Marcelo,
>>
>
> Hi
>
>> So I've looked at the code.
>> As far as I understand, the problem is a race condition between
>> setsockopt(SCTP_SOCKOPT_CONNECTX) and shutdown on an sctp socket.
>> setsockopt() calls sctp_wait_for_connect(), which exits the for loop
>> on the sk->sk_shutdown & RCV_SHUTDOWN if clause, and then frees asoc
>> with sctp_association_put() and returns err = 0.
>> Then __sctp_connect() checks that err == 0 and reads asoc->assoc_id
>> from the freed asoc.
>
> Suddenly this seems familiar. Your description makes sense, thanks for
> looking deeper into this, Andrey.
>
> This fix should do it, can you please try it? I'll post it properly
> if it works.
Yes, it fixes the issue.
Tested-by: Andrey Konovalov <[email protected]>
Thanks for the fix!
>
> wait_for_connect is only used in two places, we can move the ref to a
> broader scope and cover that read too, instead of holding another ref.
>
> sendmsg path won't read anything from the asoc after waiting, so this
> should be enough for it too.
>
> ---8<---
>
> commit 7f7ba9b4fb834a61ab097dfd7c1f267e6a6d70a8
> Author: Marcelo Ricardo Leitner <[email protected]>
> Date: Thu Nov 3 15:47:45 2016 -0200
>
> sctp: hold the asoc longer when associating
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 9fbb6feb8c27..aac271571930 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -1214,9 +1214,11 @@ static int __sctp_connect(struct sock *sk,
>
> timeo = sock_sndtimeo(sk, f_flags & O_NONBLOCK);
>
> + sctp_association_hold(asoc);
> err = sctp_wait_for_connect(asoc, &timeo);
> if ((err == 0 || err == -EINPROGRESS) && assoc_id)
> *assoc_id = asoc->assoc_id;
> + sctp_association_put(asoc);
>
> /* Don't free association on exit. */
> asoc = NULL;
> @@ -1985,7 +1987,9 @@ static int sctp_sendmsg(struct sock *sk, struct msghdr *msg, size_t msg_len)
>
> if (unlikely(wait_connect)) {
> timeo = sock_sndtimeo(sk, msg_flags & MSG_DONTWAIT);
> + sctp_association_hold(asoc);
> sctp_wait_for_connect(asoc, &timeo);
> + sctp_association_put(asoc);
> }
>
> /* If we are already past ASSOCIATE, the lower
> @@ -7501,6 +7505,7 @@ static int sctp_writeable(struct sock *sk)
>
> /* Wait for an association to go into ESTABLISHED state. If timeout is 0,
> * returns immediately with EINPROGRESS.
> + * Note: caller must hold a ref on asoc before calling this function.
> */
> static int sctp_wait_for_connect(struct sctp_association *asoc, long *timeo_p)
> {
> @@ -7511,9 +7516,6 @@ static int sctp_wait_for_connect(struct sctp_association *asoc, long *timeo_p)
>
> pr_debug("%s: asoc:%p, timeo:%ld\n", __func__, asoc, *timeo_p);
>
> - /* Increment the association's refcnt. */
> - sctp_association_hold(asoc);
> -
> for (;;) {
> prepare_to_wait_exclusive(&asoc->wait, &wait,
> TASK_INTERRUPTIBLE);
> @@ -7543,9 +7545,6 @@ static int sctp_wait_for_connect(struct sctp_association *asoc, long *timeo_p)
> out:
> finish_wait(&asoc->wait, &wait);
>
> - /* Release the association's refcnt. */
> - sctp_association_put(asoc);
> -
> return err;
>
> do_error:
On Thu, Nov 03, 2016 at 07:02:47PM +0100, Andrey Konovalov wrote:
> On Thu, Nov 3, 2016 at 6:52 PM, Marcelo Ricardo Leitner
> <[email protected]> wrote:
> > On Thu, Nov 03, 2016 at 06:11:01PM +0100, Andrey Konovalov wrote:
> >> On Wed, Nov 2, 2016 at 11:42 PM, Andrey Konovalov <[email protected]> wrote:
> >> > On Wed, Oct 19, 2016 at 6:57 PM, Marcelo Ricardo Leitner
> >> > <[email protected]> wrote:
> >> >> On Wed, Oct 19, 2016 at 02:25:24PM +0200, Andrey Konovalov wrote:
> >> >>> Hi,
> >> >>>
> >> >>> I've got the following error report while running the syzkaller fuzzer:
> >> >>>
> >> >>> ==================================================================
> >> >>> BUG: KASAN: use-after-free in __sctp_connect+0xabe/0xbf0 at addr
> >> >>> ffff88006b1dc610
> >> >>
> >> >> Seems this is the same that Dmitry Vyukov had reported back in Jan 13th.
> >> >> So far I couldn't identify the reason.
> >> >> "Good" to know it's still there, thanks for reporting it.
> >>
> >> Hi Marcelo,
> >>
> >
> > Hi
> >
> >> So I've looked at the code.
> >> As far as I understand, the problem is a race condition between
> >> setsockopt(SCTP_SOCKOPT_CONNECTX) and shutdown on an sctp socket.
> >> setsockopt() calls sctp_wait_for_connect(), which exits the for loop
> >> on the sk->sk_shutdown & RCV_SHUTDOWN if clause, and then frees asoc
> >> with sctp_association_put() and returns err = 0.
> >> Then __sctp_connect() checks that err == 0 and reads asoc->assoc_id
> >> from the freed asoc.
> >
> > Suddenly this seems familiar. Your description makes sense, thanks for
> > looking deeper into this, Andrey.
> >
> > This fix should do it, can you please try it? I'll post it properly
> > if it works.
>
> Yes, it fixes the issue.
>
> Tested-by: Andrey Konovalov <[email protected]>
>
> Thanks for the fix!
Ahm this other fix looks better: do the read before calling
sctp_wait_for_connect() as that id won't change in this call and the
application shouldn't trust this number if an error is returned, so
there should be no issues by returning it in such situation.
Can you please confirm this one also works? Thanks!
---8<---
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 6cdc61c21438..be1d9bb98230 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;
On Thu, Nov 3, 2016 at 7:35 PM, Marcelo Ricardo Leitner
<[email protected]> wrote:
> On Thu, Nov 03, 2016 at 07:02:47PM +0100, Andrey Konovalov wrote:
>> On Thu, Nov 3, 2016 at 6:52 PM, Marcelo Ricardo Leitner
>> <[email protected]> wrote:
>> > On Thu, Nov 03, 2016 at 06:11:01PM +0100, Andrey Konovalov wrote:
>> >> On Wed, Nov 2, 2016 at 11:42 PM, Andrey Konovalov <[email protected]> wrote:
>> >> > On Wed, Oct 19, 2016 at 6:57 PM, Marcelo Ricardo Leitner
>> >> > <[email protected]> wrote:
>> >> >> On Wed, Oct 19, 2016 at 02:25:24PM +0200, Andrey Konovalov wrote:
>> >> >>> Hi,
>> >> >>>
>> >> >>> I've got the following error report while running the syzkaller fuzzer:
>> >> >>>
>> >> >>> ==================================================================
>> >> >>> BUG: KASAN: use-after-free in __sctp_connect+0xabe/0xbf0 at addr
>> >> >>> ffff88006b1dc610
>> >> >>
>> >> >> Seems this is the same that Dmitry Vyukov had reported back in Jan 13th.
>> >> >> So far I couldn't identify the reason.
>> >> >> "Good" to know it's still there, thanks for reporting it.
>> >>
>> >> Hi Marcelo,
>> >>
>> >
>> > Hi
>> >
>> >> So I've looked at the code.
>> >> As far as I understand, the problem is a race condition between
>> >> setsockopt(SCTP_SOCKOPT_CONNECTX) and shutdown on an sctp socket.
>> >> setsockopt() calls sctp_wait_for_connect(), which exits the for loop
>> >> on the sk->sk_shutdown & RCV_SHUTDOWN if clause, and then frees asoc
>> >> with sctp_association_put() and returns err = 0.
>> >> Then __sctp_connect() checks that err == 0 and reads asoc->assoc_id
>> >> from the freed asoc.
>> >
>> > Suddenly this seems familiar. Your description makes sense, thanks for
>> > looking deeper into this, Andrey.
>> >
>> > This fix should do it, can you please try it? I'll post it properly
>> > if it works.
>>
>> Yes, it fixes the issue.
>>
>> Tested-by: Andrey Konovalov <[email protected]>
>>
>> Thanks for the fix!
>
> Ahm this other fix looks better: do the read before calling
> sctp_wait_for_connect() as that id won't change in this call and the
> application shouldn't trust this number if an error is returned, so
> there should be no issues by returning it in such situation.
>
> Can you please confirm this one also works? Thanks!
Sure!
This one also works.
Tested-by: Andrey Konovalov <[email protected]>
>
> ---8<---
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 6cdc61c21438..be1d9bb98230 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;
On Thu, Nov 03, 2016 at 04:35:33PM -0200, Marcelo Ricardo Leitner wrote:
> On Thu, Nov 03, 2016 at 07:02:47PM +0100, Andrey Konovalov wrote:
> > On Thu, Nov 3, 2016 at 6:52 PM, Marcelo Ricardo Leitner
> > <[email protected]> wrote:
> > > On Thu, Nov 03, 2016 at 06:11:01PM +0100, Andrey Konovalov wrote:
> > >> On Wed, Nov 2, 2016 at 11:42 PM, Andrey Konovalov <[email protected]> wrote:
> > >> > On Wed, Oct 19, 2016 at 6:57 PM, Marcelo Ricardo Leitner
> > >> > <[email protected]> wrote:
> > >> >> On Wed, Oct 19, 2016 at 02:25:24PM +0200, Andrey Konovalov wrote:
> > >> >>> Hi,
> > >> >>>
> > >> >>> I've got the following error report while running the syzkaller fuzzer:
> > >> >>>
> > >> >>> ==================================================================
> > >> >>> BUG: KASAN: use-after-free in __sctp_connect+0xabe/0xbf0 at addr
> > >> >>> ffff88006b1dc610
> > >> >>
> > >> >> Seems this is the same that Dmitry Vyukov had reported back in Jan 13th.
> > >> >> So far I couldn't identify the reason.
> > >> >> "Good" to know it's still there, thanks for reporting it.
> > >>
> > >> Hi Marcelo,
> > >>
> > >
> > > Hi
> > >
> > >> So I've looked at the code.
> > >> As far as I understand, the problem is a race condition between
> > >> setsockopt(SCTP_SOCKOPT_CONNECTX) and shutdown on an sctp socket.
> > >> setsockopt() calls sctp_wait_for_connect(), which exits the for loop
> > >> on the sk->sk_shutdown & RCV_SHUTDOWN if clause, and then frees asoc
> > >> with sctp_association_put() and returns err = 0.
> > >> Then __sctp_connect() checks that err == 0 and reads asoc->assoc_id
> > >> from the freed asoc.
> > >
> > > Suddenly this seems familiar. Your description makes sense, thanks for
> > > looking deeper into this, Andrey.
> > >
> > > This fix should do it, can you please try it? I'll post it properly
> > > if it works.
> >
> > Yes, it fixes the issue.
> >
> > Tested-by: Andrey Konovalov <[email protected]>
> >
> > Thanks for the fix!
>
> Ahm this other fix looks better: do the read before calling
> sctp_wait_for_connect() as that id won't change in this call and the
> application shouldn't trust this number if an error is returned, so
> there should be no issues by returning it in such situation.
>
> Can you please confirm this one also works? Thanks!
>
> ---8<---
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 6cdc61c21438..be1d9bb98230 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;
>
Agreed, this version looks better. Please repost it with a proper changelog and
I'll ack it.
Neil
On Fri, Nov 04, 2016 at 08:59:58AM -0400, Neil Horman wrote:
> On Thu, Nov 03, 2016 at 04:35:33PM -0200, Marcelo Ricardo Leitner wrote:
> > On Thu, Nov 03, 2016 at 07:02:47PM +0100, Andrey Konovalov wrote:
> > > On Thu, Nov 3, 2016 at 6:52 PM, Marcelo Ricardo Leitner
> > > <[email protected]> wrote:
> > > > On Thu, Nov 03, 2016 at 06:11:01PM +0100, Andrey Konovalov wrote:
> > > >> On Wed, Nov 2, 2016 at 11:42 PM, Andrey Konovalov <[email protected]> wrote:
> > > >> > On Wed, Oct 19, 2016 at 6:57 PM, Marcelo Ricardo Leitner
> > > >> > <[email protected]> wrote:
> > > >> >> On Wed, Oct 19, 2016 at 02:25:24PM +0200, Andrey Konovalov wrote:
> > > >> >>> Hi,
> > > >> >>>
> > > >> >>> I've got the following error report while running the syzkaller fuzzer:
> > > >> >>>
> > > >> >>> ==================================================================
> > > >> >>> BUG: KASAN: use-after-free in __sctp_connect+0xabe/0xbf0 at addr
> > > >> >>> ffff88006b1dc610
> > > >> >>
> > > >> >> Seems this is the same that Dmitry Vyukov had reported back in Jan 13th.
> > > >> >> So far I couldn't identify the reason.
> > > >> >> "Good" to know it's still there, thanks for reporting it.
> > > >>
> > > >> Hi Marcelo,
> > > >>
> > > >
> > > > Hi
> > > >
> > > >> So I've looked at the code.
> > > >> As far as I understand, the problem is a race condition between
> > > >> setsockopt(SCTP_SOCKOPT_CONNECTX) and shutdown on an sctp socket.
> > > >> setsockopt() calls sctp_wait_for_connect(), which exits the for loop
> > > >> on the sk->sk_shutdown & RCV_SHUTDOWN if clause, and then frees asoc
> > > >> with sctp_association_put() and returns err = 0.
> > > >> Then __sctp_connect() checks that err == 0 and reads asoc->assoc_id
> > > >> from the freed asoc.
> > > >
> > > > Suddenly this seems familiar. Your description makes sense, thanks for
> > > > looking deeper into this, Andrey.
> > > >
> > > > This fix should do it, can you please try it? I'll post it properly
> > > > if it works.
> > >
> > > Yes, it fixes the issue.
> > >
> > > Tested-by: Andrey Konovalov <[email protected]>
> > >
> > > Thanks for the fix!
> >
> > Ahm this other fix looks better: do the read before calling
> > sctp_wait_for_connect() as that id won't change in this call and the
> > application shouldn't trust this number if an error is returned, so
> > there should be no issues by returning it in such situation.
> >
> > Can you please confirm this one also works? Thanks!
> >
> > ---8<---
> >
> > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > index 6cdc61c21438..be1d9bb98230 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;
> >
> Agreed, this version looks better. Please repost it with a proper changelog and
> I'll ack it.
> Neil
It already is. :-)
Please look for
Subject: [PATCH net] sctp: assign assoc_id earlier in __sctp_connect
Thanks,
Marcelo