diff mbox series

NULL deref in drivers/net/wireless/ath/ath{6kl|10k}/usb.c

Message ID 137aecba-a410-7795-61c9-0e431d75eaaa@nebelwelt.net
State RFC
Headers show
Series NULL deref in drivers/net/wireless/ath/ath{6kl|10k}/usb.c | expand

Commit Message

Mathias Payer Dec. 20, 2018, 10:43 a.m. UTC
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
==================================================================
diff mbox series

Patch

commit 7feca65b6481514ffadcd64905612d91d23fcd39
Author: Mathias Payer <mathias.payer@nebelwelt.net>
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 <benquike@gmail.com>
    Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
    Signed-off-by: Hui Peng <benquike@gmail.com>
    Signed-off-by: Mathias Payer <mathias.payer@nebelwelt.net>

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 =