| Message ID | 20240301135134.29577-1-duoming@zju.edu.cn |
|---|---|
| State | New |
| Headers | show |
| Series | wifi: brcmfmac: pcie: handle randbuf allocation failure | expand |
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);
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?
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
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
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);
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(+)