* [PATCH] wifi: rtw88: sdio: check packet length against remaining RX data @ 2026-11-08 22:27 Noah Weinberger 2026-11-10 1:34 ` Luka Gejak 0 siblings, 1 reply; 4+ messages in thread From: Noah Weinberger @ 2026-11-08 22:27 UTC (permalink / raw) To: pkshih@realtek.com Cc: linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org, security@kernel.org Hello, 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 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. Drop the frame instead when pkt_offset + pkt_len exceeds the remaining rx_len. 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. This was found during an assisted (AI-supported) static source review of mainline fa4ad798f6f681031775266044ba1b0cf3c670bd (v7.4-rc1, 2026-11-07); 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. Reported-by: Noah Weinberger <noahweinberger2003@proton.me> --- 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 @@ -1389,6 +1389,15 @@ static void rtw_sdio_rxfifo_recv(struct rtw_dev *rtwdev, u32 rx_len) curr_pkt_len = ALIGN(pkt_offset + pkt_stat.pkt_len, RTW_SDIO_DATA_PTR_ALIGN); + /* 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. + */ + if (pkt_offset + pkt_stat.pkt_len > rx_len) { + dev_kfree_skb_any(skb); + break; + } + if ((curr_pkt_len + pkt_desc_sz) >= rx_len) { /* Use the original skb (with it's adjusted offset) * when processing the last (or even the only) entry to ^ permalink raw reply related [flat|nested] 4+ messages in thread
* 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 2026-11-12 9:24 ` Krzysztof Kozlowski 0 siblings, 1 reply; 4+ messages in thread From: Luka Gejak @ 2026-11-10 1:34 UTC (permalink / raw) To: linux-wireless, Noah Weinberger, pkshih@realtek.com Cc: linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org, security@kernel.org 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] 4+ messages in thread
* Re: [PATCH] wifi: rtw88: sdio: check packet length against remaining RX data 2026-11-10 1:34 ` Luka Gejak @ 2026-11-12 9:24 ` Krzysztof Kozlowski 2026-11-12 9:38 ` Luka Gejak 0 siblings, 1 reply; 4+ messages in thread From: Krzysztof Kozlowski @ 2026-11-12 9:24 UTC (permalink / raw) To: Luka Gejak, linux-wireless, Noah Weinberger, pkshih@realtek.com Cc: linux-kernel@vger.kernel.org, security@kernel.org On 10/11/2026 02:34, Luka Gejak wrote: > 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 This was a report, not a patch whihc can be applied. It's visible by lack of DCO. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] wifi: rtw88: sdio: check packet length against remaining RX data 2026-11-12 9:24 ` Krzysztof Kozlowski @ 2026-11-12 9:38 ` Luka Gejak 0 siblings, 0 replies; 4+ messages in thread From: Luka Gejak @ 2026-11-12 9:38 UTC (permalink / raw) To: Krzysztof Kozlowski, linux-wireless, Noah Weinberger, pkshih@realtek.com Cc: linux-kernel@vger.kernel.org, security@kernel.org On November 12, 2026 10:24:53 AM GMT+01:00, Krzysztof Kozlowski <krzk@kernel.org> wrote: >On 10/11/2026 02:34, Luka Gejak wrote: >> 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 > >This was a report, not a patch whihc can be applied. It's visible by >lack of DCO. > > > >Best regards, >Krzysztof > As Noah wrote in private email, emphasis on submission and version of the patch: >Hi Luka, >Thanks for the detailed review >and for pointing me to the >existing fix >in >rtw-next. I’ll drop this >version of the patch. >I appreciate the corrections >and the guidance on the target >tree, >testing >and submission format. I’ll >keep those in mind for future >patches. >Thanks, >Noah Best regards, Luka Gejak ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-11-12 9:38 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 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 2026-11-12 9:24 ` Krzysztof Kozlowski 2026-11-12 9:38 ` Luka Gejak
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox