lkml.org 
[lkml]   [2022]   [Dec]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] rtlwifi: rtl8821ae: Fix global-out-of-bounds bug in _rtl8812ae_phy_set_txpower_limit()
From
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

>

\
 
 \ /
  Last update: 2022-12-10 14:53    [W:0.044 / U:0.316 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site