[PATCH] wifi: brcmfmac: pcie: handle randbuf allocation failure

[PATCH] wifi: brcmfmac: pcie: handle randbuf allocation failure

[Duoming Zhou]

The kzalloc() in brcmf_pcie_download_fw_nvram() will return
null if the physical memory has run out. As a result, if we
use get_random_bytes() to generate random bytes in the randbuf,
the null pointer dereference bug will happen.

Return -ENOMEM from brcmf_pcie_download_fw_nvram() if kzalloc()
fails for randbuf.

Fixes: 91918ce88d9f ("wifi: brcmfmac: pcie: Provide a buffer of random bytes to the device")
Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
index d7fb88bb6ae..5ab9c902e49 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
@@ -1730,6 +1730,8 @@ static int brcmf_pcie_download_fw_nvram(struct brcmf_pciedev_info *devinfo,
 
 			address -= rand_len;
 			randbuf = kzalloc(rand_len, GFP_KERNEL);
+			if (!randbuf)
+				return -ENOMEM;
 			get_random_bytes(randbuf, rand_len);
 			memcpy_toio(devinfo->tcm + address, randbuf, rand_len);
 			kfree(randbuf);
-- 
2.17.1

Re: [PATCH] wifi: brcmfmac: pcie: handle randbuf allocation failure

[Arend van Spriel]

[-- Attachment #1: Type: text/plain, Size: 1559 bytes --]

On 3/1/2024 2:51 PM, Duoming Zhou wrote:
> The kzalloc() in brcmf_pcie_download_fw_nvram() will return
> null if the physical memory has run out. As a result, if we
> use get_random_bytes() to generate random bytes in the randbuf,
> the null pointer dereference bug will happen.
> 
> Return -ENOMEM from brcmf_pcie_download_fw_nvram() if kzalloc()
> fails for randbuf.
> 
> Fixes: 91918ce88d9f ("wifi: brcmfmac: pcie: Provide a buffer of random bytes to the device")

Looks good to me. Looking for kernel guideline about stack usage to 
determine whether it would be ok to just use buffer on stack. Does 
anyone know. This one is 256 bytes so I guess the allocation is 
warranted here.

Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
> ---
>   drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> index d7fb88bb6ae..5ab9c902e49 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> @@ -1730,6 +1730,8 @@ static int brcmf_pcie_download_fw_nvram(struct brcmf_pciedev_info *devinfo,
>   
>   			address -= rand_len;
>   			randbuf = kzalloc(rand_len, GFP_KERNEL);
> +			if (!randbuf)
> +				return -ENOMEM;
>   			get_random_bytes(randbuf, rand_len);
>   			memcpy_toio(devinfo->tcm + address, randbuf, rand_len);
>   			kfree(randbuf);

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

Re: [PATCH] wifi: brcmfmac: pcie: handle randbuf allocation failure

[Kalle Valo]

Arend van Spriel <arend.vanspriel@broadcom.com> writes:

> On 3/1/2024 2:51 PM, Duoming Zhou wrote:
>> The kzalloc() in brcmf_pcie_download_fw_nvram() will return
>> null if the physical memory has run out. As a result, if we
>> use get_random_bytes() to generate random bytes in the randbuf,
>> the null pointer dereference bug will happen.
>> Return -ENOMEM from brcmf_pcie_download_fw_nvram() if kzalloc()
>> fails for randbuf.
>> Fixes: 91918ce88d9f ("wifi: brcmfmac: pcie: Provide a buffer of
>> random bytes to the device")
>
> Looks good to me. Looking for kernel guideline about stack usage to
> determine whether it would be ok to just use buffer on stack. Does
> anyone know. This one is 256 bytes so I guess the allocation is
> warranted here.

Arnd, what do you suggest? Do we have any documentation or guidelines
anywhere?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

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

Re: [PATCH] wifi: brcmfmac: pcie: handle randbuf allocation failure

[Arnd Bergmann]

On Wed, Mar 6, 2024, at 11:53, Kalle Valo wrote:
> Arend van Spriel <arend.vanspriel@broadcom.com> writes:
>
>> On 3/1/2024 2:51 PM, Duoming Zhou wrote:
>>> The kzalloc() in brcmf_pcie_download_fw_nvram() will return
>>> null if the physical memory has run out. As a result, if we
>>> use get_random_bytes() to generate random bytes in the randbuf,
>>> the null pointer dereference bug will happen.
>>> Return -ENOMEM from brcmf_pcie_download_fw_nvram() if kzalloc()
>>> fails for randbuf.
>>> Fixes: 91918ce88d9f ("wifi: brcmfmac: pcie: Provide a buffer of
>>> random bytes to the device")
>>
>> Looks good to me. Looking for kernel guideline about stack usage to
>> determine whether it would be ok to just use buffer on stack. Does
>> anyone know. This one is 256 bytes so I guess the allocation is
>> warranted here.
>
> Arnd, what do you suggest? Do we have any documentation or guidelines
> anywhere?

I don't think we have anything document about this. I usually
consider anything more than half a kilobyte as excessive,
even though the warning limit is higher.

256 bytes is usually fine, but in this case I would split out
the basic block that does this into a separate function
so it does not share the stack frame with other leaf functions
below brcmf_pcie_download_fw_nvram(). It might also be justified
to then mark it as noinline_for_stack.

      Arnd

Re: [PATCH] wifi: brcmfmac: pcie: handle randbuf allocation failure

[Arend van Spriel]

[-- Attachment #1: Type: text/plain, Size: 1730 bytes --]

On 3/6/2024 12:07 PM, Arnd Bergmann wrote:
> On Wed, Mar 6, 2024, at 11:53, Kalle Valo wrote:
>> Arend van Spriel <arend.vanspriel@broadcom.com> writes:
>>
>>> On 3/1/2024 2:51 PM, Duoming Zhou wrote:
>>>> The kzalloc() in brcmf_pcie_download_fw_nvram() will return
>>>> null if the physical memory has run out. As a result, if we
>>>> use get_random_bytes() to generate random bytes in the randbuf,
>>>> the null pointer dereference bug will happen.
>>>> Return -ENOMEM from brcmf_pcie_download_fw_nvram() if kzalloc()
>>>> fails for randbuf.
>>>> Fixes: 91918ce88d9f ("wifi: brcmfmac: pcie: Provide a buffer of
>>>> random bytes to the device")
>>>
>>> Looks good to me. Looking for kernel guideline about stack usage to
>>> determine whether it would be ok to just use buffer on stack. Does
>>> anyone know. This one is 256 bytes so I guess the allocation is
>>> warranted here.
>>
>> Arnd, what do you suggest? Do we have any documentation or guidelines
>> anywhere?
> 
> I don't think we have anything document about this. I usually
> consider anything more than half a kilobyte as excessive,
> even though the warning limit is higher.
> 
> 256 bytes is usually fine, but in this case I would split out
> the basic block that does this into a separate function
> so it does not share the stack frame with other leaf functions
> below brcmf_pcie_download_fw_nvram(). It might also be justified
> to then mark it as noinline_for_stack.

Thanks, Arnd

Makes sense.

@Duoming Zhou,

Can you provide a v2 with separate function using buffer on stack?

static noinline_for_stack
void brcmf_pcie_provide_random_bytes(struct brcmf_pciedev_info *devinfo, 
u32 address)
{
	u8 randbuf[BRCMF_RANDOM_SEED_LENGTH];
	:
	:
}

Regards,
Arend

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

Re: [PATCH] wifi: brcmfmac: pcie: handle randbuf allocation failure

[duoming]

On Wed, 6 Mar 2024 13:53:19 +0100 Arend van Spriel wrote:
> >>>> The kzalloc() in brcmf_pcie_download_fw_nvram() will return
> >>>> null if the physical memory has run out. As a result, if we
> >>>> use get_random_bytes() to generate random bytes in the randbuf,
> >>>> the null pointer dereference bug will happen.
> >>>> Return -ENOMEM from brcmf_pcie_download_fw_nvram() if kzalloc()
> >>>> fails for randbuf.
> >>>> Fixes: 91918ce88d9f ("wifi: brcmfmac: pcie: Provide a buffer of
> >>>> random bytes to the device")
> >>>
> >>> Looks good to me. Looking for kernel guideline about stack usage to
> >>> determine whether it would be ok to just use buffer on stack. Does
> >>> anyone know. This one is 256 bytes so I guess the allocation is
> >>> warranted here.
> >>
> >> Arnd, what do you suggest? Do we have any documentation or guidelines
> >> anywhere?
> > 
> > I don't think we have anything document about this. I usually
> > consider anything more than half a kilobyte as excessive,
> > even though the warning limit is higher.
> > 
> > 256 bytes is usually fine, but in this case I would split out
> > the basic block that does this into a separate function
> > so it does not share the stack frame with other leaf functions
> > below brcmf_pcie_download_fw_nvram(). It might also be justified
> > to then mark it as noinline_for_stack.
> 
> Thanks, Arnd
> 
> Makes sense.
> 
> @Duoming Zhou,
> 
> Can you provide a v2 with separate function using buffer on stack?
> 
> static noinline_for_stack
> void brcmf_pcie_provide_random_bytes(struct brcmf_pciedev_info *devinfo, 
> u32 address)
> {
> 	u8 randbuf[BRCMF_RANDOM_SEED_LENGTH];
> 	:
> 	:
> }

Thank you for your suggestions, I have already provided a v2.

Best regards,
Duoming Zhou