| Message ID | 20230510100050.27099-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: brcmfmac: Check for probe() id argument being NULL | expand |
On 5/10/2023 12:00 PM, Hans de Goede wrote: > The probe() id argument may be NULL in 2 scenarios: > > 1. brcmf_pcie_pm_leave_D3() calling brcmf_pcie_probe() to reprobe > the device. > > 2. If a user tries to manually bind the driver from sysfs then the sdio / > pcie / usb probe() function gets called with NULL as id argument. > > 1. Is being hit by users causing the following oops on resume and causing > wifi to stop working: > > BUG: kernel NULL pointer dereference, address: 0000000000000018 > <snip> > Hardware name: Dell Inc. XPS 13 9350/0PWNCR, BIDS 1.13.0 02/10/2020 > Workgueue: events_unbound async_run_entry_fn > RIP: 0010:brcmf_pcie_probe+Ox16b/0x7a0 [brcmfmac] > <snip> > Call Trace: > <TASK> > brcmf_pcie_pm_leave_D3+0xc5/8x1a0 [brcmfmac be3b4cefca451e190fa35be8f00db1bbec293887] > ? pci_pm_resume+0x5b/0xf0 > ? pci_legacy_resume+0x80/0x80 > dpm_run_callback+0x47/0x150 > device_resume+0xa2/0x1f0 > async_resume+0x1d/0x30 > <snip> > > Fix this by checking for id being NULL. > > In the case of brcmf_pcie_probe() also add a manual lookup of the id > when it is NULL so that fwvid will still get set correctly on reprobe > on resume. Hi Hans, Thanks for looking into this and coming up with a fix. Does it make sense to proceed with BRCMF_FWVENDOR_INVALID though. I think it will end up in probe failure in brcmf_fwvid_attach() because of it so why not bail out right away. Regards, Arend > Fixes: da6d9c8ecd00 ("wifi: brcmfmac: add firmware vendor info in driver info") > Reported-by: Felix <nimrod4garoa@gmail.com> > Link: https://lore.kernel.org/regressions/4ef3f252ff530cbfa336f5a0d80710020fc5cb1e.camel@gmail.com/ > Cc: Arend van Spriel <arend.vanspriel@broadcom.com> > Cc: stable@vger.kernel.org > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 2 +- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 8 +++++++- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 2 +- > 3 files changed, 9 insertions(+), 3 deletions(-)
Hi Arend, On 5/10/23 14:12, Arend van Spriel wrote: > On 5/10/2023 12:00 PM, Hans de Goede wrote: >> The probe() id argument may be NULL in 2 scenarios: >> >> 1. brcmf_pcie_pm_leave_D3() calling brcmf_pcie_probe() to reprobe >> the device. >> >> 2. If a user tries to manually bind the driver from sysfs then the sdio / >> pcie / usb probe() function gets called with NULL as id argument. >> >> 1. Is being hit by users causing the following oops on resume and causing >> wifi to stop working: >> >> BUG: kernel NULL pointer dereference, address: 0000000000000018 >> <snip> >> Hardware name: Dell Inc. XPS 13 9350/0PWNCR, BIDS 1.13.0 02/10/2020 >> Workgueue: events_unbound async_run_entry_fn >> RIP: 0010:brcmf_pcie_probe+Ox16b/0x7a0 [brcmfmac] >> <snip> >> Call Trace: >> <TASK> >> brcmf_pcie_pm_leave_D3+0xc5/8x1a0 [brcmfmac be3b4cefca451e190fa35be8f00db1bbec293887] >> ? pci_pm_resume+0x5b/0xf0 >> ? pci_legacy_resume+0x80/0x80 >> dpm_run_callback+0x47/0x150 >> device_resume+0xa2/0x1f0 >> async_resume+0x1d/0x30 >> <snip> >> >> Fix this by checking for id being NULL. >> >> In the case of brcmf_pcie_probe() also add a manual lookup of the id >> when it is NULL so that fwvid will still get set correctly on reprobe >> on resume. > > Hi Hans, > > Thanks for looking into this and coming up with a fix. Does it make sense to proceed with > BRCMF_FWVENDOR_INVALID though. I think it will end up in probe failure in brcmf_fwvid_attach() > because of it so why not bail out right away. You are right since the vendor stuff is in separate modules I thought the driver would just continue without, but that indeed does not work. I have just prepared a version 2 which immediately bails from probe() if id is still null after doing a pci_match_id() / usb_match_id() to try and lookup the id. Note for the SDIO case v2 will immediately bail on a NULL id since there is no sdio_match_id() helper. I'll send out v2 right away. Regards, Hans >> Fixes: da6d9c8ecd00 ("wifi: brcmfmac: add firmware vendor info in driver info") >> Reported-by: Felix <nimrod4garoa@gmail.com> >> Link: https://lore.kernel.org/regressions/4ef3f252ff530cbfa336f5a0d80710020fc5cb1e.camel@gmail.com/ >> Cc: Arend van Spriel <arend.vanspriel@broadcom.com> >> Cc: stable@vger.kernel.org >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 2 +- >> drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 8 +++++++- >> drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 2 +- >> 3 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c index 65d4799a5658..833524bc9b50 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c @@ -1067,7 +1067,7 @@ static int brcmf_ops_sdio_probe(struct sdio_func *func, sdiodev->bus_if = bus_if; bus_if->bus_priv.sdio = sdiodev; bus_if->proto_type = BRCMF_PROTO_BCDC; - bus_if->fwvid = id->driver_data; + bus_if->fwvid = id ? id->driver_data : BRCMF_FWVENDOR_INVALID; dev_set_drvdata(&func->dev, bus_if); dev_set_drvdata(&sdiodev->func1->dev, bus_if); sdiodev->dev = &sdiodev->func1->dev; diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c index a9b9b2dc62d4..b29ab19d695d 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c @@ -2339,6 +2339,9 @@ static void brcmf_pcie_debugfs_create(struct device *dev) } #endif +/* Forward declaration for pci_match_id() call */ +static const struct pci_device_id brcmf_pcie_devid_table[]; + static int brcmf_pcie_probe(struct pci_dev *pdev, const struct pci_device_id *id) { @@ -2351,6 +2354,9 @@ brcmf_pcie_probe(struct pci_dev *pdev, const struct pci_device_id *id) brcmf_dbg(PCIE, "Enter %x:%x\n", pdev->vendor, pdev->device); + if (!id) + id = pci_match_id(brcmf_pcie_devid_table, pdev); + ret = -ENOMEM; devinfo = kzalloc(sizeof(*devinfo), GFP_KERNEL); if (devinfo == NULL) @@ -2406,7 +2412,7 @@ brcmf_pcie_probe(struct pci_dev *pdev, const struct pci_device_id *id) bus->bus_priv.pcie = pcie_bus_dev; bus->ops = &brcmf_pcie_bus_ops; bus->proto_type = BRCMF_PROTO_MSGBUF; - bus->fwvid = id->driver_data; + bus->fwvid = id ? id->driver_data : BRCMF_FWVENDOR_INVALID; bus->chip = devinfo->coreid; bus->wowl_supported = pci_pme_capable(pdev, PCI_D3hot); dev_set_drvdata(&pdev->dev, bus); diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c index 246843aeb696..2541a47b1f8f 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c @@ -1425,7 +1425,7 @@ brcmf_usb_probe(struct usb_interface *intf, const struct usb_device_id *id) else brcmf_dbg(USB, "Broadcom full speed USB WLAN interface detected\n"); - ret = brcmf_usb_probe_cb(devinfo, id->driver_info); + ret = brcmf_usb_probe_cb(devinfo, id ? id->driver_info : BRCMF_FWVENDOR_INVALID); if (ret) goto fail;
The probe() id argument may be NULL in 2 scenarios: 1. brcmf_pcie_pm_leave_D3() calling brcmf_pcie_probe() to reprobe the device. 2. If a user tries to manually bind the driver from sysfs then the sdio / pcie / usb probe() function gets called with NULL as id argument. 1. Is being hit by users causing the following oops on resume and causing wifi to stop working: BUG: kernel NULL pointer dereference, address: 0000000000000018 <snip> Hardware name: Dell Inc. XPS 13 9350/0PWNCR, BIDS 1.13.0 02/10/2020 Workgueue: events_unbound async_run_entry_fn RIP: 0010:brcmf_pcie_probe+Ox16b/0x7a0 [brcmfmac] <snip> Call Trace: <TASK> brcmf_pcie_pm_leave_D3+0xc5/8x1a0 [brcmfmac be3b4cefca451e190fa35be8f00db1bbec293887] ? pci_pm_resume+0x5b/0xf0 ? pci_legacy_resume+0x80/0x80 dpm_run_callback+0x47/0x150 device_resume+0xa2/0x1f0 async_resume+0x1d/0x30 <snip> Fix this by checking for id being NULL. In the case of brcmf_pcie_probe() also add a manual lookup of the id when it is NULL so that fwvid will still get set correctly on reprobe on resume. Fixes: da6d9c8ecd00 ("wifi: brcmfmac: add firmware vendor info in driver info") Reported-by: Felix <nimrod4garoa@gmail.com> Link: https://lore.kernel.org/regressions/4ef3f252ff530cbfa336f5a0d80710020fc5cb1e.camel@gmail.com/ Cc: Arend van Spriel <arend.vanspriel@broadcom.com> Cc: stable@vger.kernel.org Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 2 +- drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 8 +++++++- drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 2 +- 3 files changed, 9 insertions(+), 3 deletions(-)