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: <ed5b02e3-ded8-1edd-c903-d7a439065c3a@redhat.com>
Date:   Wed, 10 May 2023 16:18:47 +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] wifi: brcmfmac: Check for probe() id argument being NULL
To:     Arend van Spriel <arend.vanspriel@broadcom.com>,
        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>, stable@vger.kernel.org
References: <20230510100050.27099-1-hdegoede@redhat.com>
 <7f563b21-4c06-134e-f30d-02ed61521c1f@broadcom.com>
Content-Language: en-US, nl
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <7f563b21-4c06-134e-f30d-02ed61521c1f@broadcom.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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