Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b2f19e4b-7d33-695d-4a7a-e4aabe5d2d99@redhat.com>
Date:   Sat, 13 May 2023 21:48:08 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH REGRESSION FIX v2] wifi: brcmfmac: Check for probe() id
 argument being NULL
To:     Arend van Spriel <aspriel@gmail.com>,
        Franky Lin <franky.lin@broadcom.com>,
        Hante Meuleman <hante.meuleman@broadcom.com>,
        Kalle Valo <kvalo@kernel.org>
Cc:     linux-wireless@vger.kernel.org,
        brcm80211-dev-list.pdl@broadcom.com,
        SHA-cyfmac-dev-list@infineon.com, regressions@lists.linux.dev,
        Felix <nimrod4garoa@gmail.com>,
        Arend van Spriel <arend.vanspriel@broadcom.com>,
        stable@vger.kernel.org
References: <20230510141856.46532-1-hdegoede@redhat.com>
Content-Language: en-US, nl
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20230510141856.46532-1-hdegoede@redhat.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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);