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:
> 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]. 

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

> 
> 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?

>         }
> 
> +       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;

[...]