From patchwork Thu Dec 20 10:43:16 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mathias Payer X-Patchwork-Id: 10738751 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 0A13713AD for ; Thu, 20 Dec 2018 10:50:56 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id E0D6B285A2 for ; Thu, 20 Dec 2018 10:50:55 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id D533628770; Thu, 20 Dec 2018 10:50:55 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 97F57285A2 for ; Thu, 20 Dec 2018 10:50:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730965AbeLTKus (ORCPT ); Thu, 20 Dec 2018 05:50:48 -0500 Received: from mail.albtraum.org ([94.130.183.3]:57646 "EHLO mail.albtraum.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725869AbeLTKus (ORCPT ); Thu, 20 Dec 2018 05:50:48 -0500 X-Greylist: delayed 445 seconds by postgrey-1.27 at vger.kernel.org; Thu, 20 Dec 2018 05:50:44 EST Received: from [128.178.122.103] (icdhcp-2-103.epfl.ch [128.178.122.103]) by mail.albtraum.org (Postfix) with ESMTPSA id D6B781F437; Thu, 20 Dec 2018 11:43:16 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=albtraum.org; s=mail; t=1545302597; bh=Qr775KMw9la3CJZm3XyPK5nchF+0C1wTq0kay6U0lrc=; h=To:From:Subject:Cc:Date:From; b=jcLvmeUaIdIBo3AQo+ZLtp8BYDbXc5jQf4wVZ893dBBKgSri9Be076J57s6RCoqpA oEkg93WZxbVJthmoOyEnwpJK2Wp0OC/wqpUiF2/cQC6KHcESx7AzniIvmxGYVnoy8I 7whACKln7FJ1DraizqlmKRXui+K+igredOZ847+XyIW9SDGcaqxAZdLj6Xq0OohUDV /461y0xRWtasnggnPBxJJ3+e7Np3F3+2DQAW40ET9ZKz6JWyl01xP1BeM9AimQwfAC LrsSK6tr8fHPpWMPjBEE3zDT1NLF/uTrszo+aeEiK4L5O1gPmApw/BctPZGLZQCgD/ ZljQ/6kBZNfkg== To: Kalle Valo , "David S. Miller" , linux-wireless@vger.kernel.org From: Mathias Payer Subject: NULL deref in drivers/net/wireless/ath/ath{6kl|10k}/usb.c Openpgp: preference=signencrypt Autocrypt: addr=mathias.payer@nebelwelt.net; keydata= mQINBFJN/c4BEADoVGNq24bAaMFvV6wA/lMi6pUqxWIjp8LnV/grWoSbSgSzyzZAMzehojQY jow3KaHsBvbIPStUHM3zwJqgF5VY5ZlhATJ2qFbfG8SHZxLRs2WmQlJDWRC2JmxoihNgnmww 2C21qIuWgVJPdsWJ1RVonhsqjLThZBwVy4KddIoR1SN3IzsOl4AvdfaAtF6/kP15UG03ZOkm BEaW945fHjDwbFtRGu/UGzSfGH+vv4BrBMOY+1HtNEve23MZSRh9ExNbNVksXvZibjEin8pm ui0+AhzzodMWtvgSPpI1GsLjRIMmjzuv5BPsAY8F0uLbvalCn7Q4GLDygZ1oIh3oRPxMfiL3 toKb0Sximt9aFFUPvirTfXPl8QomYt7aHbC/OUtFK3K+ueQKG1D3pSTaHCLP3zb7dq3E4goO +Lopfe79m7E1iiXBY+06+lZf99wvywRcXcnczjfi3kRhIYJS2yphDM013pCBYvlQi6E7NBbb FL/+Q8xKyXJA1wMAj/F0oqqU2LxEgTUiRR/OSyYOknFCZ+wKn2FdDsXxxcCtXmJjD9CDjwbW qg/kRyWxAr76EEPokjQ848Vc6Ou/798PHo0kq+r6PODuPZ/hDvQwR9j6Res2Zkx2jOV+Y7Q0 RD6c2v7dZPC/sIdtqeKqLOb2cI+qXxCFeI6ZQ1cvOh0OzidVhQARAQABtCtNYXRoaWFzIFBh eWVyIDxtYXRoaWFzLnBheWVyQG5lYmVsd2VsdC5uZXQ+iQI+BBMBAgAoAhsDBgsJCAcDAgYV CAIJCgsEFgIDAQIeAQIXgAUCWaxB8gUJCyCrDwAKCRBOYz7xOgSICIg8D/0a8F4ImrdE3cOq ww1gf+uBiDkzxHIxctPdChaJLR2s8zUctErW9naDEgUtyZ/iUReAox3POGIS49V4j/EPJoxY 2akO5zNh0jbw4zKeT/BYXCuDbF4XDxtve2ZAAkpo+UtXAweAdZhYMFGZVaaodHW6f/29u4wr rtZ5RlsdOf4nQTPzLpUdVQke5WhJ+xPH0giWZtGvNaSfF35JHaMcCkJK7edt+WVQEuChmevq YG+C50HTfucVz1TcuToMybm0iBW3W5HQ9vpOd3CsFP9Za7KNoScX0yfoROD33Za3wOdtTSQH MtnddXj3mHRL7MaaeTtlMoanD7/+kU+wE8EvTKCGfquho3h/6rsU7I6oF+heHZJnHcjKZz6p fS0c0xZ2Zs2Vqcf4o/j4cZBvCakxzTyedsCDZxPDQc8jmufFUYYlXG3EY4QOEzEHRbWN5U6I RHM1z+EnFuv9s3tQ16lT43RG72tgusKkXzdm8+qzcc2K0lp2udeKgD2cTCB6BT4dmli/pBgO wW++0+LcnnvZiSCoi3u+ZGS/GYYF7l+QQONdmHjIq2dwdheE3hKZXV9WnGPnesg2mhVO9C+U pUx6g/B/5l06Gh1SoWl1TDTjFnRU1KQ3kRhgmCDmheUw7L6I0lrnvmVuQBEB3V2pMHgIfvIg 9dmu90UYpb/ykobB2J9kRLkCDQRSTf3OARAAxle1/hIek26B+KuEQV60TLzyht7NUFfVy8DE 1uOixTnfcZ95lWVtStaA2ntymkbdT8uK7gOYegvErt43eKX89N/h1929ZxWoEZYF/K8KT5Cf rPOakM+cl1wRuwKkxk2HZ9Bmtjrmq/AD6cArQzogkkyvPgQuyRteW9fLgzz8i73/9vv72oEN Rew3eoNlITZB2qxwtQIkRAIyQ+KmUVjCn/qedwZ59ivJI3JrCk/r9NecSzRPH6bzThFfmKbH X2ki/TG62EseR1j0+YNApO2KP1opEE4Tovh99Ux6jqRR3G6AB2+cVSquKHVPIyB4uXH5qNH1 Fm0nbL0gqTpW1Q55oFlgd5ifM2jIMjShj8G4lcNhb4sKNNEYHlrIBuZEwEO1ACX/tHkUAdvK d+F9AhPJfiYiDHBbpwB5P+VSsdrEqfZHEUz4b+u3s562RMsTTjnFYI62UtLAl9BbAZsE/XME EMUhaGbcBVvgxAgGBV3U0KHfJhjDHZ6+hLLfJFaDZltIMXg+1Dl1Ow5UuE23E5Xrbupkwfxd mXqYyMnXS4d/gRccFekQ1oZpY/mCrPx6LrV1nprrbdwS4EA4wxOOkFIT8fvjiX+av6TRNOox UGPKI2EWPFrxZJvh0YtOL8VmYa1J+Vz++93uHEjIJxmVP+wTFYNyFwXw1GWvNxDACDBQqyUA EQEAAYkCJQQYAQIADwIbDAUCWaxCIAUJCyCrTwAKCRBOYz7xOgSICJXnD/9DswXXFWsNm7k3 fESjo8IVNkjJUd4yfWvaeVXH8FN+UYLwTsm1bd8SoMKSwmeeUqifjUj7bIQmDaAilyUwwGlI 67Y8uXaugvCVbcYnqDxXGCDxZKP17oGTGd8406m2I0Mi++K9hQH+znmRCyPRnguWfN/ZiNVV aSjG0IbB6Bj3764AmwI/mrUCB7pFezrDEH+NTOWmnlYgD4bjjNTP53SPu1zzQV7QSrIruxUG SLZkEf1sLo4L832b6wugnDGD1kMuRXVLqXRjupvSTNyhSqUsd03IH8fdnJz3meTxgrw/MQ6k AfH3tRm4inkfmbr9ICa0he2PB/BI3vhE974TysqHY8biaoZW8LghXuMRV5CAURrJVH4Ri3/1 tRUK2+lBpMcLQgXv39em0nZ8lb/xIKzunHJPyGyAc6xJog5SWQBM/nI4Dn0AuaTlGnB1CNTc s0TJM0kr9ig6hjHNOOuV6HxaF8cRX/3Dglw77bAJUCg0sil8kAiFreiwM2gOteIc5vzgUFc2 UPLclTYXBHPTQ9Dy8o6IlFSPVjLgt/eSjF7K+4LPPjGTGgM5FQ4BF4f1gnxCtngRfyKDHCSD e5tkK8aEUkMQq9vwYODqhULzyhXLWJFN4hJZ8tiyn8uf9MUhlr1AE4sS9mMvvdiH9Nb93D5T 6cVR5xSPwYD5e5fg6EsTcA== Cc: Hui Peng Message-ID: <137aecba-a410-7795-61c9-0e431d75eaaa@nebelwelt.net> Date: Thu, 20 Dec 2018 11:43:16 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.0 MIME-Version: 1.0 Sender: linux-wireless-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-wireless@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Hi there, We have developed a USB testing framework and found two NULL derefes in the ath driver when an attacker-controlled USB device sends a malformed USB packet. The bug is in drivers/net/wireless/ath/ath{6kl|10k}/usb.c We show the bug for ath6kl/usb.c but the code seems to be copied over to ath10k/usb.c and it is equally vulnerable. ``` ath6kl_usb_alloc_urb_from_pipe(struct ath6kl_usb_pipe *pipe) { struct ath6kl_urb_context *urb_context = NULL; unsigned long flags; // buggy statement spin_lock_irqsave(&pipe->ar_usb->cs_lock, flags); if (!list_empty(&pipe->urb_list_head)) { urb_context = list_first_entry(&pipe->urb_list_head, struct ath6kl_urb_context, link); list_del(&urb_context->link); pipe->urb_cnt--; } spin_unlock_irqrestore(&pipe->ar_usb->cs_lock, flags); return urb_context; } ``` The `field pipe->ar_usb` may be NULL if the corresponding pipes have not been setup, resulting in a NULL pointer deref, when the spin_lock tries to access the `cs_lock` field. During setup, the initialization code allocates the pipes according to whatever is requested from the USB channel, potentially missing the initialization of some pipes. # Code flow 1. `ath6kl_usb_probe` calls ` ath6kl_usb_create` to create an `ath6kl_usb` object 2. Inside `ath6kl_usb_create` (https://elixir.bootlin.com/linux/v4.14.81/source/drivers/net/wireless/ath/ath6kl/usb.c#L613) a. it allocates memory for the ath6kl_usb object using `kzalloc` b. the array of `ath6kl_usb_pipe` (`pipes` field) is setup by calling `ath6kl_usb_setup_pipe_resources`, where it may goes wrong 3. The `ath6kl_usb_setup_pipe_resources` (https://elixir.bootlin.com/linux/v4.14.81/source/drivers/net/wireless/ath/ath6kl/usb.c#L295) function iterates through all endpoints reported by the device and initializes each pipe according to the endpoint information It calculates the pipnum (the index to the pipes array) according to the endpoint address by calling `ath6kl_usb_get_logical_pipe_num`, and then sets up the pipe object. The selected pipe object's `ar_usb` field will be set to the containing `ath6kl_usb` object. If a pipe object is not selected by the endpoint information, their `ar_usb` field will be zero, as the underlying `ath6kl_usb` object is allocated by `kzalloc`. The driver assumes that all endpoint addresses are complete (covering range 0 to MAX_PIPE_NUM), assuming all pipe objects are setup correctly. A malicious USB device can force some pipes to not be setup. # Proposed fix As a simple fix, we propose to check for NULL in ath6kl_usb_alloc_urb_from_pipe, making sure the underlying object was correctly initialized. This *should* work for the two drivers and will be a quick fix. The cleanest solution would check at the end of `ath6kl_usb_setup_pipe_resources` that all required pipes are set up correctly. The question is what is the minimum set of pipes that the hardware exports? As I don't have access to the real hardware, I cannot test this on a valid device. Looking through the code, I see that the the driver itself expects at least pipes[ATH6KL_USB_PIPE_RX_DATA] #482, #484, #1188 pipes[ATH6KL_USB_PIPE_RX_DATA2] #1190 to be valid as they are accessed directly. `hif_start` #677 seems to expect that all TX pipes are setup: ``` for (i = ATH6KL_USB_PIPE_TX_CTRL; i <= ATH6KL_USB_PIPE_TX_DATA_HP; i++) { device->pipes[i].urb_cnt_thresh = device->pipes[i].urb_alloc / 2; } ``` -> ATH6KL_USB_PIPE_TX_CTRL = 0, ATH6KL_USB_PIPE_TX_DATA_LP, ATH6KL_USB_PIPE_TX_DATA_MP, ATH6KL_USB_PIPE_TX_DATA_HP, But, there are other accesses to pipes[XXX] where XXX is passed into the module from outside. I don't know the network stack well enough to infer what values are passed in. If we can assume that all pipes in `enum ATH6KL_USB_PIPE_ID` #30 are setup, then we could simply iterate through the pipe array at the end of `ath6kl_usb_setup_pipe_resources` and check that they are all non-NULL. The simple (attached) fix is likely enough but someone with access to the hardware may want to develop a more extensive fix. Let me know what you think :) Best, Mathias and Hui BUG: KASAN: null-ptr-deref in __lock_acquire+0x5a8/0x1b40 kernel/locking/lockdep.c:3365 CPU: 0 PID: 5496 Comm: test Not tainted 4.14.81 #1 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014 Call Trace: __dump_stack lib/dump_stack.c:17 [inline] dump_stack+0xc1/0x127 lib/dump_stack.c:53 kasan_report_error mm/kasan/report.c:349 [inline] kasan_report.cold.6+0x6d/0x2d3 mm/kasan/report.c:409 check_memory_region_inline mm/kasan/kasan.c:260 [inline] __asan_load8+0x54/0x90 mm/kasan/kasan.c:694 __lock_acquire+0x5a8/0x1b40 kernel/locking/lockdep.c:3365 lock_acquire+0xc9/0x200 kernel/locking/lockdep.c:3991 __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline] _raw_spin_lock_irqsave+0x4a/0x5e kernel/locking/spinlock.c:160 ath6kl_usb_alloc_urb_from_pipe+0x3c/0x130 drivers/net/wireless/ath/ath6kl/usb.c:135 ath6kl_usb_post_recv_transfers.constprop.10+0x17e/0x210 drivers/net/wireless/ath/ath6kl/usb.c:410 ath6kl_usb_start_recv_pipes drivers/net/wireless/ath/ath6kl/usb.c:484 [inline] hif_start drivers/net/wireless/ath/ath6kl/usb.c:682 [inline] ath6kl_usb_power_on+0x4b/0xa0 drivers/net/wireless/ath/ath6kl/usb.c:1041 ath6kl_hif_power_on drivers/net/wireless/ath/ath6kl/hif-ops.h:136 [inline] ath6kl_core_init+0x119/0x7c0 drivers/net/wireless/ath/ath6kl/core.c:97 ath6kl_usb_probe+0x748/0x8e0 drivers/net/wireless/ath/ath6kl/usb.c:1147 usb_probe_interface+0x1b5/0x450 drivers/usb/core/driver.c:361 really_probe drivers/base/dd.c:405 [inline] driver_probe_device+0x381/0x460 drivers/base/dd.c:549 __device_attach_driver+0x13a/0x160 drivers/base/dd.c:645 bus_for_each_drv+0xdb/0x130 drivers/base/bus.c:463 __device_attach+0x14b/0x1d0 drivers/base/dd.c:702 device_initial_probe+0x1f/0x30 drivers/base/dd.c:749 bus_probe_device+0x14a/0x160 drivers/base/bus.c:523 device_add+0x570/0xaf0 drivers/base/core.c:1849 usb_set_configuration+0x875/0xcc0 drivers/usb/core/message.c:1947 generic_probe+0x7d/0xb0 drivers/usb/core/generic.c:174 usb_probe_device+0x64/0xa0 drivers/usb/core/driver.c:266 really_probe drivers/base/dd.c:405 [inline] driver_probe_device+0x381/0x460 drivers/base/dd.c:549 __device_attach_driver+0x13a/0x160 drivers/base/dd.c:645 bus_for_each_drv+0xdb/0x130 drivers/base/bus.c:463 __device_attach+0x14b/0x1d0 drivers/base/dd.c:702 device_initial_probe+0x1f/0x30 drivers/base/dd.c:749 bus_probe_device+0x14a/0x160 drivers/base/bus.c:523 device_add+0x570/0xaf0 drivers/base/core.c:1849 usb_new_device+0x584/0xd60 drivers/usb/core/hub.c:2544 hub_port_connect drivers/usb/core/hub.c:5002 [inline] hub_port_connect_change drivers/usb/core/hub.c:5117 [inline] port_event drivers/usb/core/hub.c:5223 [inline] hub_event_impl+0x10be/0x1f80 drivers/usb/core/hub.c:5335 hub_event+0x38/0x50 drivers/usb/core/hub.c:5222 process_one_work+0x944/0x15f0 kernel/workqueue.c:2112 worker_thread+0xef/0x10d0 kernel/workqueue.c:2246 kthread+0x367/0x420 kernel/kthread.c:238 ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:437 RIP: 0033:0x452d07 RSP: 002b:00007ffcbb2ff968 EFLAGS: 00000202 ORIG_RAX: 0000000000000010 RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 0000000000452d07 RDX: 00007ffcbb2ff980 RSI: 00000000c0105512 RDI: 0000000000000000 RBP: 00000000200f0000 R08: 000000002096cff8 R09: 000000002071ffa2 R10: 0000000000000004 R11: 0000000000000202 R12: 0000000000000024 R13: 000000002096cff8 R14: 0000000000000004 R15: 0000000000000003 ================================================================== commit 7feca65b6481514ffadcd64905612d91d23fcd39 Author: Mathias Payer Date: Tue Dec 18 11:12:28 2018 +0100 Fix NULL deref in drivers/net/wireless/ath/ath{6kl|10k}/usb.c ath{6kl|10k}_usb_alloc_urb_from_pipe does not check if the pipe is valid and that is has been correctly allocated. Add a NULL check before accessing the pipe. Reported-by: Hui Peng Reported-by: Mathias Payer Signed-off-by: Hui Peng Signed-off-by: Mathias Payer diff --git a/drivers/net/wireless/ath/ath10k/usb.c b/drivers/net/wireless/ath/ath10k/usb.c index f731d35ee76d..56d0bb57891b 100644 --- a/drivers/net/wireless/ath/ath10k/usb.c +++ b/drivers/net/wireless/ath/ath10k/usb.c @@ -49,6 +49,11 @@ ath10k_usb_alloc_urb_from_pipe(struct ath10k_usb_pipe *pipe) struct ath10k_urb_context *urb_context = NULL; unsigned long flags; + /* bail if this pipe is not allocated */ + if (pipe->ar_usb == NULL) { + return NULL; + } + spin_lock_irqsave(&pipe->ar_usb->cs_lock, flags); if (!list_empty(&pipe->urb_list_head)) { urb_context = list_first_entry(&pipe->urb_list_head, diff --git a/drivers/net/wireless/ath/ath6kl/usb.c b/drivers/net/wireless/ath/ath6kl/usb.c index 4defb7a0330f..64cd8825fcb7 100644 --- a/drivers/net/wireless/ath/ath6kl/usb.c +++ b/drivers/net/wireless/ath/ath6kl/usb.c @@ -132,6 +132,11 @@ ath6kl_usb_alloc_urb_from_pipe(struct ath6kl_usb_pipe *pipe) struct ath6kl_urb_context *urb_context = NULL; unsigned long flags; + /* bail if this pipe is not allocated */ + if (pipe->ar_usb == NULL) { + return NULL; + } + spin_lock_irqsave(&pipe->ar_usb->cs_lock, flags); if (!list_empty(&pipe->urb_list_head)) { urb_context =