Re: [PATCH] rtlwifi: rtl8821ae: Fix global-out-of-bounds bug in _rtl8812ae_phy_set_txpower_limit()

[Ping-Ke Shih <pkshih@realtek.com>]

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.


After I think deeper, it seems like third parameter 'u32 num' isn't necessary,
and then just strcmp(str1, str2) is enough.

Ping-Ke