From patchwork Thu Jul 2 11:26:30 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Lothar_Wa=C3=9Fmann?= X-Patchwork-Id: 29400 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@bilbo.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from ozlabs.org (ozlabs.org [203.10.76.45]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "mx.ozlabs.org", Issuer "CA Cert Signing Authority" (verified OK)) by bilbo.ozlabs.org (Postfix) with ESMTPS id 22C12B70DB for ; Thu, 2 Jul 2009 22:17:08 +1000 (EST) Received: by ozlabs.org (Postfix) id 12F27DDD1C; Thu, 2 Jul 2009 22:17:08 +1000 (EST) Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.176.167]) by ozlabs.org (Postfix) with ESMTP id 887E8DDD1B for ; Thu, 2 Jul 2009 22:17:06 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751944AbZGBMQ4 (ORCPT ); Thu, 2 Jul 2009 08:16:56 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751359AbZGBMQz (ORCPT ); Thu, 2 Jul 2009 08:16:55 -0400 Received: from mail.karo-electronics.de ([81.173.242.67]:51813 "EHLO mail.karo-electronics.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751225AbZGBMQy (ORCPT ); Thu, 2 Jul 2009 08:16:54 -0400 X-Greylist: delayed 3024 seconds by postgrey-1.27 at vger.kernel.org; Thu, 02 Jul 2009 08:16:52 EDT Received: from lothar by ipc1.ka-ro with local (Exim 4.63 #1 (Debian)) id 1MMKQw-0003vW-Fi; Thu, 02 Jul 2009 13:26:30 +0200 Message-ID: <19020.39270.408816.360526@ipc1.ka-ro> Date: Thu, 2 Jul 2009 13:26:30 +0200 From: =?iso-8859-15?q?Lothar_Wa=DFmann?= To: netdev@vger.kernel.org Cc: davem@davemloft.net Subject: use after free bug in socket code X-Mailer: VM 7.19 under Emacs 21.4.1 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Hi, while developing a canbus driver (with kernel 2.6.30-rc4) I encountered a use-after-free bug that led to the following crash (due to CONFIG_DEBUG_SLAB being enabled): |Unable to handle kernel paging request at virtual address 6b6b6b6b |pgd = d1b4c000 |[6b6b6b6b] *pgd=00000000 |Internal error: Oops: 1 [#1] PREEMPT |Modules linked in: can_raw can flexcan [last unloaded: usbcore] |CPU: 0 Tainted: G W (2.6.30-rc4 #215) |PC is at __wake_up_common+0x30/0x88 |LR is at __wake_up_sync_key+0x48/0x64 |pc : [] lr : [] psr: 90000093 |sp : d18f5d70 ip : 6b6b6b5f fp : d18f5d9c |r10: 00000001 r9 : 00000304 r8 : d1517aec |r7 : 00000001 r6 : 00000001 r5 : 00000001 r4 : a0000013 |r3 : 00000001 r2 : 00000001 r1 : 00000001 r0 : d1517aec |Flags: NzcV IRQs off FIQs on Mode SVC_32 ISA ARM Segment user |Control: 0005317f Table: 91b4c000 DAC: 00000015 |Process ifconfig (pid: 893, stack limit = 0xd18f4268) |Stack: (0xd18f5d70 to 0xd18f6000) |5d60: d18f4000 a0000013 00000001 00000001 |5d80: 00000304 d1517ad0 d18f4000 d18f5ea8 d18f5dcc d18f5da0 c0041d10 c0041884 |5da0: 00000304 d18f5ea8 00000000 d1b029f0 d1b02b44 d1a1c398 c035a8c0 00008914 |5dc0: 00000000 d18f5dd0 c01d8070 c0041cd8 d1517ad0 6b6b6b6b 6b6b6b6b c006b594 |5de0: 00000001 d1b029f0 d1a1c280 c01d8434 d1b1ad88 c01d95fc d1b1ad88 c01db6d8 |5e00: d1b1ad88 c01dabdc d1a1c348 c01f4bac d1a1c280 d1b4a5a8 d1a1c2dc c01f3c1c |5e20: d1a1c280 c01f3df4 00000000 00000000 00000000 d1afe8e0 00000081 c01f45a4 |5e40: 000000c0 d1afe8e0 bf086344 000000c0 00000081 00008914 d18f5ea8 c01e3c08 |5e60: 00008914 00000041 d1afe8e0 c01e3a14 c070d97c be90cbe8 d18f4000 00000000 |5e80: 00000000 c0227bc0 00000020 d18f5eb8 00008913 c01e66cc 00000000 d1b8ed30 |5ea0: d1afe8e0 00000000 316e6163 00000000 00000000 00000000 000000c0 40012ed0 |5ec0: be90ced1 00050918 000000c0 40012ed0 be90ced1 00050918 be90ced1 00008914 |5ee0: be90cbe8 be90cbe8 d187ed10 c0027f68 d18f4000 00000000 00000000 c01d3510 |5f00: d187ed10 be90cbe8 00008914 c00ac4d0 00000003 d187ed10 be90cbe8 c00ac924 |5f20: 00000001 00000000 d1b0ba48 60000013 401b2100 d1b0ba44 00000000 d18f5fb0 |5f40: 00000200 c025c3e4 d1b0ba48 60000013 d1b0ba10 401b2100 d1b0ba44 00000000 |5f60: d1b0ba48 00000003 be90cbe8 00008914 d187ed10 c0027f68 d18f4000 00000000 |5f80: 00000000 c00acc0c be90cdd8 00000000 00000001 00052928 00000001 be90cddc |5fa0: 00000036 c0027dc0 00052928 00000001 00000003 00008914 be90cbe8 00052928 |5fc0: 00052928 00000001 be90cddc 00000036 00000028 be90cbe8 00000003 00000000 |5fe0: 00052a78 be90cbc8 0000ca40 401b210c 60000010 00000003 00000000 00000000 |[] (__wake_up_common+0x30/0x88) from [] (__wake_up_sync_key+0x48/0x64) |[] (__wake_up_sync_key+0x48/0x64) from [] (sock_def_write_space+0xcc/0xec) |[] (sock_def_write_space+0xcc/0xec) from [] (sock_wfree+0x70/0x74) |[] (sock_wfree+0x70/0x74) from [] (skb_release_head_state+0x38/0x5c) |[] (skb_release_head_state+0x38/0x5c) from [] (skb_release_all+0xc/0x18) |[] (skb_release_all+0xc/0x18) from [] (__kfree_skb+0xc/0xc4) |[] (__kfree_skb+0xc/0xc4) from [] (pfifo_fast_reset+0x44/0x6c) |[] (pfifo_fast_reset+0x44/0x6c) from [] (qdisc_reset+0x1c/0x30) |[] (qdisc_reset+0x1c/0x30) from [] (dev_deactivate_queue+0x5c/0x74) |[] (dev_deactivate_queue+0x5c/0x74) from [] (dev_deactivate+0x34/0x204) |[] (dev_deactivate+0x34/0x204) from [] (dev_close+0x70/0xc8) |[] (dev_close+0x70/0xc8) from [] (dev_change_flags+0x74/0x180) |[] (dev_change_flags+0x74/0x180) from [] (devinet_ioctl+0x5f8/0x748) |[] (devinet_ioctl+0x5f8/0x748) from [] (sock_ioctl+0x60/0x264) |[] (sock_ioctl+0x60/0x264) from [] (vfs_ioctl+0x2c/0x90) |[] (vfs_ioctl+0x2c/0x90) from [] (do_vfs_ioctl+0x2c8/0x578) |[] (do_vfs_ioctl+0x2c8/0x578) from [] (sys_ioctl+0x38/0x60) |[] (sys_ioctl+0x38/0x60) from [] (ret_fast_syscall+0x0/0x2c) |Code: e1a0a001 e1a08000 e1a06002 e59b9004 (e59c300c) |Kernel panic - not syncing: Fatal exception in interrupt |[] (unwind_backtrace+0x0/0xe8) from [] (panic+0x58/0x148) |[] (panic+0x58/0x148) from [] (die+0x120/0x150) |[] (die+0x120/0x150) from [] (__do_kernel_fault+0x70/0x80) |[] (__do_kernel_fault+0x70/0x80) from [] (do_alignment+0x174/0x58c) |[] (do_alignment+0x174/0x58c) from [] (do_DataAbort+0x34/0x98) |[] (do_DataAbort+0x34/0x98) from [] (__dabt_svc+0x4c/0x60) |Exception stack(0xd18f5d28 to 0xd18f5d70) |5d20: d1517aec 00000001 00000001 00000001 a0000013 00000001 |5d40: 00000001 00000001 d1517aec 00000304 00000001 d18f5d9c 6b6b6b5f d18f5d70 |5d60: c0041d10 c00418a4 90000093 ffffffff |[] (__dabt_svc+0x4c/0x60) from [] (__wake_up_sync_key+0x48/0x64) |[] (__wake_up_sync_key+0x48/0x64) from [] (__wake_up_sync_key+0x48/0x64) |[] (__wake_up_sync_key+0x48/0x64) from [] (sock_def_write_space+0xcc/0xec) |[] (sock_def_write_space+0xcc/0xec) from [] (sock_wfree+0x70/0x74) |[] (sock_wfree+0x70/0x74) from [] (skb_release_head_state+0x38/0x5c) |[] (skb_release_head_state+0x38/0x5c) from [] (skb_release_all+0xc/0x18) |[] (skb_release_all+0xc/0x18) from [] (__kfree_skb+0xc/0xc4) |[] (__kfree_skb+0xc/0xc4) from [] (pfifo_fast_reset+0x44/0x6c) |[] (pfifo_fast_reset+0x44/0x6c) from [] (qdisc_reset+0x1c/0x30) |[] (qdisc_reset+0x1c/0x30) from [] (dev_deactivate_queue+0x5c/0x74) |[] (dev_deactivate_queue+0x5c/0x74) from [] (dev_deactivate+0x34/0x204) |[] (dev_deactivate+0x34/0x204) from [] (dev_close+0x70/0xc8) |[] (dev_close+0x70/0xc8) from [] (dev_change_flags+0x74/0x180) |[] (dev_change_flags+0x74/0x180) from [] (devinet_ioctl+0x5f8/0x748) |[] (devinet_ioctl+0x5f8/0x748) from [] (sock_ioctl+0x60/0x264) |[] (sock_ioctl+0x60/0x264) from [] (vfs_ioctl+0x2c/0x90) |[] (vfs_ioctl+0x2c/0x90) from [] (do_vfs_ioctl+0x2c8/0x578) |[] (do_vfs_ioctl+0x2c8/0x578) from [] (sys_ioctl+0x38/0x60) |[] (sys_ioctl+0x38/0x60) from [] (ret_fast_syscall+0x0/0x2c) |Rebooting in 1 seconds.. I found that the problem occured due to a 'struct sock' being released that still contained pointers (sk_socket and sk_sleep) to a socket which had already been released earlier and the function sock_def_write_space() tried to wake the wait queue referenced by the dangling sk_sleep pointer. Checking the code that frees those objects I found a call to sock_orphan() in sk_release_common() that invalidates the pointers to the socket object inside the sk. But IMO this call would be more appropriate in sock_release() which releases the object those pointers are referring to. Furthermore adding some debug messages in sock_release() and sk_release_common() showed that sock_release() seems to be commonly called before sk_release_common() which is also a strong point for invalidating the pointers from sock_release() rather than sk_release_common(): |__sock_create: Created socket d14b5d28 sk d1a06050 |sk_alloc: Allocated sk d1a06310 |sk_free: Freeing sk d1a06310 socket (null) |sock_release: releasing socket d14b5d28 sk d1a06050 ops c025ea84 |sock_release: orphaning sk d1a06050 from socket d14b5d28 |sk_free: Freeing sk d1a06050 socket (null) With the following patch I could alleviate the problem and did not find any negative side effects, but I'm not sure, whether this is the Right Thing(TM), since I'm not too familiar with the networking code: a/net/socket.c b/net/socket.c Any comments on this? Lothar Waßmann --- net/socket.c 1 Jul 2009 09:30:42 -0000 +++ net/socket.c 2 Jul 2009 10:25:27 -0000 @@ -524,7 +524,9 @@ void sock_release(struct socket *sock) if (sock->ops) { struct module *owner = sock->ops->owner; + if (sock->sk) + sock_orphan(sock->sk); sock->ops->release(sock); sock->ops = NULL; module_put(owner); a/net/core/sock.c b/net/core/sock.c --- net/core/sock.c 2 Jun 2009 15:37:50 -0000 +++ net/core/sock.c 2 Jul 2009 10:42:33 -0000 @@ -1982,7 +1983,9 @@ void sk_common_release(struct sock *sk) * until the last reference will be released. */ - sock_orphan(sk); + //sock_orphan(sk); + if (WARN_ON(sk->sk_sleep || sk->sk_socket)) + sock_orphan(sk); xfrm_sk_free_policy(sk);