Re: [PATCH v2] wifi: brcmfmac: Fix potential slab-out-of-bounds read in brcmf_inform_single_bss()

[Arend Van Spriel <aspriel@gmail.com>]

On 11/16/2022 3:58 PM, Minsuk Kang wrote:
> This patch fixes a slab-out-of-bounds read in brcmfmac that occurs in
> cfg80211_find_elem_match() called from brcmf_inform_single_bss() when
> the offset and length values of information elements provided by the
> device exceed the boundary of the escan buffer that contains information
> elements. The patch adds a check that makes the function return -EINVAL
> if that is the case. Note that the negative return is handled by the
> caller, brcmf_inform_bss().

[...]

Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Reported-by: Dokyung Song <dokyungs@yonsei.ac.kr>
> Reported-by: Jisoo Jang <jisoo.jang@yonsei.ac.kr>
> Reported-by: Minsuk Kang <linuxlovemin@yonsei.ac.kr>
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Minsuk Kang <linuxlovemin@yonsei.ac.kr>
> ---
> v1->v2: Use the correct format for size_t in bphy_err()
> 
>   .../net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c    | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> index ae9507dec74a..2148027eb42b 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> @@ -3298,6 +3298,13 @@ static s32 brcmf_inform_single_bss(struct brcmf_cfg80211_info *cfg,
>   	notify_ielen = le32_to_cpu(bi->ie_length);
>   	bss_data.signal = (s16)le16_to_cpu(bi->RSSI) * 100;
>   
> +	if ((unsigned long)notify_ie + notify_ielen -
> +		(unsigned long)cfg->escan_info.escan_buf > BRCMF_ESCAN_BUF_SIZE) {
> +		bphy_err(drvr, "Invalid information element offset: %u, length: %zu\n",
> +			 le16_to_cpu(bi->ie_offset), notify_ielen);
> +		return -EINVAL;
> +	}
> +

Maybe this works, but it was not immediately obvious to me. Also this 
seems late in processing the scan results. Better catch it early and 
check the ie_offset and ie_length values in 
brcmf_cfg80211_escan_handler() when processing the partial result event. 
It already checks bi->length there so add a check there:

	bss_ie_offset = le16_to_cpu(bi->ie_offset);
	bss_ie_length = le16_to_cpu(bi->ie_length);
	if (bi->ie_offset + bi->ie_length > bi->length) {
		bphy_err(drvr, "Ignoring invalid information element offset: %u, 
length: %zu\n"
			 bss_ie_offset, bss_ie_length);
		goto exit;
	}

Regards,
Arend