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 <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Duoming Zhou <[email protected]>
> ---
> 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);
Arend van Spriel <[email protected]> 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
On Wed, Mar 6, 2024, at 11:53, Kalle Valo wrote:
> Arend van Spriel <[email protected]> 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
On 3/6/2024 12:07 PM, Arnd Bergmann wrote:
> On Wed, Mar 6, 2024, at 11:53, Kalle Valo wrote:
>> Arend van Spriel <[email protected]> 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
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