Re: [PATCH] wifi: brcmfmac: ensure CLM version is null-terminated to prevent stack-out-of-bounds

[Arend Van Spriel <arend.vanspriel@xxxxxxxxxxxx>]

On December 23, 2022 9:29:59 AM Jisoo Jang <jisoo.jang@xxxxxxxxxxxx> wrote:

> Fix a stack-out-of-bounds read in brcmfmac that occurs
> when 'buf' that is not null-terminated is passed as an argument of
> strreplace() in brcmf_c_preinit_dcmds(). This buffer is filled with
> a CLM version string by memcpy() in brcmf_fil_iovar_data_get().
> Ensure buf is null-terminated.
>
> Found by a modified version of syzkaller.

[...]

Thanks for this patch. Minor comment below.

Reviewed-by: Arend van Spriel<arend.vanspriel@xxxxxxxxxxxx>
> Signed-off-by: Jisoo Jang <jisoo.jang@xxxxxxxxxxxx>
>
> ---
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c 
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> index 4a309e5a5707..4b6adb6ce5e3 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> @@ -319,6 +319,7 @@ int brcmf_c_preinit_dcmds(struct brcmf_if *ifp)
>  if (err) {
>  brcmf_dbg(TRACE, "retrieving clmver failed, %d\n", err);
>  } else {
> + buf[sizeof(buf) - 1] = '\0';
>  clmver = (char *)buf;
>  /* store CLM version for adding it to revinfo debugfs file */
>  memcpy(ifp->drvr->clmver, clmver, sizeof(ifp->drvr->clmver));

Can you move the memcpy() after the strreplace() call?

Regards,
Arend

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature