| Message ID | 20230721055851.20525-1-quic_wgong@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | wifi: mac80211: avoid buffer overflow by adding clear data of VHT Tx power envelope | expand |
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
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 --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
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