* Re: [PATCH] wifi: rtw88: sdio: check packet length against remaining RX data
2026-11-08 22:27 [PATCH] wifi: rtw88: sdio: check packet length against remaining RX data Noah Weinberger
@ 2026-11-10 1:34 ` Luka Gejak
0 siblings, 0 replies; 2+ messages in thread
From: Luka Gejak @ 2026-11-10 1:34 UTC (permalink / raw)
To: linux-wireless, Noah Weinberger, pkshih
Cc: linux-wireless, linux-kernel, security
Hi Noah,
Thanks for looking into this. A few comments below.
On November 8, 2026 11:27:09 PM GMT+01:00, Noah Weinberger
<NoahWeinberger2003@proton.me> wrote:
> Subject: [PATCH] wifi: rtw88: sdio: check packet length against remaining RX data
This doesn't apply to rtw-next, which is the tree rtw88 patches should be
based on, because the same check is already there. I sent it in October and
Ping-Ke applied it as
83ca360e8528 ("wifi: rtw88: sdio: reject oversized RX packets")
It tests pkt_offset + pkt_len > rx_len at the same place, so this patch can
be dropped.
For future rtw88 patches, please base them on rtw-next and put that in the
subject as [PATCH rtw-next].
> Hello,
This would end up in the git log, so please leave the greeting out.
> rtw_sdio_rxfifo_recv() validates the descriptor's pkt_len only against
> the absolute maximum (11454), never against the amount of data actually
> transferred (rx_len). The last-packet branch is taken precisely when
> curr_pkt_len + pkt_desc_sz >= rx_len - which includes the case where the
> descriptor *overshoots* the buffer - and rtw_sdio_rx_skb() then calls
> skb_put(skb, pkt_stat.pkt_len) on a buffer allocated for the aligned
> rx_len.
The length checks are in rtw_rx_query_rx_desc() rather than here, and there
is a lower bound too.
Commit messages are plain text, so please drop the asterisks and the dashes
around that aside as well.
> The loop's own comment documents that the 8051-class chips can produce
> packets whose pkt_len is zero or points beyond the end of the buffer
> when the RX fill race hits; pkt_len == 0 is guarded, the overshoot is
> not. An inconsistent descriptor therefore reaches skb_over_panic() ->
> BUG.
That comment is in rtw_sdio_rx_isr(), not in this loop, and it explains why
the driver stops when no RX request is pending. So it describes a case the
driver already avoids there, and I don't think it explains the overshoot
here.
The zero length check also only covers normal frames, because
rtw_rx_query_rx_desc() returns for C2H before it gets there.
The BUG isn't always reached either. For normal frames skb_put() only gets
pkt_len, and pkt_offset is applied after it:
skb_put(skb, pkt_stat->pkt_len);
skb_reserve(skb, pkt_offset);
So a small overshoot gets past skb_put() without a BUG.
> Affected version/commit: mainline
> fa4ad798f6f681031775266044ba1b0cf3c670bd (v7.4-rc1, 2026-11-07),
> drivers/net/wireless/realtek/rtw88/sdio.c:1352-1419
> (rtw_sdio_rxfifo_recv), sdio.c:1331-1350 (rtw_sdio_rx_skb), rx.c:295-320
> (rtw_rx_query_rx_desc).
> Loop predates this window (8703B); newly reachable via 8723BS support
> added in this window.
fa4ad798f6f6 is not v7.4-rc1, which is 6628e7d4c6b2 from Nov 1. Line
numbers
go stale quickly and the first line is well over 75 columns, so please
leave
this out of the commit message.
"Newly reachable" isn't right. The loop came with the SDIO support in
65371a3f14e7, and 8822BS, 8822CS and 8821CS have used it since then. It is
also not clear to me what "this window" refers to.
> [ ... ] every code path cited below was re-verified by the
> reporter against that tree. The patch below has NOT been compiled and has
> NOT been runtime tested, and no reproducer was produced - the triggering
> conditions are described above instead. Please treat this as a report for
> verification rather than a battle-tested fix.
This is a note for reviewers, so it belongs under the --- line and not in
the git log. The code paths are cited above, not below, and since you are
the reporter, writing it in the first person would read more naturally.
Please build a patch before sending it,
Documentation/process/submit-checklist.rst asks for that. If you only want
someone to verify a report, maybe RFC would fit better than a plain PATCH.
You also sent this to security@kernel.org.
Documentation/process/threat-model.rst treats devices reporting values
outside their documented ranges as regular bugs, so that list should be
left out of this one.
> Reported-by: Noah Weinberger <noahweinberger2003@proton.me>
There is no sign off on this patch, so it couldn't be applied even without
the conflict. This tag isn't needed on your own patch, and checkpatch also
asks for a Closes or Link tag after it.
A fix like this should also have a Fixes tag:
Fixes: 65371a3f14e7 ("wifi: rtw88: sdio: Add HCI implementation for SDIO based chipsets")
> diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
> index dc2fd0f..0a07d8d 100644
> --- a/drivers/net/wireless/realtek/rtw88/sdio.c
> +++ b/drivers/net/wireless/realtek/rtw88/sdio.c
[ ... ]
> + /* The device may provide an inconsistent descriptor whose
> + * packet length points beyond the received data; do not let
> + * skb_put() run past the allocation in that case.
> + */
I think this comment can go, the check is clear without it. It also isn't
quite accurate. skb_put() would stop with skb_over_panic() rather than run
past the allocation, and the check compares with rx_len, not with the
allocation.
The style is wrong either way, a multi line comment should start with /* on
its own line.
Best regards,
Luka Gejak
^ permalink raw reply [flat|nested] 2+ messages in thread