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
commit 7feca65b6481514ffadcd64905612d91d23fcd39
Author: Mathias Payer <mathias.payer@xxxxxxxxxxxxx>
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@xxxxxxxxx>
Reported-by: Mathias Payer <mathias.payer@xxxxxxxxxxxxx>
Signed-off-by: Hui Peng <benquike@xxxxxxxxx>
Signed-off-by: Mathias Payer <mathias.payer@xxxxxxxxxxxxx>
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 =
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 ==================================================================
Attachment:
signature.asc
Description: OpenPGP digital signature
