Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1616235 > unrolled thread

Re: net/sctp: list double add warning in sctp_endpoint_add_asoc

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: net/sctp: list double add warning in sctp_endpoint_add_asoc Xin Long <lucien.xin@gmail.com> - 2017-04-04 19:30 +0200
    Re: net/sctp: list double add warning in sctp_endpoint_add_asoc Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> - 2017-04-04 23:20 +0200
      Re: net/sctp: list double add warning in sctp_endpoint_add_asoc Xin Long <lucien.xin@gmail.com> - 2017-04-05 13:00 +0200
        Re: net/sctp: list double add warning in sctp_endpoint_add_asoc Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> - 2017-04-05 14:50 +0200
          Re: net/sctp: list double add warning in sctp_endpoint_add_asoc Andrey Konovalov <andreyknvl@google.com> - 2017-04-05 16:10 +0200
      Re: net/sctp: list double add warning in sctp_endpoint_add_asoc Andrey Konovalov <andreyknvl@google.com> - 2017-04-05 16:10 +0200
        Re: net/sctp: list double add warning in sctp_endpoint_add_asoc Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> - 2017-04-05 16:30 +0200

#1616235 — Re: net/sctp: list double add warning in sctp_endpoint_add_asoc

On Tue, Apr 4, 2017 at 9:28 PM, Andrey Konovalov <andreyknvl@google.com> wrote:
> Hi,
>
> I've got the following error report while fuzzing the kernel with syzkaller.
>
> On commit a71c9a1c779f2499fb2afc0553e543f18aff6edf (4.11-rc5).
>
> A reproducer and .config are attached.
The script is pretty hard to reproduce the issue in my env.
But there seems a case to cause a use-after-free when out of snd_buf.

the case is like:
-----------
one thread:                       another thread:
                                  sctp_rcv hold asoc (hold transport)
                                  enqueue the chunk to backlog queue
                                  [refcnt=2]

sctp_close free assoc
[refcnt=1]

sctp_sendmsg find asoc
but not hold it

out of snd_buf
hold asoc, schedule out
[refcnt = 2]

                                  process backlog and put asoc/transport
                                  [refcnt=1]

schedule in, put asoc
[refcnt=0] <--- destroyed

sctp_sendmsg continue
using asoc, panic

--------------------

Maybe we should check if asoc is dead already when schedule back
into sctp_sendmsg because of out of snd_buf.

[toc] | [next] | [standalone]

#1616383

On Wed, Apr 05, 2017 at 01:29:19AM +0800, Xin Long wrote:
> On Tue, Apr 4, 2017 at 9:28 PM, Andrey Konovalov <andreyknvl@google.com> wrote:
> > Hi,
> >
> > I've got the following error report while fuzzing the kernel with syzkaller.
> >
> > On commit a71c9a1c779f2499fb2afc0553e543f18aff6edf (4.11-rc5).
> >
> > A reproducer and .config are attached.
> The script is pretty hard to reproduce the issue in my env.

I didn't try running it but I also found the reproducer very complicated
to follow. Do you have any plans on having some PoC optimizer, so we can
have a more readable code?
strace is handy for filtering the noise, yes, but sometimes it doesn't
cut it.

> But there seems a case to cause a use-after-free when out of snd_buf.
> 
> the case is like:
> -----------
> one thread:                       another thread:
>                                   sctp_rcv hold asoc (hold transport)
>                                   enqueue the chunk to backlog queue
>                                   [refcnt=2]
> 
> sctp_close free assoc
> [refcnt=1]
> 
> sctp_sendmsg find asoc
> but not hold it
> 
> out of snd_buf
> hold asoc, schedule out
> [refcnt = 2]
> 
>                                   process backlog and put asoc/transport
>                                   [refcnt=1]
> 
> schedule in, put asoc
> [refcnt=0] <--- destroyed
> 
> sctp_sendmsg continue

It shouldn't be continuing here because sctp_wait_for_sndbuf and
sctp_wait_for_connect functions are checking if the asoc is dead
already when it schedules in, even though sctp_wait_for_connect return
value is ignored and sctp_sendmsg() simply returns after that.
Or the checks for dead asocs in there aren't enough somehow.

> using asoc, panic

[toc] | [prev] | [next] | [standalone]

#1616837

On Wed, Apr 5, 2017 at 5:14 AM, Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
> On Wed, Apr 05, 2017 at 01:29:19AM +0800, Xin Long wrote:
>> On Tue, Apr 4, 2017 at 9:28 PM, Andrey Konovalov <andreyknvl@google.com> wrote:
>> > Hi,
>> >
>> > I've got the following error report while fuzzing the kernel with syzkaller.
>> >
>> > On commit a71c9a1c779f2499fb2afc0553e543f18aff6edf (4.11-rc5).
>> >
>> > A reproducer and .config are attached.
>> The script is pretty hard to reproduce the issue in my env.
>
> I didn't try running it but I also found the reproducer very complicated
> to follow. Do you have any plans on having some PoC optimizer, so we can
> have a more readable code?
> strace is handy for filtering the noise, yes, but sometimes it doesn't
> cut it.
I got the script now:
1. create sk
2. set sk->sndbuf = x
3. sendmsg with size s1 (s1 < x)
4. sendmsg with size s2 (s1+s2 > x)
5. sendmsg with size s3 (wspace < 0), wait sndbuf, schedule out.
6. listen sk (abnormal operation on sctp client)
7. accept sk.

In step 6, sk->sk_state = listening, then step 7 could get the first asoc
from ep->asoc_list and alloc a new sk2, attach the asoc to sk2.

after a while, sendmsg schedule in, but asoc->sk is sk2, !=sk.
the same issue we fix for peeloff on commit dfcb9f4f99f1 ("sctp: deny
peeloff operation on asocs with threads sleeping on it") happens.

But we should not fix it by the same way as for peeloff. the real reason
causes this issue is on step 6, it should disallow listen on the established sk.

The following fix should work for this, just similar with what
inet_listen() did.

@@ -7174,6 +7175,9 @@ int sctp_inet_listen(struct socket *sock, int backlog)
        if (sock->state != SS_UNCONNECTED)
                goto out;

+       if (!sctp_sstate(sk, LISTENING) && !sctp_sstate(sk,CLOSED))
+               goto out;
+

what do you think ?

[toc] | [prev] | [next] | [standalone]

#1616901

On Wed, Apr 05, 2017 at 06:48:45PM +0800, Xin Long wrote:
> On Wed, Apr 5, 2017 at 5:14 AM, Marcelo Ricardo Leitner
> <marcelo.leitner@gmail.com> wrote:
> > On Wed, Apr 05, 2017 at 01:29:19AM +0800, Xin Long wrote:
> >> On Tue, Apr 4, 2017 at 9:28 PM, Andrey Konovalov <andreyknvl@google.com> wrote:
> >> > Hi,
> >> >
> >> > I've got the following error report while fuzzing the kernel with syzkaller.
> >> >
> >> > On commit a71c9a1c779f2499fb2afc0553e543f18aff6edf (4.11-rc5).
> >> >
> >> > A reproducer and .config are attached.
> >> The script is pretty hard to reproduce the issue in my env.
> >
> > I didn't try running it but I also found the reproducer very complicated
> > to follow. Do you have any plans on having some PoC optimizer, so we can
> > have a more readable code?
> > strace is handy for filtering the noise, yes, but sometimes it doesn't
> > cut it.
> I got the script now:
> 1. create sk
> 2. set sk->sndbuf = x
> 3. sendmsg with size s1 (s1 < x)
> 4. sendmsg with size s2 (s1+s2 > x)
> 5. sendmsg with size s3 (wspace < 0), wait sndbuf, schedule out.
> 6. listen sk (abnormal operation on sctp client)
> 7. accept sk.
> 
> In step 6, sk->sk_state = listening, then step 7 could get the first asoc
> from ep->asoc_list and alloc a new sk2, attach the asoc to sk2.
> 
> after a while, sendmsg schedule in, but asoc->sk is sk2, !=sk.
> the same issue we fix for peeloff on commit dfcb9f4f99f1 ("sctp: deny
> peeloff operation on asocs with threads sleeping on it") happens.

Yes. That explains why the asoc isn't dead by when sendmsg comes back,
and avoid that dead check.

> 
> But we should not fix it by the same way as for peeloff. the real reason
> causes this issue is on step 6, it should disallow listen on the established sk.

Agreed.

> 
> The following fix should work for this, just similar with what
> inet_listen() did.
> 
> @@ -7174,6 +7175,9 @@ int sctp_inet_listen(struct socket *sock, int backlog)
>         if (sock->state != SS_UNCONNECTED)
>                 goto out;
> 
> +       if (!sctp_sstate(sk, LISTENING) && !sctp_sstate(sk,CLOSED))
> +               goto out;
> +
> 
> what do you think ?

Yes, agreed.
Thanks!

  Marcelo

[toc] | [prev] | [next] | [standalone]

#1616976

On Wed, Apr 5, 2017 at 2:44 PM, Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
> On Wed, Apr 05, 2017 at 06:48:45PM +0800, Xin Long wrote:
>> On Wed, Apr 5, 2017 at 5:14 AM, Marcelo Ricardo Leitner
>> <marcelo.leitner@gmail.com> wrote:
>> > On Wed, Apr 05, 2017 at 01:29:19AM +0800, Xin Long wrote:
>> >> On Tue, Apr 4, 2017 at 9:28 PM, Andrey Konovalov <andreyknvl@google.com> wrote:
>> >> > Hi,
>> >> >
>> >> > I've got the following error report while fuzzing the kernel with syzkaller.
>> >> >
>> >> > On commit a71c9a1c779f2499fb2afc0553e543f18aff6edf (4.11-rc5).
>> >> >
>> >> > A reproducer and .config are attached.
>> >> The script is pretty hard to reproduce the issue in my env.
>> >
>> > I didn't try running it but I also found the reproducer very complicated
>> > to follow. Do you have any plans on having some PoC optimizer, so we can
>> > have a more readable code?
>> > strace is handy for filtering the noise, yes, but sometimes it doesn't
>> > cut it.
>> I got the script now:
>> 1. create sk
>> 2. set sk->sndbuf = x
>> 3. sendmsg with size s1 (s1 < x)
>> 4. sendmsg with size s2 (s1+s2 > x)
>> 5. sendmsg with size s3 (wspace < 0), wait sndbuf, schedule out.
>> 6. listen sk (abnormal operation on sctp client)
>> 7. accept sk.
>>
>> In step 6, sk->sk_state = listening, then step 7 could get the first asoc
>> from ep->asoc_list and alloc a new sk2, attach the asoc to sk2.
>>
>> after a while, sendmsg schedule in, but asoc->sk is sk2, !=sk.
>> the same issue we fix for peeloff on commit dfcb9f4f99f1 ("sctp: deny
>> peeloff operation on asocs with threads sleeping on it") happens.
>
> Yes. That explains why the asoc isn't dead by when sendmsg comes back,
> and avoid that dead check.
>
>>
>> But we should not fix it by the same way as for peeloff. the real reason
>> causes this issue is on step 6, it should disallow listen on the established sk.
>
> Agreed.
>
>>
>> The following fix should work for this, just similar with what
>> inet_listen() did.
>>
>> @@ -7174,6 +7175,9 @@ int sctp_inet_listen(struct socket *sock, int backlog)
>>         if (sock->state != SS_UNCONNECTED)
>>                 goto out;
>>
>> +       if (!sctp_sstate(sk, LISTENING) && !sctp_sstate(sk,CLOSED))
>> +               goto out;
>> +

This fixes the report.

Tested-by: Andrey Konovalov <andreyknvl@google.com>

Thanks!

>>
>> what do you think ?
>
> Yes, agreed.
> Thanks!
>
>   Marcelo

[toc] | [prev] | [next] | [standalone]

#1616962

[Multipart message — attachments visible in raw view] — view raw

On Tue, Apr 4, 2017 at 11:14 PM, Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
> On Wed, Apr 05, 2017 at 01:29:19AM +0800, Xin Long wrote:
>> On Tue, Apr 4, 2017 at 9:28 PM, Andrey Konovalov <andreyknvl@google.com> wrote:
>> > Hi,
>> >
>> > I've got the following error report while fuzzing the kernel with syzkaller.
>> >
>> > On commit a71c9a1c779f2499fb2afc0553e543f18aff6edf (4.11-rc5).
>> >
>> > A reproducer and .config are attached.
>> The script is pretty hard to reproduce the issue in my env.
>
> I didn't try running it but I also found the reproducer very complicated
> to follow. Do you have any plans on having some PoC optimizer, so we can
> have a more readable code?
> strace is handy for filtering the noise, yes, but sometimes it doesn't
> cut it.

We do have some plans (like to remote all those unnecessary helper
functions), but it's probably not going to become much better.

You mostly only need to look at the thr() function to understand
what's going on.

What I sometimes do is run each of the switch cases under strace
separately to understand what each of them do.

I've also attached a program in syzkaller format.
You can take a look at it, if you find it useful, I can start
attaching them for subsequent reports.

>
>> But there seems a case to cause a use-after-free when out of snd_buf.
>>
>> the case is like:
>> -----------
>> one thread:                       another thread:
>>                                   sctp_rcv hold asoc (hold transport)
>>                                   enqueue the chunk to backlog queue
>>                                   [refcnt=2]
>>
>> sctp_close free assoc
>> [refcnt=1]
>>
>> sctp_sendmsg find asoc
>> but not hold it
>>
>> out of snd_buf
>> hold asoc, schedule out
>> [refcnt = 2]
>>
>>                                   process backlog and put asoc/transport
>>                                   [refcnt=1]
>>
>> schedule in, put asoc
>> [refcnt=0] <--- destroyed
>>
>> sctp_sendmsg continue
>
> It shouldn't be continuing here because sctp_wait_for_sndbuf and
> sctp_wait_for_connect functions are checking if the asoc is dead
> already when it schedules in, even though sctp_wait_for_connect return
> value is ignored and sctp_sendmsg() simply returns after that.
> Or the checks for dead asocs in there aren't enough somehow.
>
>> using asoc, panic
>
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[toc] | [prev] | [next] | [standalone]

#1616996

On Wed, Apr 05, 2017 at 04:02:44PM +0200, Andrey Konovalov wrote:
> On Tue, Apr 4, 2017 at 11:14 PM, Marcelo Ricardo Leitner
> <marcelo.leitner@gmail.com> wrote:
> > On Wed, Apr 05, 2017 at 01:29:19AM +0800, Xin Long wrote:
> >> On Tue, Apr 4, 2017 at 9:28 PM, Andrey Konovalov <andreyknvl@google.com> wrote:
> >> > Hi,
> >> >
> >> > I've got the following error report while fuzzing the kernel with syzkaller.
> >> >
> >> > On commit a71c9a1c779f2499fb2afc0553e543f18aff6edf (4.11-rc5).
> >> >
> >> > A reproducer and .config are attached.
> >> The script is pretty hard to reproduce the issue in my env.
> >
> > I didn't try running it but I also found the reproducer very complicated
> > to follow. Do you have any plans on having some PoC optimizer, so we can
> > have a more readable code?
> > strace is handy for filtering the noise, yes, but sometimes it doesn't
> > cut it.
> 
> We do have some plans (like to remote all those unnecessary helper
> functions), but it's probably not going to become much better.
> 
> You mostly only need to look at the thr() function to understand
> what's going on.

Okay.

> 
> What I sometimes do is run each of the switch cases under strace
> separately to understand what each of them do.
> 
> I've also attached a program in syzkaller format.
> You can take a look at it, if you find it useful, I can start
> attaching them for subsequent reports.

Comparing it to thr() they look very close, at least for this one.
But when you cannot extract a reproducer, it will certainly help.

Thanks,
Marcelo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web