diff mbox series

wifi: nl80211: avoid NULL-ptr deref after cfg80211_cqm_rssi_update

Message ID eb944f1f-8d7c-5057-35f2-34812907e4d1@online.de
State New
Headers show
Series wifi: nl80211: avoid NULL-ptr deref after cfg80211_cqm_rssi_update | expand

Commit Message

Max Schulze Aug. 11, 2023, 7:30 a.m. UTC 
In cfg80211_cqm_rssi_notify, when calling cfg80211_cqm_rssi_update, this might free
the wdev->cqm_config . Check for this when it returns.

This has been observed on brcmfmac, when a RSSI event is generated just right
after disconnecting from AP. Then probing for STA details returns nothing, as
evidenced i.e. by
"ieee80211 phy0: brcmf_cfg80211_get_station: GET STA INFO failed, -52".


Signed-off-by: Max Schulze <max.schulze@online.de>
Tested-by: Max Schulze <max.schulze@online.de>
Link: https://lore.kernel.org/linux-wireless/bc3bf8f6-7ad7-bf69-9227-f972dac4e66b@online.de/
---

I have deployed this to 22 systems without issues and eliminating those null-ptr deref.

Example Trace from Problem:

wpa_supplicant[332]: wlan0: CTRL-EVENT-DISCONNECTED bssid=XX:XX:XX:XX:74:1f reason=3 locally_generated=1
brcmfmac: brcmf_rx_event Enter: mmc1:0001:1: rxp=0000000017163222
brcmfmac: brcmf_fweh_event_worker event LINK (16) ifidx 0 bsscfg 0 addr xx:xx:xx:xx:74:1f
brcmfmac: brcmf_fweh_event_worker   version 2 flags 0 status 0 reason 2
brcmutil: event payload, len=0
brcmfmac: brcmf_is_linkdown Processing link down
brcmfmac: brcmf_notify_connect_status Linkdown
brcmfmac: brcmf_rx_event Enter: mmc1:0001:1: rxp=00000000dcf7c0c0
brcmfmac: brcmf_fweh_event_worker event RSSI (56) ifidx 0 bsscfg 0 addr 00:00:xx:xx:00:50
brcmfmac: brcmf_fweh_event_worker   version 2 flags 0 status 0 reason 0
brcmutil: event payload, len=12
00000000: 00 00 00 00 00 00 00 00 00 00 00 00              ............
brcmfmac: brcmf_notify_rssi LOW rssi=0
brcmfmac: brcmf_cfg80211_del_key key index (0)
brcmfmac: brcmf_cfg80211_del_key Ignore clearing of (never configured) key
brcmfmac: brcmf_fil_cmd_data Firmware error: BCME_NOTFOUND (-30)
brcmfmac: brcmf_fil_iovar_data_get ifidx=0, name=tdls_sta_info, len=296, err=-52
brcmfmac: brcmf_fil_cmd_data Firmware error: BCME_BADADDR (-21)
brcmfmac: brcmf_fil_iovar_data_get ifidx=0, name=sta_info, len=296, err=-52
ieee80211 phy0: brcmf_cfg80211_get_station: GET STA INFO failed, -52
==================================================================
BUG: KASAN: null-ptr-deref in cfg80211_cqm_rssi_notify (/home/r/linux/net/wireless/nl80211.c:19089) cfg80211


 net/wireless/nl80211.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Max Schulze Aug. 12, 2023, 9:35 a.m. UTC | #1 
Am 11.08.23 um 11:54 schrieb Johannes Berg:
> On Fri, 2023-08-11 at 09:30 +0200, Max Schulze wrote:
>> In cfg80211_cqm_rssi_notify, when calling cfg80211_cqm_rssi_update, this might free
>> the wdev->cqm_config . Check for this when it returns.
> 
> That doesn't seem right? How does cfg80211_cqm_rssi_update() free it?
> 


You are probably right, that it doesn't. I amended the original bug report where I have a trace of the free and it is not a descendant of _rssi_update().



>> This has been observed on brcmfmac, when a RSSI event is generated just right
>> after disconnecting from AP. Then probing for STA details returns nothing, as
>> evidenced i.e. by
>> "ieee80211 phy0: brcmf_cfg80211_get_station: GET STA INFO failed, -52".
> 
> I think the issue then isn't that this frees it but rather than a free
> of it races with the reporting?


I do not understand what you mean. I think there is a race, yes, somewhere else it get's freed, and when we access it, there is the null-ptr-deref.
The GET STA INFO failed is just the expected answer from the chip, because it currently is not associated with any station.


> 
>> --- a/net/wireless/nl80211.c
>> +++ b/net/wireless/nl80211.c
>> @@ -19088,7 +19088,7 @@ void cfg80211_cqm_rssi_notify(struct net_device *dev,
>>  
>>  		cfg80211_cqm_rssi_update(rdev, dev);
>>  
>> -		if (rssi_level == 0)
>> +		if (rssi_level == 0 && wdev->cqm_config)
>>  			rssi_level = wdev->cqm_config->last_rssi_event_value;
>>
> 
> But if it's a race, then this isn't actually going to really fix the
> issue, rather it just makes it (much) less likely.
> 

You have been right with your prediction. It made it around ~15x less likely but I have recorded a trace. It just has the null-ptr somewhere else.

> Since we can probably neither lock the wdev here nor require calls to
> this function with wdev lock held, it looks like we need to protect the
> pointer with RCU instead?
> 

I do not know how to do that and need help.

Max

[this is with my patch some lines above, no more crashes in _rssi_notify, but now somewhere else ].

13:15:03: brcmfmac: brcmf_is_linkdown Processing link down
13:15:03: brcmfmac: brcmf_notify_connect_status Linkdown
13:15:03: brcmfmac: brcmf_fweh_event_worker event RSSI (56) ifidx 0 bsscfg 0 addr xx:xx:xx:xx:00:50
13:15:03: brcmfmac: brcmf_fweh_event_worker   version 2 flags 0 status 0 reason 0
13:15:03: brcmutil: event payload, len=12
13:15:03: 00000000: 00 00 00 00 00 00 00 00 00 00 00 00              ............
13:15:03: brcmfmac: brcmf_notify_rssi LOW rssi=0
13:15:03: brcmfmac: brcmf_fil_iovar_data_set ifidx=0, name=rssi_event, len=16
13:15:03: Unable to handle kernel NULL pointer dereference at virtual address 000000000000000c
13:15:03: brcmutil: data
13:15:03: Mem abort info:
13:15:03: 00000000: 00 00 00 00 03 00 00 7f 00 00 00 00 00 00 00 00  ................
13:15:03:   ESR = 0x0000000096000005
13:15:03:   EC = 0x25: DABT (current EL), IL = 32 bits
13:15:03:   SET = 0, FnV = 0
13:15:03:   EA = 0, S1PTW = 0
13:15:03:   FSC = 0x05: level 1 translation fault
13:15:03: Data abort info:
13:15:03:   ISV = 0, ISS = 0x00000005, ISS2 = 0x00000000
13:15:03:   CM = 0, WnR = 0, TnD = 0, TagAccess = 0
13:15:03:   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
13:15:03: user pgtable: 4k pages, 39-bit VAs, pgdp=00000000456cf000
13:15:03: [000000000000000c] pgd=0000000000000000, p4d=0000000000000000, pud=0000000000000000
13:15:03: Internal error: Oops: 0000000096000005 [#1] PREEMPT SMP
13:15:03: Modules linked in: brcmfmac_wcc vc4 brcmfmac snd_soc_hdmi_codec snd_soc_core snd_pcm_dmaengine snd_pcm cfg80211 rpivid_hevc(C) bcm2835_isp(C) snd_timer bcm2835_mmal_vchiq(C) snd videobuf2_dma_contig videobuf2_memops v4l2_mem2mem xt_tcpudp rtc_pcf85063 nft_compat regmap_i2c nf_tables videobuf2_v4l2 v3d videodev drm_display_helper drm_shmem_helper nfnetlink drm_dma_helper gpu_sched i2c_mux_pinctrl i2c_mux gpio_keys drm_kms_helper raspberrypi_hwmon rfkill hid_microsoft joydev i2c_brcmstb ff_memless brcmutil i2c_bcm2835 videobuf2_common vc_sm_cma(C) mc cec nvmem_rmem uio_pdrv_genirq uio drm fuse drm_panel_orientation_quirks backlight ip_tables x_tables ipv6
13:15:04: CPU: 1 PID: 57 Comm: kworker/1:2 Tainted: G         C         6.5.0-rc5-v8-g963e8f68ce3b #4
13:15:04: Hardware name: Raspberry Pi Compute Module 4 Rev 1.0 (DT)
13:15:04: Workqueue: events brcmf_fweh_event_worker [brcmfmac]
13:15:04: pstate: 80000005 (Nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
13:15:04: pc : cfg80211_cqm_rssi_update (/home/r/linux/net/wireless/nl80211.c:12838) cfg80211
13:15:04: lr : cfg80211_cqm_rssi_notify (/home/r/linux/net/wireless/nl80211.c:19088) cfg80211
13:15:04: sp : ffffffc08060baf0
13:15:04: x29: ffffffc08060baf0 x28: ffffff8052b798f0 x27: ffffff8052b798a0
13:15:04: x26: ffffff8055d4c8c0 x25: ffffff8055d4ca68 x24: ffffff8055d4c3c0
13:15:04: x23: ffffff8044a52008 x22: 0000000000000000 x21: ffffff8055d4c000
13:15:04: x20: ffffff8044a63000 x19: ffffff8044a52008 x18: 00000000fffffffe
13:15:04: x17: 2e2e2e2e2e202020 x16: ffffffddb73c9498 x15: 0000000000000020
13:15:04: x14: 0000000000000000 x13: 303d697373722057 x12: 4f4c20697373725f
13:15:04: x11: fffffffffffe39f8 x10: ffffff804018c700 x9 : ffffffdd39d20660
13:15:04: x8 : 000000005108ccd0 x7 : 0000000000000000 x6 : 000000005108ccd0
13:15:04: x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000000
13:15:04: x2 : 0000000000000000 x1 : 0000000000000000 x0 : 0000000000000004
13:15:04: Call trace:
13:15:04: cfg80211_cqm_rssi_update (/home/r/linux/net/wireless/nl80211.c:12838) cfg80211
13:15:04: cfg80211_cqm_rssi_notify (/home/r/linux/net/wireless/nl80211.c:19088) cfg80211
13:15:04: brcmf_notify_rssi (/home/r/linux/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:6651) brcmfmac
13:15:04: brcmf_fweh_call_event_handler (/home/r/linux/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c:116) brcmfmac
13:15:04: brcmf_fweh_event_worker (/home/r/linux/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c:268) brcmfmac
13:15:04: process_one_work (/home/r/linux/./arch/arm64/include/asm/jump_label.h:21 /home/r/linux/./include/linux/jump_label.h:207 /home/r/linux/./include/trace/events/workqueue.h:108 /home/r/linux/kernel/workqueue.c:2602) 
13:15:04: worker_thread (/home/r/linux/./include/linux/list.h:292 /home/r/linux/kernel/workqueue.c:2749) 
13:15:04: kthread (/home/r/linux/kernel/kthread.c:389) 
13:15:04: ret_from_fork (/home/r/linux/arch/arm64/kernel/entry.S:854) 
13:15:04: Code: f941b264 0a020062 93407c45 8b22c883 (b9400c63)
All code
========
   0:	f941b264 	ldr	x4, [x19, #864]
   4:	0a020062 	and	w2, w3, w2
   8:	93407c45 	sxtw	x5, w2
   c:	8b22c883 	add	x3, x4, w2, sxtw #2
  10:*	b9400c63 	ldr	w3, [x3, #12]		<-- trapping instruction

Code starting with the faulting instruction
===========================================
   0:	b9400c63 	ldr	w3, [x3, #12]
13:15:04: ---[ end trace 0000000000000000 ]---
13:15:04: brcmfmac: brcmf_fil_iovar_data_get ifidx=0, name=qtxpower, len=4, err=0
13:15:04: brcmutil: data
diff mbox series

Patch

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 8bcf8e293..b12424382 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -19088,7 +19088,7 @@  void cfg80211_cqm_rssi_notify(struct net_device *dev,
 
 		cfg80211_cqm_rssi_update(rdev, dev);
 
-		if (rssi_level == 0)
+		if (rssi_level == 0 && wdev->cqm_config)
 			rssi_level = wdev->cqm_config->last_rssi_event_value;
 	}