Skip to contentSkip to conversation

brcmfmac: Fixes potential buffer overflow in 'brcmf_fweh_event_worker'

PATCH Superseded Dokyung Song 2 participants 3 messages Oct 20, 2022 Broadcom BRCM80211 IEEE802.11 wireless drivers

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 brcmfmac: Fixes potential buffer overflow in 'brcmf_fweh_event_worker'

This patch fixes an intra-object buffer overflow in brcmfmac that occurs when the device provides a 'bsscfgidx' equal to or greater than the buffer size. The patch adds a check that leads to a safe failure if that is the case.

Found by a modified version of syzkaller.

UBSAN: array-index-out-of-bounds in drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
index 52 is out of range for type 'brcmf_if *[16]'
CPU: 0 PID: 1898 Comm: kworker/0:2 Tainted: G           O      5.14.0+ #132
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.12.1-0-ga5cab58e9a3f-prebuilt.qemu.org 04/01/2014
Workqueue: events brcmf_fweh_event_worker
Call Trace:
 dump_stack_lvl+0x57/0x7d
 ubsan_epilogue+0x5/0x40
 __ubsan_handle_out_of_bounds+0x69/0x80
 ? memcpy+0x39/0x60
 brcmf_fweh_event_worker+0xae1/0xc00
 ? brcmf_fweh_call_event_handler.isra.0+0x100/0x100
 ? rcu_read_lock_sched_held+0xa1/0xd0
 ? rcu_read_lock_bh_held+0xb0/0xb0
 ? lockdep_hardirqs_on_prepare+0x273/0x3e0
 process_one_work+0x873/0x13e0
 ? lock_release+0x640/0x640
 ? pwq_dec_nr_in_flight+0x320/0x320
 ? rwlock_bug.part.0+0x90/0x90
 worker_thread+0x8b/0xd10
 ? __kthread_parkme+0xd9/0x1d0
 ? process_one_work+0x13e0/0x13e0
 kthread+0x379/0x450
 ? _raw_spin_unlock_irq+0x24/0x30
 ? set_kthread_struct+0x100/0x100
 ret_from_fork+0x1f/0x30
================================================================================
general protection fault, probably for non-canonical address 0xe5601c0020023fff: 0000 [#1] SMP KASAN
KASAN: maybe wild-memory-access in range [0x2b0100010011fff8-0x2b0100010011ffff]
CPU: 0 PID: 1898 Comm: kworker/0:2 Tainted: G           O      5.14.0+ #132
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.12.1-0-ga5cab58e9a3f-prebuilt.qemu.org 04/01/2014
Workqueue: events brcmf_fweh_event_worker
RIP: 0010:brcmf_fweh_call_event_handler.isra.0+0x42/0x100
Code: 89 f5 53 48 89 fb 48 83 ec 08 e8 79 0b 38 fe 48 85 ed 74 7e e8 6f 0b 38 fe 48 89 ea 48 b8 00 00 00 00 00 fc ff df 48 c1 ea 03 <80> 3c 02 00 0f 85 8b 00 00 00 4c 8b 7d 00 44 89 e0 48 ba 00 00 00
RSP: 0018:ffffc9000259fbd8 EFLAGS: 00010207
RAX: dffffc0000000000 RBX: ffff888115d8cd50 RCX: 0000000000000000
RDX: 0560200020023fff RSI: ffffffff8304bc91 RDI: ffff888115d8cd50
RBP: 2b0100010011ffff R08: ffff888112340050 R09: ffffed1023549809
R10: ffff88811aa4c047 R11: ffffed1023549808 R12: 0000000000000045
R13: ffffc9000259fca0 R14: ffff888112340050 R15: ffff888112340000
FS:  0000000000000000(0000) GS:ffff88811aa00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000000004053ccc0 CR3: 0000000112740000 CR4: 0000000000750ef0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
PKRU: 55555554
Call Trace:
 brcmf_fweh_event_worker+0x117/0xc00
 ? brcmf_fweh_call_event_handler.isra.0+0x100/0x100
 ? rcu_read_lock_sched_held+0xa1/0xd0
 ? rcu_read_lock_bh_held+0xb0/0xb0
 ? lockdep_hardirqs_on_prepare+0x273/0x3e0
 process_one_work+0x873/0x13e0
 ? lock_release+0x640/0x640
 ? pwq_dec_nr_in_flight+0x320/0x320
 ? rwlock_bug.part.0+0x90/0x90
 worker_thread+0x8b/0xd10
 ? __kthread_parkme+0xd9/0x1d0
 ? process_one_work+0x13e0/0x13e0
 kthread+0x379/0x450
 ? _raw_spin_unlock_irq+0x24/0x30
 ? set_kthread_struct+0x100/0x100
 ret_from_fork+0x1f/0x30
Modules linked in: 88XXau(O) 88x2bu(O)
---[ end trace 41d302138f3ff55a ]---
RIP: 0010:brcmf_fweh_call_event_handler.isra.0+0x42/0x100
Code: 89 f5 53 48 89 fb 48 83 ec 08 e8 79 0b 38 fe 48 85 ed 74 7e e8 6f 0b 38 fe 48 89 ea 48 b8 00 00 00 00 00 fc ff df 48 c1 ea 03 <80> 3c 02 00 0f 85 8b 00 00 00 4c 8b 7d 00 44 89 e0 48 ba 00 00 00
RSP: 0018:ffffc9000259fbd8 EFLAGS: 00010207
RAX: dffffc0000000000 RBX: ffff888115d8cd50 RCX: 0000000000000000
RDX: 0560200020023fff RSI: ffffffff8304bc91 RDI: ffff888115d8cd50
RBP: 2b0100010011ffff R08: ffff888112340050 R09: ffffed1023549809
R10: ffff88811aa4c047 R11: ffffed1023549808 R12: 0000000000000045
R13: ffffc9000259fca0 R14: ffff888112340050 R15: ffff888112340000
FS:  0000000000000000(0000) GS:ffff88811aa00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000000004053ccc0 CR3: 0000000112740000 CR4: 0000000000750ef0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
PKRU: 55555554
Kernel panic - not syncing: Fatal exception
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c +7 -2
index bc3f4e4edcdf..e035e9c5a1fa 100644
@@ -255,10 +255,15 @@ static void brcmf_fweh_event_worker(struct work_struct *work)
 			goto event_free;
 		}
 
-		if (event->code == BRCMF_E_TDLS_PEER_EVENT)
+		if (event->code == BRCMF_E_TDLS_PEER_EVENT) {
 			ifp = drvr->iflist[0];
-		else
+		} else {
+			if (emsg.bsscfgidx >= BRCMF_MAX_IFS) {
+				bphy_err(drvr, "invalid bsscfg index: %u\n", emsg.bsscfgidx);
+				goto event_free;
+			}
 			ifp = drvr->iflist[emsg.bsscfgidx];
+		}
 		err = brcmf_fweh_call_event_handler(drvr, ifp, event->code,
 						    &emsg, event->data);
 		if (err) {
Signature · 1 line
2.25.1
Reviewed-by on v1 1/1
#1 lore raw
Quoted 10 lines · Dokyung Song

This patch fixes an intra-object buffer overflow in brcmfmac that occurs when the device provides a 'bsscfgidx' equal to or greater than the buffer size. The patch adds a check that leads to a safe failure if that is the case.

Found by a modified version of syzkaller.

UBSAN: array-index-out-of-bounds in drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
index 52 is out of range for type 'brcmf_if *[16]'
CPU: 0 PID: 1898 Comm: kworker/0:2 Tainted: G           O      5.14.0+ #132

[...]

Quoted 5 lines
Kernel panic - not syncing: Fatal exception

Not sure what the rules are for using 'Reported-by' tag, but it looks a bit odd. I leave it to the wireless maintainer.

Quoted 12 lines
---
  drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c | 9 +++++++--
  1 file changed, 7 insertions(+), 2 deletions(-)
drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c +0 -0
index bc3f4e4edcdf..e035e9c5a1fa 100644
@@ -255,10 +255,15 @@ static void brcmf_fweh_event_worker(struct work_struct *work)
  			goto event_free;
  		}

drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c static void brcmf_fweh_event_worker(struct work_struct *work) · line 264

Quoted 10 lines
drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c +6 -2
 
-		if (event->code == BRCMF_E_TDLS_PEER_EVENT)
+		if (event->code == BRCMF_E_TDLS_PEER_EVENT) {
  			ifp = drvr->iflist[0];
-		else
+		} else {
+			if (emsg.bsscfgidx >= BRCMF_MAX_IFS) {
+				bphy_err(drvr, "invalid bsscfg index: %u\n", emsg.bsscfgidx);
+				goto event_free;
+			}

probably better to do the validation before any other handling. So right after converting the event message at line 245

https://elixir.bootlin.com/linux/latest/source/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c#L245

Quoted 5 lines
drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c +1 -0
  			ifp = drvr->iflist[emsg.bsscfgidx];
+		}
  		err = brcmf_fweh_call_event_handler(drvr, ifp, event->code,
  						    &emsg, event->data);
  		if (err) {
#2 lore raw
Quoted 11 lines · Arend Van Spriel
Quoted 7 lines

-        else
+        } else {
+            if (emsg.bsscfgidx >= BRCMF_MAX_IFS) { +                bphy_err(drvr, "invalid bsscfg index: %u\n", emsg.bsscfgidx);
+                goto event_free;
+            }

probably better to do the validation before any other handling. So right after converting the event message at line 245

https://elixir.bootlin.com/linux/latest/source/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c#L245

Actually, if we ignore the event message we can avoid the whole conversion as well doing the validation before the convert.

Regards,
Arend