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

[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@xxxxxxxxxx>
> > > > Sent: Wednesday, December 7, 2022 11:23 PM
> > > > To: Ping-Ke Shih <pkshih@xxxxxxxxxxx>; kvalo@xxxxxxxxxx; davem@xxxxxxxxxxxxx;
> > > > edumazet@xxxxxxxxxx;
> > > > kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx
> > > > Cc: lizetao1@xxxxxxxxxx; Larry.Finger@xxxxxxxxxxxx; linville@xxxxxxxxxxxxx;
> > > > linux-wireless@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> > > > 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@xxxxxxxxxx>
> > > > ---
> > > >    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

>