| Message ID | 20250106103749.5764-1-stefan@codeweavers.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: brcmfmac: Check the return value of of_property_read_string_index | expand |
On 1/6/2025 11:37 AM, Stefan Dösinger wrote: > Somewhen between 6.10 and 6.11 the driver started to crash on my > MacBookPro14,3. The property doesn't exist and 'tmp' remains > uninitialized, so we pass a random pointer to devm_kstrdup(). By the looks of it this is an intel-based platform. Is that correct? So does it have a devicetree? I would expect the root node find to fail, but apparently is does not. Strange though that root node does not have a compatible property. Anyway, the analysis looks sane so ... minor remark below. Acked-by: Arend van Spriel > Signed-off-by: Stefan Dösinger <stefan@codeweavers.com> > [...] > --- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c > index c1f18e2fe540..ee589a7b4f4f 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c > @@ -99,13 +99,15 @@ int brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type, > /* Set board-type to the first string of the machine compatible prop */ > root = of_find_node_by_path("/"); > if (root && err) { > - char *board_type; > + char *board_type = NULL; > const char *tmp; > > - of_property_read_string_index(root, "compatible", 0, &tmp); > + err = of_property_read_string_index(root, "compatible", 0, &tmp); > > /* get rid of '/' in the compatible string to be able to find the FW */ > - board_type = devm_kstrdup(dev, tmp, GFP_KERNEL); No need to use 'err'. You can directly do of_property_read_string_index() in the if statement below. > + if (!err) > + board_type = devm_kstrdup(dev, tmp, GFP_KERNEL); > + > if (!board_type) { > of_node_put(root); > return 0;
Am Montag, 6. Januar 2025, 14:02:17 Ostafrikanische Zeit schrieb Arend van Spriel: > On 1/6/2025 11:37 AM, Stefan Dösinger wrote: > > Somewhen between 6.10 and 6.11 the driver started to crash on my > > MacBookPro14,3. The property doesn't exist and 'tmp' remains > > uninitialized, so we pass a random pointer to devm_kstrdup(). > > By the looks of it this is an intel-based platform. Is that correct? So > does it have a devicetree? I would expect the root node find to fail, > but apparently is does not. Strange though that root node does not have > a compatible property. Anyway, the analysis looks sane so ... Yes, this is an Intel based MacBook Pro - the 2017 version. I was curious about the same thing and tried to find out where it expects to get those properties from. I didn't find a definitive answer and concluded that it reads the properties from somewhere on the wifi cards ROM rather than the computer's firmware / ACPI Tabes / whatever. If you can tell me where I should look I can see if I find out more. If you think it is helpful or might point towards deeper issues I can try do a bisect for whatever patch broke this. I vaguely suspect though that it was always broken but by random luck a NULL pointer happened to be on the stack in the right place. > No need to use 'err'. You can directly do > of_property_read_string_index() in the if statement below. Check, I'll resend. I found both styles in use (though admittedly reusing 'err' to print it to dmesg).
Hello Arend, Am Montag, 6. Januar 2025, 14:22:29 Ostafrikanische Zeit schrieb Stefan Dösinger: > Am Montag, 6. Januar 2025, 14:02:17 Ostafrikanische Zeit schrieb Arend van > > Spriel: > > On 1/6/2025 11:37 AM, Stefan Dösinger wrote: > > > Somewhen between 6.10 and 6.11 the driver started to crash on my > > > MacBookPro14,3. The property doesn't exist and 'tmp' remains > > > uninitialized, so we pass a random pointer to devm_kstrdup(). > > > > By the looks of it this is an intel-based platform. Is that correct? So > > does it have a devicetree? I would expect the root node find to fail, > > but apparently is does not. Strange though that root node does not have > > a compatible property. Anyway, the analysis looks sane so ... > > Yes, this is an Intel based MacBook Pro - the 2017 version. I have an updated theory why the codepath was entered: My kernel config had CONFIG_OF (and CONFIG_OF_OVERLAY) enabled. I did not provide any DTBs on boot, but this configuration apparently resulted in an empty root node being found. I also see an empty (0 byte) /proc/device-tree/name file. With CONFIG_OF=n the of.c file isn't compiled in the first place. I think we still want to patch the code. While enabling this option on standard x86 is arguably wrong, the driver shouldn't crash because of it. I don't know where CONFIG_OF=y came from. This is a kernel configuration grown over 15 years. I might have accidentally enabled it in a "make oldconfig" run, or I enabled it 'just in case' without knowing what I was doing - this particular Linux installation is on a USB drive that I plug into many different x86_64 computers, so I enabled pretty much every driver (as a module if possible).
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c index c1f18e2fe540..ee589a7b4f4f 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c @@ -99,13 +99,15 @@ int brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type, /* Set board-type to the first string of the machine compatible prop */ root = of_find_node_by_path("/"); if (root && err) { - char *board_type; + char *board_type = NULL; const char *tmp; - of_property_read_string_index(root, "compatible", 0, &tmp); + err = of_property_read_string_index(root, "compatible", 0, &tmp); /* get rid of '/' in the compatible string to be able to find the FW */ - board_type = devm_kstrdup(dev, tmp, GFP_KERNEL); + if (!err) + board_type = devm_kstrdup(dev, tmp, GFP_KERNEL); + if (!board_type) { of_node_put(root); return 0;
Somewhen between 6.10 and 6.11 the driver started to crash on my MacBookPro14,3. The property doesn't exist and 'tmp' remains uninitialized, so we pass a random pointer to devm_kstrdup(). Signed-off-by: Stefan Dösinger <stefan@codeweavers.com> --- The crash I am getting looks like this: BUG: unable to handle page fault for address: 00007f033c669379 PF: supervisor read access in kernel mode PF: error_code(0x0001) - permissions violation PGD 8000000101341067 P4D 8000000101341067 PUD 101340067 PMD 1013bb067 PTE 800000010aee9025 Oops: Oops: 0001 [#1] SMP PTI CPU: 4 UID: 0 PID: 827 Comm: (udev-worker) Not tainted 6.11.8-gentoo #1 Hardware name: Apple Inc. MacBookPro14,3/Mac-551B86E5744E2388, BIOS 529.140.2.0.0 06/23/2024 RIP: 0010:strlen+0x4/0x30 Code: f7 75 ec 31 c0 c3 cc cc cc cc 48 89 f8 c3 cc cc cc cc 0f 1f 40 00 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 f3 0f 1e fa <80> 3f 00 74 14 48 89 f8 48 83 c0 01 80 38 00 75 f7 48 29 f8 c3 cc RSP: 0018:ffffb4aac0683ad8 EFLAGS: 00010202 RAX: 00000000ffffffea RBX: 00007f033c669379 RCX: 0000000000000001 RDX: 0000000000000cc0 RSI: 00007f033c669379 RDI: 00007f033c669379 RBP: 00000000ffffffea R08: 0000000000000000 R09: 00000000c0ba916a R10: ffffffffffffffff R11: ffffffffb61ea260 R12: ffff91f7815b50c8 R13: 0000000000000cc0 R14: ffff91fafefffe30 R15: ffffb4aac0683b30 FS: 00007f033ccbe8c0(0000) GS:ffff91faeed00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007f033c669379 CR3: 0000000107b1e004 CR4: 00000000003706f0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: <TASK> ? __die+0x23/0x70 ? page_fault_oops+0x149/0x4c0 ? raw_spin_rq_lock_nested+0xe/0x20 ? sched_balance_newidle+0x22b/0x3c0 ? update_load_avg+0x78/0x770 ? exc_page_fault+0x6f/0x150 ? asm_exc_page_fault+0x26/0x30 ? __pfx_pci_conf1_write+0x10/0x10 ? strlen+0x4/0x30 devm_kstrdup+0x25/0x70 brcmf_of_probe+0x273/0x350 [brcmfmac] --- drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)