This patch fixes a NULL pointer dereference in mt76 that occurs when a
status work like mt76u_tx_status_data() queued from mt76u_status_worker()
is called in worker thread while the device initialization failed.
Pointers dereferenced in the work that should have been initialized
during the device registration in mt76_register_device(),
'dev->mphy.chandef.chan' in mt76x02_mac_fill_tx_status(), for example,
may be NULL if the initialization failed. The patch adds a check that
safely terminates the function if that is the case.
Found by a modified version of syzkaller.
KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
CPU: 0 PID: 98 Comm: kworker/u2:2 Not tainted 5.14.0+ #78
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.12.1-0-ga5cab58e9a3f-prebuilt.qemu.org 04/01/2014
Workqueue: mt76 mt76u_tx_status_data
RIP: 0010:mt76x02_mac_fill_tx_status.isra.0+0x82c/0x9e0
Code: c5 48 b8 00 00 00 00 00 fc ff df 80 3c 02 00 0f 85 94 01 00 00 48 b8 00 00 00 00 00 fc ff df 4d 8b 34 24 4c 89 f2 48 c1 ea 03 <0f> b6 04 02 84 c0 74 08 3c 03 0f 8e 89 01 00 00 41 8b 16 41 0f b7
RSP: 0018:ffffc900005af988 EFLAGS: 00010246
RAX: dffffc0000000000 RBX: ffffc900005afae8 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffffffff832fc661 RDI: ffffc900005afc2a
RBP: ffffc900005afae0 R08: 0000000000000001 R09: fffff520000b5f3c
R10: 0000000000000003 R11: fffff520000b5f3b R12: ffff88810b6132d8
R13: 000000000000ffff R14: 0000000000000000 R15: ffffc900005afc28
FS: 0000000000000000(0000) GS:ffff88811aa00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fa0eda6a000 CR3: 0000000118f17000 CR4: 0000000000750ef0
PKRU: 55555554
Call Trace:
? do_raw_spin_lock+0x125/0x2e0
? mt76x02_mac_write_txwi+0xdc0/0xdc0
? rwlock_bug.part.0+0x90/0x90
? __dev_printk+0x1d6/0x1fe
mt76x02_send_tx_status+0x1d2/0xeb0
? usleep_range+0xb3/0x170
? mt76x02_mac_load_tx_status+0x4b0/0x4b0
? rcu_read_lock_sched_held+0xa1/0xd0
? rcu_read_lock_bh_held+0xb0/0xb0
? mt76u_rr+0x3c/0x50
mt76x02_tx_status_data+0x8e/0xd0
? mt76x02_tx_set_txpwr_auto+0x330/0x330
mt76u_tx_status_data+0xe1/0x240
? mt76u_read_copy_ext+0x180/0x180
? rcu_read_lock_sched_held+0x81/0xd0
? rcu_read_lock_bh_held+0xb0/0xb0
? lockdep_hardirqs_on_prepare+0x273/0x3e0
process_one_work+0x92b/0x1460
? pwq_dec_nr_in_flight+0x330/0x330
? rwlock_bug.part.0+0x90/0x90
worker_thread+0x95/0xe00
? __kthread_parkme+0x115/0x1e0
? process_one_work+0x1460/0x1460
kthread+0x3a1/0x480
? set_kthread_struct+0x120/0x120
ret_from_fork+0x1f/0x30
Modules linked in:
---[ end trace 8df5d20fc5040f65 ]---
RIP: 0010:mt76x02_mac_fill_tx_status.isra.0+0x82c/0x9e0
Code: c5 48 b8 00 00 00 00 00 fc ff df 80 3c 02 00 0f 85 94 01 00 00 48 b8 00 00 00 00 00 fc ff df 4d 8b 34 24 4c 89 f2 48 c1 ea 03 <0f> b6 04 02 84 c0 74 08 3c 03 0f 8e 89 01 00 00 41 8b 16 41 0f b7
RSP: 0018:ffffc900005af988 EFLAGS: 00010246
RAX: dffffc0000000000 RBX: ffffc900005afae8 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffffffff832fc661 RDI: ffffc900005afc2a
RBP: ffffc900005afae0 R08: 0000000000000001 R09: fffff520000b5f3c
R10: 0000000000000003 R11: fffff520000b5f3b R12: ffff88810b6132d8
R13: 000000000000ffff R14: 0000000000000000 R15: ffffc900005afc28
FS: 0000000000000000(0000) GS:ffff88811aa00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fa0eda6a000 CR3: 0000000118f17000 CR4: 0000000000750ef0
PKRU: 55555554
Reported-by: Dokyung Song <dokyungs@xxxxxxxxxxxx>
Reported-by: Jisoo Jang <jisoo.jang@xxxxxxxxxxxx>
Reported-by: Minsuk Kang <linuxlovemin@xxxxxxxxxxxx>
Signed-off-by: Minsuk Kang <linuxlovemin@xxxxxxxxxxxx>
---
The crash we found occurs when the initialization failed in
mt76x0u_register_device() and mt76u_stop_tx() is called via
mt76u_queues_deinit() as an error handling. mt76u_stop_tx()
enables a kthread with mt76_worker_enable() and this
make 'dev->mphy.chandef.chan', which is NULL, be dereferenced
in mt76x02_mac_fill_tx_status(), called in the worker.
I think that calling mt76_worker_enable() in mt76u_stop_tx()
may be a fundamental problem in this crash. What I found
is that mt76u_stop_tx() is invoked twice by mt76x0u_stop()
and mt76x0u_cleanup() from mt76x0_disconnect() when
disconnecting the device. In this situation, enabling
kthreads in mt76u_stop_tx() after disabling them will prevents
them from being repeatedly parked, which will return -EBUSY.
If invoking mt76u_stop_tx() in both mt76x0u_stop() and
mt76x0u_cleanup() is unnecessary, and preventing kthreads
from being continuously parked is the only reason of
mt76_worker_enable() in mt76u_stop_tx(), I think we can
make a solution that fundamentally prevent the work from
being called when initialization is failed, instead of
checking the state after the work is called.
---
drivers/net/wireless/mediatek/mt76/sdio.c | 3 ++-
drivers/net/wireless/mediatek/mt76/usb.c | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/sdio.c b/drivers/net/wireless/mediatek/mt76/sdio.c
index 0ec308f99af5..464d1c713554 100644
--- a/drivers/net/wireless/mediatek/mt76/sdio.c
+++ b/drivers/net/wireless/mediatek/mt76/sdio.c
@@ -499,7 +499,8 @@ static void mt76s_tx_status_data(struct work_struct *work)
dev = container_of(sdio, struct mt76_dev, sdio);
while (true) {
- if (test_bit(MT76_REMOVED, &dev->phy.state))
+ if (test_bit(MT76_REMOVED, &dev->phy.state) ||
+ !test_bit(MT76_STATE_INITIALIZED, &dev->phy.state))
break;
if (!dev->drv->tx_status_data(dev, &update))
diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index 4c4033bb1bb3..6cfdaa9d09d1 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -803,7 +803,8 @@ static void mt76u_tx_status_data(struct work_struct *work)
dev = container_of(usb, struct mt76_dev, usb);
while (true) {
- if (test_bit(MT76_REMOVED, &dev->phy.state))
+ if (test_bit(MT76_REMOVED, &dev->phy.state) ||
+ !test_bit(MT76_STATE_INITIALIZED, &dev->phy.state))
break;
if (!dev->drv->tx_status_data(dev, &update))
--
2.25.1
