RE: [PATCH rtw-next v3 2/2] wifi: rtw88: check the C2H payload length before reading it

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

Huihui Huang <hhhuang@smu.edu.sg> wrote:
> With the previous patch, a C2H packet holds at least the 2-byte C2H
> header, but its payload can still be shorter than what the handlers
> read. 

If you want to describe "previous patch", say it in cover-letter. Just
describe what you do in this patch.

> These handlers read fixed payload offsets without checking the
> payload length:
> 
>  - rtw_fw_scan_result() reads payload[0];
>  - rtw_coex_info_response() reads payload[0];
>  - rtw_fw_c2h_cmd_handle_ext() reads the sub-command ID in payload[0],
>    and rtw_hw_scan_status_report() and rtw_hw_scan_chan_switch() read
>    up to payload[2] and payload[4];
>  - rtw_tx_report_handle() reads up to payload[6] for C2H_CCX_TX_RPT and
>    payload[9] for C2H_CCX_RPT.

You should think how to summarize these in subject... 

> 
> With a short payload, these reads are beyond skb->len.
> 
> Add rtw_c2h_payload_len() to get the payload length of a C2H skb, and
> ignore C2H packets whose payload is too short for the fields the
> handler reads. For C2H_SCAN_RESULT, check the length before completing
> fw_scan_density, so that a report without payload does not wake the
> waiter. A short HW scan status report is not ignored but completes the
> scan as aborted; otherwise the scan stays pending until something
> aborts it.
> 
> For coexistence responses, this only validates the byte inspected by
> rtw_coex_info_response(). Length validation for the queued response's
> GET_COEX_RESP_*() consumers is left for a separate follow-up.

The detail of what the patch does isn't so useful for reviewers. LLM can
help to generate many stuffs (but not helpful). Please remove unnecessary
part. Reading long long commit message spends a lot of time.

> 
> Fixes: e3037485c68e ("rtw88: new Realtek 802.11ac driver")
> Fixes: 4136214f7c46 ("rtw88: add BT co-existence support")
> Fixes: a853d234e179 ("rtw88: notify fw when driver in scan-period to avoid potential problem")
> Fixes: 10d162b2ed39 ("rtw88: 8822c: add ieee80211_ops::hw_scan")

Should we need these Fixes? I think the changes of this patch should not
happen. 

> Suggested-by: Luka Gejak <luka.gejak@linux.dev>
> Assisted-by: LLM

No this tag now.

> Signed-off-by: Huihui Huang <hhhuang@smu.edu.sg>
> ---
>  drivers/net/wireless/realtek/rtw88/coex.c |  3 ++-
>  drivers/net/wireless/realtek/rtw88/fw.c   | 17 +++++++++++++++--
>  drivers/net/wireless/realtek/rtw88/fw.h   |  8 ++++++++
>  drivers/net/wireless/realtek/rtw88/tx.c   |  3 +++
>  4 files changed, 28 insertions(+), 3 deletions(-)
> 
> v3: complete the HW scan as aborted on a short scan status report
>     instead of ignoring it (Luka).
> v2: new patch (Luka).
> 
> Tested on rtw-next on top of patch 1 with a temporary KUnit test (UML).
> C2H packets with the 2-byte header and 0 or 1 payload byte are passed
> to rtw_fw_c2h_cmd_rx_irqsafe(); the byte after the header is set to a
> value the handler would act on. Verbatim KUnit output:
> 
>   before this change:
>     # rtw_c2h_scan_result_no_payload_test: payload_len=0 scan_density=0xab completed=1 (want 0x00, 0)
>     not ok 1 rtw_c2h_scan_result_no_payload_test
>     # rtw_c2h_scan_result_one_byte_test: payload_len=1 scan_density=0xab completed=1 (want 0xab, 1)
>     ok 2 rtw_c2h_scan_result_one_byte_test
>     # rtw_c2h_coex_resp_no_payload_test: payload_len=0 coex queue len=1 (want 0)
>     not ok 3 rtw_c2h_coex_resp_no_payload_test
>     # rtw_c2h_coex_resp_one_byte_test: payload_len=1 coex queue len=1 (want 1)
>     ok 4 rtw_c2h_coex_resp_one_byte_test
>   after this change:
>     # rtw_c2h_scan_result_no_payload_test: payload_len=0 scan_density=0x00 completed=0 (want 0x00, 0)
>     ok 1 rtw_c2h_scan_result_no_payload_test
>     # rtw_c2h_scan_result_one_byte_test: payload_len=1 scan_density=0xab completed=1 (want 0xab, 1)
>     ok 2 rtw_c2h_scan_result_one_byte_test
>     # rtw_c2h_coex_resp_no_payload_test: payload_len=0 coex queue len=0 (want 0)
>     ok 3 rtw_c2h_coex_resp_no_payload_test
>     # rtw_c2h_coex_resp_one_byte_test: payload_len=1 coex queue len=1 (want 1)
>     ok 4 rtw_c2h_coex_resp_one_byte_test
> 
> The HALMAC paths, including the HW scan status report change in v3,
> and the TX report path were only compile-tested. Not tested on real
> hardware.
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/coex.c b/drivers/net/wireless/realtek/rtw88/coex.c
> index 37c336def419..e2c91efc529d 100644
> --- a/drivers/net/wireless/realtek/rtw88/coex.c
> +++ b/drivers/net/wireless/realtek/rtw88/coex.c
> @@ -643,7 +643,8 @@ void rtw_coex_info_response(struct rtw_dev *rtwdev, struct sk_buff *skb)
>         struct rtw_coex *coex = &rtwdev->coex;
>         u8 *payload = get_payload_from_coex_resp(skb);
> 
> -       if (payload[0] != COEX_RESP_ACK_BY_WL_FW) {
> +       if (rtw_c2h_payload_len(skb) < 1 ||
> +           payload[0] != COEX_RESP_ACK_BY_WL_FW) {
>                 dev_kfree_skb_any(skb);
>                 return;
>         }
> diff --git a/drivers/net/wireless/realtek/rtw88/fw.c b/drivers/net/wireless/realtek/rtw88/fw.c
> index 0a909bf79cf5..18d3689054e6 100644
> --- a/drivers/net/wireless/realtek/rtw88/fw.c
> +++ b/drivers/net/wireless/realtek/rtw88/fw.c
> @@ -96,6 +96,9 @@ static void rtw_fw_c2h_cmd_handle_ext(struct rtw_dev *rtwdev,
>         struct rtw_c2h_cmd *c2h;
>         u8 sub_cmd_id;
> 
> +       if (rtw_c2h_payload_len(skb) < 1)
> +               return;
> +
>         c2h = get_c2h_from_skb(skb);
>         sub_cmd_id = c2h->payload[0];
> 
> @@ -376,8 +379,10 @@ void rtw_fw_c2h_cmd_rx_irqsafe(struct rtw_dev *rtwdev, u32 pkt_offset,
>                 dev_kfree_skb_any(skb);
>                 break;
>         case C2H_SCAN_RESULT:
> -               complete(&rtwdev->fw_scan_density);
> -               rtw_fw_scan_result(rtwdev, c2h->payload, len);
> +               if (len) {

If the c2h->id is SCAN already, validate 'len' in rtw_fw_scan_result().

I don't want to add condition which I think no possible happen.

> +                       complete(&rtwdev->fw_scan_density);
> +                       rtw_fw_scan_result(rtwdev, c2h->payload, len);
> +               }
>                 dev_kfree_skb_any(skb);
>                 break;
>         default:
> @@ -2381,6 +2386,11 @@ void rtw_hw_scan_status_report(struct rtw_dev *rtwdev, struct sk_buff *skb)
>         if (!test_bit(RTW_FLAG_SCANNING, rtwdev->flags))
>                 return;
> 
> +       if (rtw_c2h_payload_len(skb) < 3) {

The 3 is because of GET_SCAN_REPORT_RETURN_CODE(). How does people understand
what 3 means?

Maybe, we should define specific struct for status_report, like

struct status_report {
	u8 id;
	u8 seq; 
	u8 rsvd[2]
	u8 retrun_code;
};

struct status_report *status_report = get_c2h_from_skb(skb);

if (rtw_c2h_payload_len(skb) < sizeof(*status_report))


> +               rtw_hw_scan_complete(rtwdev, vif, true);
> +               return;
> +       }
> +
>         c2h = get_c2h_from_skb(skb);
>         rc = GET_SCAN_REPORT_RETURN_CODE(c2h->payload);
>         aborted = rc != RTW_SCAN_REPORT_SUCCESS;
> @@ -2436,6 +2446,9 @@ void rtw_hw_scan_chan_switch(struct rtw_dev *rtwdev, struct sk_buff *skb)
>         if (!test_bit(RTW_FLAG_SCANNING, rtwdev->flags))
>                 return;
> 
> +       if (rtw_c2h_payload_len(skb) < 5)

ditto. This is ever harder to know why here is 5... 

> +               return;
> +
>         c2h = get_c2h_from_skb(skb);
>         chan = GET_CHAN_SWITCH_CENTRAL_CH(c2h->payload);
>         id = GET_CHAN_SWITCH_ID(c2h->payload);
> diff --git a/drivers/net/wireless/realtek/rtw88/fw.h b/drivers/net/wireless/realtek/rtw88/fw.h
> index 48ad9ceab6ea..6713dfac8c94 100644
> --- a/drivers/net/wireless/realtek/rtw88/fw.h
> +++ b/drivers/net/wireless/realtek/rtw88/fw.h
> @@ -810,6 +810,14 @@ static inline struct rtw_c2h_cmd *get_c2h_from_skb(struct sk_buff *skb)
>         return (struct rtw_c2h_cmd *)(skb->data + pkt_offset);
>  }
> 
> +/* The C2H header has been validated by rtw_fw_c2h_cmd_rx_irqsafe(). */
> +static inline u32 rtw_c2h_payload_len(struct sk_buff *skb)
> +{
> +       u32 pkt_offset = *((u32 *)skb->cb);
> +
> +       return skb->len - pkt_offset - sizeof(struct rtw_c2h_cmd);
> +}
> +
>  static inline bool rtw_fw_feature_check(struct rtw_fw_state *fw,
>                                         enum rtw_fw_feature feature)
>  {
> diff --git a/drivers/net/wireless/realtek/rtw88/tx.c b/drivers/net/wireless/realtek/rtw88/tx.c
> index 701ed7c4f3f7..ddfddd7cb4db 100644
> --- a/drivers/net/wireless/realtek/rtw88/tx.c
> +++ b/drivers/net/wireless/realtek/rtw88/tx.c
> @@ -241,6 +241,9 @@ void rtw_tx_report_handle(struct rtw_dev *rtwdev, struct sk_buff *skb, int src)
>         u8 sn, st;
>         u8 *n;
> 
> +       if (rtw_c2h_payload_len(skb) < (src == C2H_CCX_TX_RPT ? 7 : 10))

Ditto. 7 and 10.... very hard to understand. 

> +               return;
> +
>         c2h = get_c2h_from_skb(skb);
> 
>         if (src == C2H_CCX_TX_RPT) {
> --
> 2.43.0
>