| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <66c119cc4e184a36d525a07f2fbd092348839610.camel@realtek.com>
Date: Sun, 11 Dec 2022 11:21:40 +0000
From: Ping-Ke Shih <pkshih@...ltek.com>
To: "lizetao1@...wei.com" <lizetao1@...wei.com>
CC: "davem@...emloft.net" <davem@...emloft.net>,
"pabeni@...hat.com" <pabeni@...hat.com>,
"kvalo@...nel.org" <kvalo@...nel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
"Larry.Finger@...inger.net" <Larry.Finger@...inger.net>,
"kuba@...nel.org" <kuba@...nel.org>,
"edumazet@...gle.com" <edumazet@...gle.com>,
"linville@...driver.com" <linville@...driver.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH v2] rtlwifi: rtl8821ae: Fix global-out-of-bounds bug in _rtl8812ae_phy_set_txpower_limit()
On Sun, 2022-12-11 at 00:23 +0800, Li Zetao wrote:
> There is a global-out-of-bounds reported by KASAN:
>
> BUG: KASAN: global-out-of-bounds in
> _rtl8812ae_eq_n_byte.part.0+0x3d/0x84 [rtl8821ae]
> Read of size 1 at addr ffffffffa0773c43 by task NetworkManager/411
>
> CPU: 6 PID: 411 Comm: NetworkManager Tainted: G D
> 6.1.0-rc8+ #144 e15588508517267d37
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009),
> Call Trace:
> <TASK>
> ...
> kasan_report+0xbb/0x1c0
> _rtl8812ae_eq_n_byte.part.0+0x3d/0x84 [rtl8821ae]
> rtl8821ae_phy_bb_config.cold+0x346/0x641 [rtl8821ae]
> rtl8821ae_hw_init+0x1f5e/0x79b0 [rtl8821ae]
> ...
> </TASK>
>
> The root cause of the problem is that the comparison order of
> "prate_section" in _rtl8812ae_phy_set_txpower_limit() is wrong. The
> _rtl8812ae_eq_n_byte() is used to compare the first n bytes of the two
> strings from tail to head, which causes the problem. In the
> _rtl8812ae_phy_set_txpower_limit(), it was originally intended to meet
> this requirement by carefully designing the comparison order.
> For example, "pregulation" and "pbandwidth" are compared in order of
> length from small to large, first is 3 and last is 4. However, the
> comparison order of "prate_section" dose not obey such order requirement,
> therefore when "prate_section" is "HT", when comparing from tail to head,
> it will lead to access out of bounds in _rtl8812ae_eq_n_byte(). As
> mentioned above, the _rtl8812ae_eq_n_byte() has the same function as
> strcmp(), so just strcmp() is enough.
>
> Fix it by replacing _rtl8812ae_eq_n_byte() with strcmp(). Although it
> can be fixed by adjusting the comparison order of "prate_section", this
> may cause the value of "rate_section" to not be from 0 to 5. In
> addition, commit "21e4b0726dc6" not only moved driver from staging to
> regular tree, but also added setting txpower limit function during the
> driver config phase, so the problem was introduced by this commit.
>
> Fixes: 21e4b0726dc6 ("rtlwifi: rtl8821ae: Move driver from staging to regular tree")
> Signed-off-by: Li Zetao <lizetao1@...wei.com>
> ---
> v1 was posted at: https://lore.kernel.org/all/20221207152319.3135500-1-lizetao1@huawei.com/
> v1 -> v2: delete the third parameter of _rtl8812ae_eq_n_byte() and use
> strcmp to replace loop comparison.
>
> .../wireless/realtek/rtlwifi/rtl8821ae/phy.c | 51 ++++++++-----------
> 1 file changed, 22 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c
> b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c
> index a29321e2fa72..14b569d7d8fa 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c
> @@ -1598,16 +1598,9 @@ static bool _rtl8812ae_get_integer_from_string(const char *str, u8 *pint)
> return true;
> }
>
> -static bool _rtl8812ae_eq_n_byte(const char *str1, const char *str2, u32 num)
> +static bool _rtl8812ae_eq_n_byte(const char *str1, const char *str2)
> {
> - if (num == 0)
> - return false;
> - while (num > 0) {
> - num--;
> - if (str1[num] != str2[num])
> - return false;
> - }
> - return true;
> + return strcmp(str1, str2) == 0;
> }
I suggest to remove _rtl8812ae_eq_n_byte() and use strcmp() barely.
That would be more clear, and people don't need to check detail of
unnecessary _rtl8812ae_eq_n_byte().
>
> static s8 _rtl8812ae_phy_get_chnl_idx_of_txpwr_lmt(struct ieee80211_hw *hw,
> @@ -1659,42 +1652,42 @@ static void _rtl8812ae_phy_set_txpower_limit(struct ieee80211_hw *hw,
> power_limit = power_limit > MAX_POWER_INDEX ?
> MAX_POWER_INDEX : power_limit;
>
> - if (_rtl8812ae_eq_n_byte(pregulation, "FCC", 3))
> + if (_rtl8812ae_eq_n_byte(pregulation, "FCC"))
> regulation = 0;
> - else if (_rtl8812ae_eq_n_byte(pregulation, "MKK", 3))
> + else if (_rtl8812ae_eq_n_byte(pregulation, "MKK"))
> regulation = 1;
> - else if (_rtl8812ae_eq_n_byte(pregulation, "ETSI", 4))
> + else if (_rtl8812ae_eq_n_byte(pregulation, "ETSI"))
> regulation = 2;
> - else if (_rtl8812ae_eq_n_byte(pregulation, "WW13", 4))
> + else if (_rtl8812ae_eq_n_byte(pregulation, "WW13"))
> regulation = 3;
>
> - if (_rtl8812ae_eq_n_byte(prate_section, "CCK", 3))
> + if (_rtl8812ae_eq_n_byte(prate_section, "CCK"))
> rate_section = 0;
> - else if (_rtl8812ae_eq_n_byte(prate_section, "OFDM", 4))
> + else if (_rtl8812ae_eq_n_byte(prate_section, "OFDM"))
> rate_section = 1;
> - else if (_rtl8812ae_eq_n_byte(prate_section, "HT", 2) &&
> - _rtl8812ae_eq_n_byte(prf_path, "1T", 2))
> + else if (_rtl8812ae_eq_n_byte(prate_section, "HT") &&
> + _rtl8812ae_eq_n_byte(prf_path, "1T"))
> rate_section = 2;
> - else if (_rtl8812ae_eq_n_byte(prate_section, "HT", 2) &&
> - _rtl8812ae_eq_n_byte(prf_path, "2T", 2))
> + else if (_rtl8812ae_eq_n_byte(prate_section, "HT") &&
> + _rtl8812ae_eq_n_byte(prf_path, "2T"))
> rate_section = 3;
> - else if (_rtl8812ae_eq_n_byte(prate_section, "VHT", 3) &&
> - _rtl8812ae_eq_n_byte(prf_path, "1T", 2))
> + else if (_rtl8812ae_eq_n_byte(prate_section, "VHT") &&
> + _rtl8812ae_eq_n_byte(prf_path, "1T"))
> rate_section = 4;
> - else if (_rtl8812ae_eq_n_byte(prate_section, "VHT", 3) &&
> - _rtl8812ae_eq_n_byte(prf_path, "2T", 2))
> + else if (_rtl8812ae_eq_n_byte(prate_section, "VHT") &&
> + _rtl8812ae_eq_n_byte(prf_path, "2T"))
> rate_section = 5;
>
> - if (_rtl8812ae_eq_n_byte(pbandwidth, "20M", 3))
> + if (_rtl8812ae_eq_n_byte(pbandwidth, "20M"))
> bandwidth = 0;
> - else if (_rtl8812ae_eq_n_byte(pbandwidth, "40M", 3))
> + else if (_rtl8812ae_eq_n_byte(pbandwidth, "40M"))
> bandwidth = 1;
> - else if (_rtl8812ae_eq_n_byte(pbandwidth, "80M", 3))
> + else if (_rtl8812ae_eq_n_byte(pbandwidth, "80M"))
> bandwidth = 2;
> - else if (_rtl8812ae_eq_n_byte(pbandwidth, "160M", 4))
> + else if (_rtl8812ae_eq_n_byte(pbandwidth, "160M"))
> bandwidth = 3;
>
> - if (_rtl8812ae_eq_n_byte(pband, "2.4G", 4)) {
> + if (_rtl8812ae_eq_n_byte(pband, "2.4G")) {
> ret = _rtl8812ae_phy_get_chnl_idx_of_txpwr_lmt(hw,
> BAND_ON_2_4G,
> channel);
> @@ -1718,7 +1711,7 @@ static void _rtl8812ae_phy_set_txpower_limit(struct ieee80211_hw *hw,
> regulation, bandwidth, rate_section, channel_index,
> rtlphy->txpwr_limit_2_4g[regulation][bandwidth]
> [rate_section][channel_index][RF90_PATH_A]);
> - } else if (_rtl8812ae_eq_n_byte(pband, "5G", 2)) {
> + } else if (_rtl8812ae_eq_n_byte(pband, "5G")) {
> ret = _rtl8812ae_phy_get_chnl_idx_of_txpwr_lmt(hw,
> BAND_ON_5G,
> channel);
> --
> 2.31.1
>
>
> ------Please consider the environment before printing this e-mail.
Powered by blists - more mailing lists