| Message ID | 20230510141856.46532-1-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] wifi: brcmfmac: Check for probe() id argument being NULL | expand |
Hi, On 5/10/23 16:18, 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 PCI and USB cases try a manual lookup of the id so that manually > binding the driver through sysfs and more importantly brcmf_pcie_probe() > on resume will work. > > For the SDIO case there is no helper to do a manual sdio_device_id lookup, > so just directly error out on a NULL id there. > > 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> This is now also: Tested by: Nimrod MacIomhair <nimrod4garoa@gmail.com> Can we get this regression fix merged please ? Regards, Hans > --- > Changes in v2: > - Using BRCMF_FWVENDOR_INVALID will just lead to a probe() error later on, > if NULL error out immediately instead of using BRCMF_FWVENDOR_INVALID. > --- > .../net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 5 +++++ > .../net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 11 +++++++++++ > .../net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 11 +++++++++++ > 3 files changed, 27 insertions(+) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > index 65d4799a5658..f06684f84789 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > @@ -1032,6 +1032,11 @@ static int brcmf_ops_sdio_probe(struct sdio_func *func, > struct brcmf_sdio_dev *sdiodev; > struct brcmf_bus *bus_if; > > + if (!id) { > + dev_err(&func->dev, "Error no sdio_device_id passed for %x:%x\n", func->vendor, func->device); > + return -ENODEV; > + } > + > brcmf_dbg(SDIO, "Enter\n"); > brcmf_dbg(SDIO, "Class=%x\n", func->class); > brcmf_dbg(SDIO, "sdio vendor ID: 0x%04x\n", func->vendor); > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > index a9b9b2dc62d4..d9ecaa0cfdc4 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) > { > @@ -2349,6 +2352,14 @@ brcmf_pcie_probe(struct pci_dev *pdev, const struct pci_device_id *id) > struct brcmf_core *core; > struct brcmf_bus *bus; > > + if (!id) { > + id = pci_match_id(brcmf_pcie_devid_table, pdev); > + if (!id) { > + pci_err(pdev, "Error could not find pci_device_id for %x:%x\n", pdev->vendor, pdev->device); > + return -ENODEV; > + } > + } > + > brcmf_dbg(PCIE, "Enter %x:%x\n", pdev->vendor, pdev->device); > > ret = -ENOMEM; > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c > index 246843aeb696..2178675ae1a4 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c > @@ -1331,6 +1331,9 @@ brcmf_usb_disconnect_cb(struct brcmf_usbdev_info *devinfo) > brcmf_usb_detach(devinfo); > } > > +/* Forward declaration for usb_match_id() call */ > +static const struct usb_device_id brcmf_usb_devid_table[]; > + > static int > brcmf_usb_probe(struct usb_interface *intf, const struct usb_device_id *id) > { > @@ -1342,6 +1345,14 @@ brcmf_usb_probe(struct usb_interface *intf, const struct usb_device_id *id) > u32 num_of_eps; > u8 endpoint_num, ep; > > + if (!id) { > + id = usb_match_id(intf, brcmf_usb_devid_table); > + if (!id) { > + dev_err(&intf->dev, "Error could not find matching usb_device_id\n"); > + return -ENODEV; > + } > + } > + > brcmf_dbg(USB, "Enter 0x%04x:0x%04x\n", id->idVendor, id->idProduct); > > devinfo = kzalloc(sizeof(*devinfo), GFP_ATOMIC);
On 5/10/2023 4:18 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 PCI and USB cases try a manual lookup of the id so that manually > binding the driver through sysfs and more importantly brcmf_pcie_probe() > on resume will work. > > For the SDIO case there is no helper to do a manual sdio_device_id lookup, > so just directly error out on a NULL id there. > > 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> You can change this to: Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Cc: stable@vger.kernel.org > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Changes in v2: > - Using BRCMF_FWVENDOR_INVALID will just lead to a probe() error later on, > if NULL error out immediately instead of using BRCMF_FWVENDOR_INVALID. > --- > .../net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 5 +++++ > .../net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 11 +++++++++++ > .../net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 11 +++++++++++ > 3 files changed, 27 insertions(+)
Hans de Goede <hdegoede@redhat.com> 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 PCI and USB cases try a manual lookup of the id so that manually > binding the driver through sysfs and more importantly brcmf_pcie_probe() > on resume will work. > > For the SDIO case there is no helper to do a manual sdio_device_id lookup, > so just directly error out on a NULL id there. > > 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: stable@vger.kernel.org > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> Patch applied to wireless.git, thanks. 60fc756fc8e6 wifi: brcmfmac: Check for probe() id argument being NULL
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c index 65d4799a5658..f06684f84789 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c @@ -1032,6 +1032,11 @@ static int brcmf_ops_sdio_probe(struct sdio_func *func, struct brcmf_sdio_dev *sdiodev; struct brcmf_bus *bus_if; + if (!id) { + dev_err(&func->dev, "Error no sdio_device_id passed for %x:%x\n", func->vendor, func->device); + return -ENODEV; + } + brcmf_dbg(SDIO, "Enter\n"); brcmf_dbg(SDIO, "Class=%x\n", func->class); brcmf_dbg(SDIO, "sdio vendor ID: 0x%04x\n", func->vendor); diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c index a9b9b2dc62d4..d9ecaa0cfdc4 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) { @@ -2349,6 +2352,14 @@ brcmf_pcie_probe(struct pci_dev *pdev, const struct pci_device_id *id) struct brcmf_core *core; struct brcmf_bus *bus; + if (!id) { + id = pci_match_id(brcmf_pcie_devid_table, pdev); + if (!id) { + pci_err(pdev, "Error could not find pci_device_id for %x:%x\n", pdev->vendor, pdev->device); + return -ENODEV; + } + } + brcmf_dbg(PCIE, "Enter %x:%x\n", pdev->vendor, pdev->device); ret = -ENOMEM; diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c index 246843aeb696..2178675ae1a4 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c @@ -1331,6 +1331,9 @@ brcmf_usb_disconnect_cb(struct brcmf_usbdev_info *devinfo) brcmf_usb_detach(devinfo); } +/* Forward declaration for usb_match_id() call */ +static const struct usb_device_id brcmf_usb_devid_table[]; + static int brcmf_usb_probe(struct usb_interface *intf, const struct usb_device_id *id) { @@ -1342,6 +1345,14 @@ brcmf_usb_probe(struct usb_interface *intf, const struct usb_device_id *id) u32 num_of_eps; u8 endpoint_num, ep; + if (!id) { + id = usb_match_id(intf, brcmf_usb_devid_table); + if (!id) { + dev_err(&intf->dev, "Error could not find matching usb_device_id\n"); + return -ENODEV; + } + } + brcmf_dbg(USB, "Enter 0x%04x:0x%04x\n", id->idVendor, id->idProduct); devinfo = kzalloc(sizeof(*devinfo), GFP_ATOMIC);
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 PCI and USB cases try a manual lookup of the id so that manually binding the driver through sysfs and more importantly brcmf_pcie_probe() on resume will work. For the SDIO case there is no helper to do a manual sdio_device_id lookup, so just directly error out on a NULL id there. 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> --- Changes in v2: - Using BRCMF_FWVENDOR_INVALID will just lead to a probe() error later on, if NULL error out immediately instead of using BRCMF_FWVENDOR_INVALID. --- .../net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 5 +++++ .../net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 11 +++++++++++ .../net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 11 +++++++++++ 3 files changed, 27 insertions(+)