Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <88dbaf62-4a44-2bed-0f1d-530b1d8302c1@quicinc.com>
Date:   Fri, 21 Jul 2023 08:41:56 -0700
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] wifi: mac80211: avoid buffer overflow by adding clear
 data of VHT Tx power envelope
Content-Language: en-US
To:     Wen Gong <quic_wgong@quicinc.com>, <johannes@sipsolutions.net>,
        <ath12k@lists.infradead.org>
CC:     <linux-wireless@vger.kernel.org>
References: <20230721055851.20525-1-quic_wgong@quicinc.com>
From:   Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <20230721055851.20525-1-quic_wgong@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 7/20/2023 10:58 PM, Wen Gong wrote:
> While connecting to a 6 GHz AP, the tx_pwr_env_num of struct
> ieee80211_bss_conf is increased (e.g. from 0 to 1) in function
> ieee80211_prep_channel().
> 
> when AP send authentication with status which is not 0 to station, then
> the connection failed here, and the tx_pwr_env_num is not reset to 0,
> because it is only reset to 0 in ieee80211_set_disassoc() which will not
> entered for this fail. Then connect to AP again and hit same fail again,
> the tx_pwr_env_num will increased again and become to 2, then it is an
> invalid number because it should be 1.
> 
> When connect-fail again and again, finally it will exceed the max length
> tx_pwr_env[] in struct ieee80211_bss_conf, when driver use the value of
> tx_pwr_env_num to run loop to access the tx_pwr_env[], then overflow
> happened here.
> 
> There are many steps while connecting to AP for station, and any one step
> failure will lead connect failure, so it is hard to do reset the value of
> tx_pwr_env_num for each failure case.
> 
> And the next connection maybe change to NON-6G Hz and NON-11AX-HE AP after
> connection failure with 6 GHz AP, then the check of flag is_6ghz and flag
> of IEEE80211_CONN_DISABLE_HE will not matched in ieee80211_prep_channel().
> 
> Hence change to assign value of tx_pwr_env_num each time in function
> ieee80211_prep_channel(), then the tx_pwr_env_num will be 1 when the next
> AP is still 6 GHz AP, and it will be 0 for NON-6 GHz AP , and then it
> will be always avoid buffer overflow and invalid value of tx_pwr_env_num.
> 
> Signed-off-by: Wen Gong <quic_wgong@quicinc.com>
> ---
>   net/mac80211/mlme.c | 9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
> index 959695ed7649..d8ca7f18028e 100644
> --- a/net/mac80211/mlme.c
> +++ b/net/mac80211/mlme.c
> @@ -4712,6 +4712,7 @@ static int ieee80211_prep_channel(struct ieee80211_sub_if_data *sdata,
>   	int ret;
>   	u32 i;
>   	bool have_80mhz;
> +	u8 j = 0;

Since the scope is larger and this is more than a loop control variable, 
suggest giving this a more meaningful name like num_pwr_env

>   
>   	rcu_read_lock();
>   
> @@ -4789,10 +4790,7 @@ static int ieee80211_prep_channel(struct ieee80211_sub_if_data *sdata,
>   		he_oper = elems->he_operation;
>   
>   		if (link && is_6ghz) {
> -			struct ieee80211_bss_conf *bss_conf;
> -			u8 j = 0;
> -
> -			bss_conf = link->conf;
> +			struct ieee80211_bss_conf *bss_conf = link->conf;;

s/;;/;/

>   
>   			if (elems->pwr_constr_elem)
>   				bss_conf->pwr_reduction = *elems->pwr_constr_elem;
> @@ -4805,7 +4803,6 @@ static int ieee80211_prep_channel(struct ieee80211_sub_if_data *sdata,
>   				    sizeof(bss_conf->tx_pwr_env[j]))
>   					continue;
>   
> -				bss_conf->tx_pwr_env_num++;
>   				memcpy(&bss_conf->tx_pwr_env[j], elems->tx_pwr_env[i],
>   				       elems->tx_pwr_env_len[i]);
>   				j++;
> @@ -4818,6 +4815,8 @@ static int ieee80211_prep_channel(struct ieee80211_sub_if_data *sdata,
>   				       IEEE80211_CONN_DISABLE_EHT;
>   	}
>   
> +	link->conf->tx_pwr_env_num = j;
> +
>   	/*
>   	 * EHT requires HE to be supported as well. Specifically for 6 GHz
>   	 * channels, the operation channel information can only be deduced from
> 
> base-commit: b21fe5be53eb873c02e7479372726c8aeed171e3