diff mbox series

wifi: mac80211: avoid buffer overflow by adding clear data of VHT Tx power envelope

Message ID 20230721055851.20525-1-quic_wgong@quicinc.com
State New
Headers show
Series wifi: mac80211: avoid buffer overflow by adding clear data of VHT Tx power envelope | expand

Commit Message

Wen Gong July 21, 2023, 5:58 a.m. UTC 
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(-)


base-commit: b21fe5be53eb873c02e7479372726c8aeed171e3

Comments

Jeff Johnson July 21, 2023, 3:41 p.m. UTC | #1 
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
Dan Carpenter July 25, 2023, 11:11 a.m. UTC | #2 
Hi Wen,

kernel test robot noticed the following build warnings:

url:    https://github.com/intel-lab-lkp/linux/commits/Wen-Gong/wifi-mac80211-avoid-buffer-overflow-by-adding-clear-data-of-VHT-Tx-power-envelope/20230721-140122
base:   b21fe5be53eb873c02e7479372726c8aeed171e3
patch link:    https://lore.kernel.org/r/20230721055851.20525-1-quic_wgong%40quicinc.com
patch subject: [PATCH] wifi: mac80211: avoid buffer overflow by adding clear data of VHT Tx power envelope
config: i386-randconfig-m021-20230723 (https://download.01.org/0day-ci/archive/20230725/202307251807.z04UOfqH-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230725/202307251807.z04UOfqH-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202307251807.z04UOfqH-lkp@intel.com/

New smatch warnings:
net/mac80211/mlme.c:4818 ieee80211_prep_channel() error: we previously assumed 'link' could be null (see line 4792)
net/mac80211/mlme.c:4890 ieee80211_prep_channel() warn: variable dereferenced before check 'link' (see line 4818)

Old smatch warnings:
net/mac80211/mlme.c:7073 ieee80211_setup_assoc_link() warn: variable dereferenced before check 'elem' (see line 7071)

vim +/link +4818 net/mac80211/mlme.c

7781f0d81c7a7e6 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4789  	if (!(*conn_flags & IEEE80211_CONN_DISABLE_HE)) {
61513162aa2d6c1 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4790  		he_oper = elems->he_operation;
d524215f6cad245 net/mac80211/mlme.c          Felix Fietkau     2010-01-08  4791  
7781f0d81c7a7e6 net/mac80211/mlme.c          Johannes Berg     2022-07-12 @4792  		if (link && is_6ghz) {

Check for NULL

4df17235d03fd79 net/mac80211/mlme.c          Wen Gong          2023-07-21  4793  			struct ieee80211_bss_conf *bss_conf = link->conf;;
a607268a0d5532d net/mac80211/ieee80211_sta.c Bruno Randolf     2008-02-18  4794  
61513162aa2d6c1 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4795  			if (elems->pwr_constr_elem)
61513162aa2d6c1 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4796  				bss_conf->pwr_reduction = *elems->pwr_constr_elem;
66e67e418908442 net/mac80211/mlme.c          Johannes Berg     2012-01-20  4797  
61513162aa2d6c1 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4798  			BUILD_BUG_ON(ARRAY_SIZE(bss_conf->tx_pwr_env) !=
61513162aa2d6c1 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4799  				     ARRAY_SIZE(elems->tx_pwr_env));
66e67e418908442 net/mac80211/mlme.c          Johannes Berg     2012-01-20  4800  
61513162aa2d6c1 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4801  			for (i = 0; i < elems->tx_pwr_env_num; i++) {
61513162aa2d6c1 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4802  				if (elems->tx_pwr_env_len[i] >
61513162aa2d6c1 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4803  				    sizeof(bss_conf->tx_pwr_env[j]))
61513162aa2d6c1 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4804  					continue;
1d00ce807efaa0e net/mac80211/mlme.c          Thomas Pedersen   2020-09-21  4805  
61513162aa2d6c1 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4806  				memcpy(&bss_conf->tx_pwr_env[j], elems->tx_pwr_env[i],
61513162aa2d6c1 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4807  				       elems->tx_pwr_env_len[i]);
61513162aa2d6c1 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4808  				j++;
61513162aa2d6c1 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4809  			}
61513162aa2d6c1 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4810  		}
66e67e418908442 net/mac80211/mlme.c          Johannes Berg     2012-01-20  4811  
61513162aa2d6c1 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4812  		if (!ieee80211_verify_peer_he_mcs_support(sdata, ies, he_oper) ||
61513162aa2d6c1 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4813  		    !ieee80211_verify_sta_he_mcs_support(sdata, sband, he_oper))
7781f0d81c7a7e6 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4814  			*conn_flags |= IEEE80211_CONN_DISABLE_HE |
61513162aa2d6c1 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4815  				       IEEE80211_CONN_DISABLE_EHT;
61513162aa2d6c1 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4816  	}
1d00ce807efaa0e net/mac80211/mlme.c          Thomas Pedersen   2020-09-21  4817  
4df17235d03fd79 net/mac80211/mlme.c          Wen Gong          2023-07-21 @4818  	link->conf->tx_pwr_env_num = j;
                                                                                        ^^^^^^^^^^
Unchecked dereference

4df17235d03fd79 net/mac80211/mlme.c          Wen Gong          2023-07-21  4819  
66e67e418908442 net/mac80211/mlme.c          Johannes Berg     2012-01-20  4820  	/*
61513162aa2d6c1 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4821  	 * EHT requires HE to be supported as well. Specifically for 6 GHz
61513162aa2d6c1 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4822  	 * channels, the operation channel information can only be deduced from
61513162aa2d6c1 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4823  	 * both the 6 GHz operation information (from the HE operation IE) and
61513162aa2d6c1 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4824  	 * EHT operation.
66e67e418908442 net/mac80211/mlme.c          Johannes Berg     2012-01-20  4825  	 */
7781f0d81c7a7e6 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4826  	if (!(*conn_flags &

[ snip ]

1ad22fb5bb53ce6 net/mac80211/mlme.c          Tosoni            2018-03-14  4879  
6911458dc4283a7 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4880  	*conn_flags |=
6911458dc4283a7 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4881  		ieee80211_determine_chantype(sdata, link, *conn_flags,
6911458dc4283a7 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4882  					     sband,
6911458dc4283a7 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4883  					     cbss->channel,
6911458dc4283a7 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4884  					     bss->vht_cap_info,
6911458dc4283a7 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4885  					     ht_oper, vht_oper,
6911458dc4283a7 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4886  					     he_oper, eht_oper,
6911458dc4283a7 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4887  					     s1g_oper,
6911458dc4283a7 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4888  					     &chandef, false);
78ac51f81532c1e net/mac80211/mlme.c          Sara Sharon       2019-01-16  4889  
6911458dc4283a7 net/mac80211/mlme.c          Johannes Berg     2022-07-12 @4890  	if (link)

More checks for NULL

6911458dc4283a7 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4891  		link->needed_rx_chains =
6911458dc4283a7 net/mac80211/mlme.c          Johannes Berg     2022-07-12  4892  			min(ieee80211_max_rx_chains(link, cbss),
diff mbox series

Patch

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;
 
 	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;;
 
 			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