* NULL deref in drivers/net/wireless/ath/ath{6kl|10k}/usb.c
@ 2018-12-20 10:43 Mathias Payer
0 siblings, 0 replies; only message in thread
From: Mathias Payer @ 2018-12-20 10:43 UTC (permalink / raw)
To: Kalle Valo, David S. Miller, linux-wireless; +Cc: Hui Peng
[-- Attachment #1.1.1: Type: text/plain, Size: 4330 bytes --]
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
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.1.2: bug2-ath-usb.patch --]
[-- Type: text/x-patch; name="bug2-ath-usb.patch", Size: 1854 bytes --]
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 =
[-- Attachment #1.1.3: bug2_kasan-ath6kl-null-deref-bug.report --]
[-- Type: text/plain, Size: 3507 bytes --]
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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] only message in thread
only message in thread, other threads:[~2018-12-20 10:50 UTC | newest]
Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-20 10:43 NULL deref in drivers/net/wireless/ath/ath{6kl|10k}/usb.c Mathias Payer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).