RE: [PATCH rtw-next] wifi: rtw88: Add more validation for the RX descriptor

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

Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> On 18/05/2026 11:14, Ping-Ke Shih wrote:
> > Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> >> Some RTL8821CE cards can return frames with corrupted RX descriptor,
> >> causing warnings and crashes if they are passed to the upper layers.
> >
> > Not sure if this is the reason Larry uploaded a copy of vendor driver
> > to his repository [1].
> >
> 
> He added it for someone whose wifi card sometimes wasn't powering on:
> 
> https://github.com/lwfinger/rtw88/issues/98#issuecomment-1263962943
> 
> > Recently, we received vulnerability report of rtw_mp_efuse_set() in
> > vendor driver. I'd like to know if people are still using the vendor
> > driver [1]. If not, is it possible to remove it? If people still need it,
> > I will share the fix made by our internal later.
> >
> > [1] https://github.com/lwfinger/rtw88/tree/master/alt_rtl8821ce
> >
> 
> We haven't been updating it for the kernel API changes, so I think we
> can remove it.

Agree. As current users don't report power-on issue on rtw88.
If the problem presents again, we can spend time to dig the difference
of power-on sequence between vendor driver and rtw88.

> 
> >>
> >> The PHY status size field is 4 bits wide, but in rtw88 its value should
> >> only be 0 or 4. Checking this catches most of the corrupt frames.
> >>
> >> If a PHY status is present, the PHY status size should not be 0.
> >>
> >> The frame size should not be less than or equal to 4 and should not
> >> exceed 11454.
> >>
> >> Discard the frame if any of these checks fail.
> >>
> >> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=221286
> >> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> >
> > Acked-by: Ping-Ke Shih <pkshih@realtek.com>
> >
> > [...]
> >
> >> diff --git a/drivers/net/wireless/realtek/rtw88/rx.c b/drivers/net/wireless/realtek/rtw88/rx.c
> >> index d9e11343d498..65f6db3d7fcb 100644
> >> --- a/drivers/net/wireless/realtek/rtw88/rx.c
> >> +++ b/drivers/net/wireless/realtek/rtw88/rx.c
> >> @@ -3,6 +3,7 @@
> >>   */
> >>
> >>  #include "main.h"
> >> +#include "mac.h"
> >>  #include "rx.h"
> >>  #include "ps.h"
> >>  #include "debug.h"
> >> @@ -261,9 +262,9 @@ static void rtw_rx_fill_rx_status(struct rtw_dev *rtwdev,
> >>         }
> >>  }
> >>
> >> -void rtw_rx_query_rx_desc(struct rtw_dev *rtwdev, void *rx_desc8,
> >> -                         struct rtw_rx_pkt_stat *pkt_stat,
> >> -                         struct ieee80211_rx_status *rx_status)
> >> +int rtw_rx_query_rx_desc(struct rtw_dev *rtwdev, void *rx_desc8,
> >> +                        struct rtw_rx_pkt_stat *pkt_stat,
> >> +                        struct ieee80211_rx_status *rx_status)
> >>  {
> >>         u32 desc_sz = rtwdev->chip->rx_pkt_desc_sz;
> >>         struct rtw_rx_desc *rx_desc = rx_desc8;
> >> @@ -303,12 +304,25 @@ void rtw_rx_query_rx_desc(struct rtw_dev *rtwdev, void *rx_desc8,
> >>                 pkt_stat->bw = RTW_CHANNEL_WIDTH_20;
> >
> > Do you think if we should return -EINVAL for this case too?
> >
> 
> Yes. What do we do with the debug message? Should the other
> conditions also have a debug message?

Personally I'd remove the debug message. If you think they are helpful to
dig problems (for unconsidered corner cases), it is fine to me to add
a message for each condition.

I suppose you will send a new patch, and I'll wait for v2.

> 
> >>         }
> >>
> >> +       if (unlikely(pkt_stat->drv_info_sz &&
> >> +                    pkt_stat->drv_info_sz != PHY_STATUS_SIZE))
> >> +               return -EINVAL;
> >> +
> >> +       if (unlikely(pkt_stat->phy_status && !pkt_stat->drv_info_sz))
> >> +               return -EINVAL;
> >> +
> >> +       if (unlikely(pkt_stat->pkt_len > IEEE80211_MAX_MPDU_LEN_VHT_11454))
> >> +               return -EINVAL;
> >> +
> >>         /* drv_info_sz is in unit of 8-bytes */
> >>         pkt_stat->drv_info_sz *= 8;
> >>
> >>         /* c2h cmd pkt's rx/phy status is not interested */
> >>         if (pkt_stat->is_c2h)
> >> -               return;
> >> +               return 0;
> >> +
> >> +       if (unlikely(pkt_stat->pkt_len <= FCS_LEN))
> >> +               return -EINVAL;
> >>
> >>         phy_status = rx_desc8 + desc_sz + pkt_stat->shift;
> >>         hdr = phy_status + pkt_stat->drv_info_sz;
> >
> > [...]
> >