Re: net/sctp: list double add warning in sctp_endpoint_add_asoc
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 > 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 > >> 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
Re: net/sctp: list double add warning in sctp_endpoint_add_asoc
On Wed, Apr 5, 2017 at 2:44 PM, Marcelo Ricardo Leitner
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
>> 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
>> >> 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
Thanks!
>>
>> what do you think ?
>
> Yes, agreed.
> Thanks!
>
> Marcelo
Re: net/sctp: list double add warning in sctp_endpoint_add_asoc
On Tue, Apr 4, 2017 at 11:14 PM, Marcelo Ricardo Leitner 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 >> 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+unsubscr...@googlegroups.com. > For more options, visit https://groups.google.com/d/optout. log Description: Binary data
Re: net/sctp: list double add warning in sctp_endpoint_add_asoc
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
> 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
> >> 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
Re: net/sctp: list double add warning in sctp_endpoint_add_asoc
On Wed, Apr 5, 2017 at 5:14 AM, Marcelo Ricardo Leitner
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
>> 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 ?
Re: net/sctp: list double add warning in sctp_endpoint_add_asoc
On Wed, Apr 05, 2017 at 01:29:19AM +0800, Xin Long wrote: > On Tue, Apr 4, 2017 at 9:28 PM, Andrey Konovalov > 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
Re: net/sctp: list double add warning in sctp_endpoint_add_asoc
On Tue, Apr 4, 2017 at 9:28 PM, Andrey Konovalov 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.
