[PATCH wireless-drivers-next] brcmfmac: add basic validation of shared RAM address

[PATCH wireless-drivers-next] brcmfmac: add basic validation of shared RAM address

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

While experimenting with firmware loading I ended up in a state of
firmware reporting shared RAM address 0x04000001. It was causing:
[   94.448015] Unable to handle kernel paging request at virtual address cd680001
due to reading out of the mapped memory.

This patch adds some basic validation to avoid kernel crashes due to the
unexpected firmware behavior.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
index 257f919c52cc..58a6bc379358 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
@@ -1560,6 +1560,12 @@ static int brcmf_pcie_download_fw_nvram(struct brcmf_pciedev_info *devinfo,
 		brcmf_err(bus, "FW failed to initialize\n");
 		return -ENODEV;
 	}
+	if (sharedram_addr < devinfo->ci->rambase ||
+	    sharedram_addr >= devinfo->ci->rambase + devinfo->ci->ramsize) {
+		brcmf_err(bus, "Invalid shared RAM address 0x%08x\n",
+			  sharedram_addr);
+		return -ENODEV;
+	}
 	brcmf_dbg(PCIE, "Shared RAM addr: 0x%08x\n", sharedram_addr);
 
 	return (brcmf_pcie_init_share_ram_info(devinfo, sharedram_addr));
-- 
2.20.1

Re: [PATCH wireless-drivers-next] brcmfmac: add basic validation of shared RAM address

[Arend Van Spriel]

On 2/20/2019 11:30 AM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> While experimenting with firmware loading I ended up in a state of
> firmware reporting shared RAM address 0x04000001. It was causing:
> [   94.448015] Unable to handle kernel paging request at virtual address cd680001
> due to reading out of the mapped memory.
> 
> This patch adds some basic validation to avoid kernel crashes due to the
> unexpected firmware behavior.

Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>   drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 6 ++++++
>   1 file changed, 6 insertions(+)

Re: [PATCH wireless-drivers-next] brcmfmac: add basic validation of shared RAM address

[Kalle Valo]

Rafał Miłecki wrote:

> From: Rafał Miłecki <rafal@milecki.pl>
> 
> While experimenting with firmware loading I ended up in a state of
> firmware reporting shared RAM address 0x04000001. It was causing:
> [   94.448015] Unable to handle kernel paging request at virtual address cd680001
> due to reading out of the mapped memory.
> 
> This patch adds some basic validation to avoid kernel crashes due to the
> unexpected firmware behavior.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>

Patch applied to wireless-drivers-next.git, thanks.

e0a8ef4d7b43 brcmfmac: add basic validation of shared RAM address

-- 
https://patchwork.kernel.org/patch/10821803/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH wireless-drivers-next] brcmfmac: add basic validation of shared RAM address

[Rafał Miłecki]

On Wed, 20 Feb 2019 at 11:31, Rafał Miłecki <zajec5@gmail.com> wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
>
> While experimenting with firmware loading I ended up in a state of
> firmware reporting shared RAM address 0x04000001. It was causing:
> [   94.448015] Unable to handle kernel paging request at virtual address cd680001
> due to reading out of the mapped memory.
>
> This patch adds some basic validation to avoid kernel crashes due to the
> unexpected firmware behavior.

For a reference for the further hackers. That has been caused by a
BCMA_CORE_SYS_MEM core on my BCM4366/4 not being up.

Re: [PATCH wireless-drivers-next] brcmfmac: add basic validation of shared RAM address

[Arend Van Spriel]

On 2/21/2019 9:01 AM, Rafał Miłecki wrote:
> On Wed, 20 Feb 2019 at 11:31, Rafał Miłecki <zajec5@gmail.com> wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> While experimenting with firmware loading I ended up in a state of
>> firmware reporting shared RAM address 0x04000001. It was causing:
>> [   94.448015] Unable to handle kernel paging request at virtual address cd680001
>> due to reading out of the mapped memory.
>>
>> This patch adds some basic validation to avoid kernel crashes due to the
>> unexpected firmware behavior.
> 
> For a reference for the further hackers. That has been caused by a
> BCMA_CORE_SYS_MEM core on my BCM4366/4 not being up.

Thanks, Rafał

Does that happen all the time or in some specific scenario. Anyway, it 
seems like we need to add a check in brcmf_chip_sysmem_ramsize() and 
bringup the core if needed. Although, I am curious in what the state the 
other cores are at that time. Might need a chip-wide reset.

Regards,
Arend

Re: [PATCH wireless-drivers-next] brcmfmac: add basic validation of shared RAM address

[Rafał Miłecki]

On Thu, 21 Feb 2019 at 09:59, Arend Van Spriel
<arend.vanspriel@broadcom.com> wrote:
> On 2/21/2019 9:01 AM, Rafał Miłecki wrote:
> > On Wed, 20 Feb 2019 at 11:31, Rafał Miłecki <zajec5@gmail.com> wrote:
> >> From: Rafał Miłecki <rafal@milecki.pl>
> >>
> >> While experimenting with firmware loading I ended up in a state of
> >> firmware reporting shared RAM address 0x04000001. It was causing:
> >> [   94.448015] Unable to handle kernel paging request at virtual address cd680001
> >> due to reading out of the mapped memory.
> >>
> >> This patch adds some basic validation to avoid kernel crashes due to the
> >> unexpected firmware behavior.
> >
> > For a reference for the further hackers. That has been caused by a
> > BCMA_CORE_SYS_MEM core on my BCM4366/4 not being up.
>
> Thanks, Rafał
>
> Does that happen all the time or in some specific scenario. Anyway, it
> seems like we need to add a check in brcmf_chip_sysmem_ramsize() and
> bringup the core if needed. Although, I am curious in what the state the
> other cores are at that time. Might need a chip-wide reset.

It happens after brcmf_pcie_reset_device() which is probably a 100%
expected case ;)

Normally brcmfmac does not call brcmf_pcie_reset_device() between
brcmf_chip_sysmem_ramsize() and the brcmf_pcie_download_fw_nvram() so
it was only my hacking that caused that issue for me.

-- 
Rafał

Re: [PATCH wireless-drivers-next] brcmfmac: add basic validation of shared RAM address

[Arend Van Spriel]

On 2/21/2019 10:27 AM, Rafał Miłecki wrote:
> On Thu, 21 Feb 2019 at 09:59, Arend Van Spriel
> <arend.vanspriel@broadcom.com> wrote:
>> On 2/21/2019 9:01 AM, Rafał Miłecki wrote:
>>> On Wed, 20 Feb 2019 at 11:31, Rafał Miłecki <zajec5@gmail.com> wrote:
>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>>
>>>> While experimenting with firmware loading I ended up in a state of
>>>> firmware reporting shared RAM address 0x04000001. It was causing:
>>>> [   94.448015] Unable to handle kernel paging request at virtual address cd680001
>>>> due to reading out of the mapped memory.
>>>>
>>>> This patch adds some basic validation to avoid kernel crashes due to the
>>>> unexpected firmware behavior.
>>>
>>> For a reference for the further hackers. That has been caused by a
>>> BCMA_CORE_SYS_MEM core on my BCM4366/4 not being up.
>>
>> Thanks, Rafał
>>
>> Does that happen all the time or in some specific scenario. Anyway, it
>> seems like we need to add a check in brcmf_chip_sysmem_ramsize() and
>> bringup the core if needed. Although, I am curious in what the state the
>> other cores are at that time. Might need a chip-wide reset.
> 
> It happens after brcmf_pcie_reset_device() which is probably a 100%
> expected case ;)

Did not check what that function exactly does, but could be expected.

> Normally brcmfmac does not call brcmf_pcie_reset_device() between
> brcmf_chip_sysmem_ramsize() and the brcmf_pcie_download_fw_nvram() so
> it was only my hacking that caused that issue for me.

Ah. Can only wish you happy hacking then ;-)

Gr. AvS