Messages in this thread |  | | Date | Sat, 10 Dec 2022 21:52:15 +0800 | Subject | Re: [PATCH] rtlwifi: rtl8821ae: Fix global-out-of-bounds bug in _rtl8812ae_phy_set_txpower_limit() | From | Li Zetao <> |
| |
Hi Ping-Ke,
On 2022/12/10 21:15, Ping-Ke Shih wrote: > On Sat, 2022-12-10 at 20:47 +0800, Li Zetao wrote: >> Hi Ping-Ke, >> >> On 2022/12/9 13:11, Ping-Ke Shih wrote: >>>> -----Original Message----- >>>> From: Li Zetao <lizetao1@huawei.com> >>>> Sent: Wednesday, December 7, 2022 11:23 PM >>>> To: Ping-Ke Shih <pkshih@realtek.com>; kvalo@kernel.org; davem@davemloft.net; >>>> edumazet@google.com; >>>> kuba@kernel.org; pabeni@redhat.com >>>> Cc: lizetao1@huawei.com; Larry.Finger@lwfinger.net; linville@tuxdriver.com; >>>> linux-wireless@vger.kernel.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org >>>> Subject: [PATCH] rtlwifi: rtl8821ae: Fix global-out-of-bounds bug in >>>> _rtl8812ae_phy_set_txpower_limit() >>>> >>>> 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, so this requires the length of the two strings be greater >>>> than or equal to n. 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", it will >>>> lead to access out of bounds in _rtl8812ae_eq_n_byte(). >>>> >>>> Fix it by adding a length check in _rtl8812ae_eq_n_byte(). 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@huawei.com> >>>> --- >>>> drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c >>>> b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c >>>> index a29321e2fa72..720114a9ddb2 100644 >>>> --- a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c >>>> +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c >>>> @@ -1600,7 +1600,7 @@ static bool _rtl8812ae_get_integer_from_string(const char *str, u8 >>>> *pint) >>>> >>>> static bool _rtl8812ae_eq_n_byte(const char *str1, const char *str2, u32 num) >>>> { >>> This can causes problem because it compares characters from tail to head, and >>> we can't simply replace this by strncmp() that does similar work. But, I also >>> don't like strlen() to loop 'str1' constantly. >>> >>> How about having a simple loop to compare characters forward: >>> >>> for (i = 0; i < num; i++) >>> if (str1[i] != str2[i]) >>> return false; >>> >>> return true; >> Thanks for your comment, but I don't think the problem has anything to >> do with head-to-tail or >> >> tail-to-head comparison. The problem is that num is the length of str2, >> but the length of str1 may >> >> be less than num, which may lead to reading str1 out of bounds, for >> example, when comparing >> >> "prate_section", str1 may be "HT", while str2 may by "CCK", and num is >> 3. So I think it is neccssary >> >> to check the length of str1 to ensure that will not read out of bounds. >> > I know your point, and I believe your patch can work well, but I would like > to have simple code that can solve this specific problem. > > Since both str1 and str2 are null-terminator strings, so str1[2]='\0' is > accessible if str1="HT", right? Then, if length of str1 and str2 is > different, null-terminator can help to break head-to-tail loop. > > Take "12" and "1234" as an example: > Then, num=4, > > head-to-tail tail-to-head > ------------------- ------------------------------------------------- > str1[0] == str2[0] str1[3] >< str2[3] (str1[3] is inaccessible) > str1[1] == str2[1] > str1[2] != str2[2] > > > I hope this can help to explain my point. Thank you, I get your point now. > > After I think deeper, it seems like third parameter 'u32 num' isn't necessary, > and then just strcmp(str1, str2) is enough. > > Ping-Ke
I will replace _rtl8812ae_eq_n_byte with strcmp in the v2 and do a full test.
With Best Regards,
Li Zetao
>
|  |