From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vlad Yasevich Subject: Re: [PATCH net v2] net: sctp: fix NULL pointer dereference in socket destruction Date: Thu, 06 Jun 2013 10:39:52 -0400 Message-ID: <51B09F38.2000907@gmail.com> References: <1370526827-23882-1-git-send-email-dborkman@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: davem@davemloft.net, netdev@vger.kernel.org, linux-sctp@vger.kernel.org To: Daniel Borkmann Return-path: Received: from mail-ye0-f175.google.com ([209.85.213.175]:46081 "EHLO mail-ye0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752096Ab3FFOj4 (ORCPT ); Thu, 6 Jun 2013 10:39:56 -0400 In-Reply-To: <1370526827-23882-1-git-send-email-dborkman@redhat.com> Sender: netdev-owner@vger.kernel.org List-ID: On 06/06/2013 09:53 AM, Daniel Borkmann wrote: > While stress testing sctp sockets, I hit the following panic: > > BUG: unable to handle kernel NULL pointer dereference at 0000000000000020 > IP: [] sctp_endpoint_free+0xe/0x40 [sctp] > PGD 7cead067 PUD 7ce76067 PMD 0 > Oops: 0000 [#1] SMP > Modules linked in: sctp(F) libcrc32c(F) [...] > CPU: 7 PID: 2950 Comm: acc Tainted: GF 3.10.0-rc2+ #1 > Hardware name: Dell Inc. PowerEdge T410/0H19HD, BIOS 1.6.3 02/01/2011 > task: ffff88007ce0e0c0 ti: ffff88007b568000 task.ti: ffff88007b568000 > RIP: 0010:[] [] sctp_endpoint_free+0xe/0x40 [sctp] > RSP: 0018:ffff88007b569e08 EFLAGS: 00010292 > RAX: 0000000000000000 RBX: ffff88007db78a00 RCX: dead000000200200 > RDX: ffffffffa049fdb0 RSI: ffff8800379baf38 RDI: 0000000000000000 > RBP: ffff88007b569e18 R08: ffff88007c230da0 R09: 0000000000000001 > R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000 > R13: ffff880077990d00 R14: 0000000000000084 R15: ffff88007db78a00 > FS: 00007fc18ab61700(0000) GS:ffff88007fc60000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b > CR2: 0000000000000020 CR3: 000000007cf9d000 CR4: 00000000000007e0 > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400 > Stack: > ffff88007b569e38 ffff88007db78a00 ffff88007b569e38 ffffffffa049fded > ffffffff81abf0c0 ffff88007db78a00 ffff88007b569e58 ffffffff8145b60e > 0000000000000000 0000000000000000 ffff88007b569eb8 ffffffff814df36e > Call Trace: > [] sctp_destroy_sock+0x3d/0x80 [sctp] > [] sk_common_release+0x1e/0xf0 > [] inet_create+0x2ae/0x350 > [] __sock_create+0x11f/0x240 > [] sock_create+0x30/0x40 > [] SyS_socket+0x4c/0xc0 > [] ? do_page_fault+0xe/0x10 > [] ? page_fault+0x22/0x30 > [] system_call_fastpath+0x16/0x1b > Code: 0c c9 c3 66 2e 0f 1f 84 00 00 00 00 00 e8 fb fe ff ff c9 c3 66 0f > 1f 84 00 00 00 00 00 55 48 89 e5 53 48 83 ec 08 66 66 66 66 90 <48> > 8b 47 20 48 89 fb c6 47 1c 01 c6 40 12 07 e8 9e 68 01 00 48 > RIP [] sctp_endpoint_free+0xe/0x40 [sctp] > RSP > CR2: 0000000000000020 > ---[ end trace e0d71ec1108c1dd9 ]--- > > I did not hit this with the lksctp-tools functional tests, but with a > small, multi-threaded test program, that heavily allocates, binds, > listens and waits in accept on sctp sockets, and then randomly kills > some of them (no need for an actual client in this case to hit this). > Then, again, allocating, binding, etc, and then killing child processes. > > This panic then only occurs when ``echo 1 > /proc/sys/net/sctp/auth_enable'' > is set. The cause for that is actually very simple: in sctp_endpoint_init() > we enter the path of sctp_auth_init_hmacs(). There, we try to allocate > our crypto transforms through crypto_alloc_hash(). In our scenario, > it then can happen that crypto_alloc_hash() fails with -EINTR from > crypto_larval_wait(), thus we bail out and release the socket via > sk_common_release(), sctp_destroy_sock() and hit the NULL pointer > dereference as soon as we try to access members in the endpoint during > sctp_endpoint_free(), since endpoint at that time is still NULL. Now, > if we have that case, we do not need to do any cleanup work and just > leave the destruction handler. > > Signed-off-by: Daniel Borkmann Acked-by: Vlad Yasevich should be queued for stable as well. -vlad > --- > net/sctp/socket.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > index f631c5f..3df7327 100644 > --- a/net/sctp/socket.c > +++ b/net/sctp/socket.c > @@ -4003,6 +4003,12 @@ SCTP_STATIC void sctp_destroy_sock(struct sock *sk) > > /* Release our hold on the endpoint. */ > sp = sctp_sk(sk); > + /* This could happen during socket init, thus we bail out > + * early, since the rest of the below is not setup either. > + */ > + if (sp->ep == NULL) > + return; > + > if (sp->do_auto_asconf) { > sp->do_auto_asconf = 0; > list_del(&sp->auto_asconf_list); >