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 <[email protected]>
Link: https://lore.kernel.org/regressions/[email protected]/
Cc: Arend van Spriel <[email protected]>
Cc: [email protected]
Signed-off-by: Hans de Goede <[email protected]>
---
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);
--
2.40.1
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 <[email protected]>
> Link: https://lore.kernel.org/regressions/[email protected]/
> Cc: Arend van Spriel <[email protected]>
> Cc: [email protected]
> Signed-off-by: Hans de Goede <[email protected]>
This is now also:
Tested by: Nimrod MacIomhair <[email protected]>
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 <[email protected]>
> Link: https://lore.kernel.org/regressions/[email protected]/
> Cc: Arend van Spriel <[email protected]>
You can change this to:
Reviewed-by: Arend van Spriel <[email protected]>
> Cc: [email protected]
> Signed-off-by: Hans de Goede <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]>
> Link: https://lore.kernel.org/regressions/[email protected]/
> Cc: [email protected]
> Signed-off-by: Hans de Goede <[email protected]>
> Reviewed-by: Arend van Spriel <[email protected]>
Patch applied to wireless.git, thanks.
60fc756fc8e6 wifi: brcmfmac: Check for probe() id argument being NULL
--
https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches