From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753091AbdDEODu (ORCPT ); Wed, 5 Apr 2017 10:03:50 -0400 Received: from mail-pg0-f42.google.com ([74.125.83.42]:34815 "EHLO mail-pg0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752584AbdDEODo (ORCPT ); Wed, 5 Apr 2017 10:03:44 -0400 MIME-Version: 1.0 In-Reply-To: <20170405124415.GC911@localhost.localdomain> References: <20170404211454.GA911@localhost.localdomain> <20170405124415.GC911@localhost.localdomain> From: Andrey Konovalov Date: Wed, 5 Apr 2017 16:03:43 +0200 Message-ID: Subject: Re: net/sctp: list double add warning in sctp_endpoint_add_asoc To: Marcelo Ricardo Leitner Cc: Xin Long , Vlad Yasevich , Neil Horman , "David S. Miller" , linux-sctp@vger.kernel.org, netdev , LKML , Dmitry Vyukov , Eric Dumazet , Kostya Serebryany , syzkaller Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrey Konovalov Date: Wed, 05 Apr 2017 14:03:43 +0000 Subject: Re: net/sctp: list double add warning in sctp_endpoint_add_asoc Message-Id: List-Id: References: <20170404211454.GA911@localhost.localdomain> <20170405124415.GC911@localhost.localdomain> In-Reply-To: <20170405124415.GC911@localhost.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Marcelo Ricardo Leitner Cc: Xin Long , Vlad Yasevich , Neil Horman , "David S. Miller" , linux-sctp@vger.kernel.org, netdev , LKML , Dmitry Vyukov , Eric Dumazet , Kostya Serebryany , syzkaller 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