Message-ID: <63f72045-e51d-d9a4-a0ed-c221bcdcee03@gmail.com>
Date:   Mon, 27 Feb 2023 20:59:26 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.2
Subject: Re: [PATCH v2] wifi: brcmfmac: Fix potential slab-out-of-bounds read
 in brcmf_inform_single_bss()
To:     Minsuk Kang <linuxlovemin@yonsei.ac.kr>,
        linux-wireless@vger.kernel.org
Cc:     arend.vanspriel@broadcom.com, dokyungs@yonsei.ac.kr,
        jisoo.jang@yonsei.ac.kr, kernel test robot <lkp@intel.com>
References: <20221116145821.544266-1-linuxlovemin@yonsei.ac.kr>
Content-Language: en-US
From:   Arend Van Spriel <aspriel@gmail.com>
In-Reply-To: <20221116145821.544266-1-linuxlovemin@yonsei.ac.kr>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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