[PATCH rtw-next] wifi: rtw88: sdio: reject oversized RX packets

[PATCH rtw-next] wifi: rtw88: sdio: reject oversized RX packets

[Luka Gejak]

rtw_sdio_rxfifo_recv() also takes the last-entry path when the descriptor
claims more data than rx_len. The skb is then passed to rtw_sdio_rx_skb(),
which calls skb_put() with pkt_len and does not check it against the space
left, so a bad descriptor can overflow the skb.

Reject the packet when pkt_offset + pkt_len exceeds the bytes left.

Fixes: 65371a3f14e7 ("wifi: rtw88: sdio: Add HCI implementation for SDIO based chipsets")
Cc: stable@vger.kernel.org
Signed-off-by: Luka Gejak <luka.gejak@linux.dev>
---
 drivers/net/wireless/realtek/rtw88/sdio.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
index dc2fd0f8f9ff..3a86fdcde367 100644
--- a/drivers/net/wireless/realtek/rtw88/sdio.c
+++ b/drivers/net/wireless/realtek/rtw88/sdio.c
@@ -1389,6 +1389,14 @@ 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);
 
+		if (unlikely(pkt_offset + pkt_stat.pkt_len > rx_len)) {
+			rtw_dbg(rtwdev, RTW_DBG_SDIO,
+				"oversized RX packet: %u, %u bytes left\n",
+				pkt_offset + pkt_stat.pkt_len, rx_len);
+			dev_kfree_skb_any(skb);
+			return;
+		}
+
 		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
-- 
2.55.0

RE: [PATCH rtw-next] wifi: rtw88: sdio: reject oversized RX packets

[Ping-Ke Shih]

Luka Gejak <luka.gejak@linux.dev> wrote:
> --- a/drivers/net/wireless/realtek/rtw88/sdio.c
> +++ b/drivers/net/wireless/realtek/rtw88/sdio.c
> @@ -1389,6 +1389,14 @@ 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);
> 
> +               if (unlikely(pkt_offset + pkt_stat.pkt_len > rx_len)) {

Can you use real hardware to test this patch to see if there is actual case
falling into this branch? (see below)

> +                       rtw_dbg(rtwdev, RTW_DBG_SDIO,
> +                               "oversized RX packet: %u, %u bytes left\n",
> +                               pkt_offset + pkt_stat.pkt_len, rx_len);
> +                       dev_kfree_skb_any(skb);
> +                       return;
> +               }
> +
>                 if ((curr_pkt_len + pkt_desc_sz) >= rx_len) {

Because I wonder if this condition is right? In my perspective, the
curr_pkt_len = pkt_offset          + pkt_stat.pkt_len + align_bytes
             = pkt_desc_sz + ...   + pkt_stat.pkt_len + align_bytes

Why does the if-condition plus pkt_desc_sz twice and compare with rx_len?

However, it should enter to handle/free the last aggregated packet. 
Can you enlighten me how it looks like that?

>                         /* Use the original skb (with it's adjusted offset)
>                          * when processing the last (or even the only) entry to
> --
> 2.55.0

Re: [PATCH rtw-next] wifi: rtw88: sdio: reject oversized RX packets

[Luka Gejak]

October 12, 2026 at 3:53 AM, "Ping-Ke Shih" <pkshih@realtek.com> wrote:

> 
> Luka Gejak <luka.gejak@linux.dev> wrote:
> 
> > 
> > --- a/drivers/net/wireless/realtek/rtw88/sdio.c
> >  +++ b/drivers/net/wireless/realtek/rtw88/sdio.c
> >  @@ -1389,6 +1389,14 @@ 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);
> >  
> >  + if (unlikely(pkt_offset + pkt_stat.pkt_len > rx_len)) {
> > 
> Can you use real hardware to test this patch to see if there is actual case
> falling into this branch? (see below)
> 

Sure, it does happen. I ran it on the 8723BS with debug_mask=0x40000 for 3
hours 27 minutes, cycling iperf3 RX traffic, scans and reassociations, and
the message came out twice. Both times the descriptor claimed more bytes
than were left in that transfer, so this is not just a theoretical bound.
The aggregate gets dropped, the link stays up, and nothing else stood out
at the time.

> > 
> > + rtw_dbg(rtwdev, RTW_DBG_SDIO,
> >  + "oversized RX packet: %u, %u bytes left\n",
> >  + pkt_offset + pkt_stat.pkt_len, rx_len);
> >  + dev_kfree_skb_any(skb);
> >  + return;
> >  + }
> >  +
> >  if ((curr_pkt_len + pkt_desc_sz) >= rx_len) {
> > 
> Because I wonder if this condition is right? In my perspective, the
> curr_pkt_len = pkt_offset + pkt_stat.pkt_len + align_bytes
>  = pkt_desc_sz + ... + pkt_stat.pkt_len + align_bytes
> 
> Why does the if-condition plus pkt_desc_sz twice and compare with rx_len?
> 

pkt_desc_sz turns up twice because two descriptors are involved. The one
inside pkt_offset belongs to this entry:

        pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz + pkt_stat.shift;
        curr_pkt_len = ALIGN(pkt_offset + pkt_stat.pkt_len,
                             RTW_SDIO_DATA_PTR_ALIGN);

rx_len is what is left of the transfer from this descriptor on, since the
loop subtracts curr_pkt_len from it each time round. The second pkt_desc_sz
is the header of the entry after this one, so

        (curr_pkt_len + pkt_desc_sz) >= rx_len

means there is no room left for another descriptor. That makes this the last
entry, and the original skb can go up instead of a copy so the stack frees
it. Without that term, whenever less than pkt_desc_sz is left behind this
entry, the loop would come round once more and read a descriptor out of
padding. curr_pkt_len is rounded up to 8, so it stops up to 7 bytes early,
which only decides whether the last packet is copied or not.

> However, it should enter to handle/free the last aggregated packet. 
> Can you enlighten me how it looks like that?
> 

For a sane aggregate that is exactly what happens. My check reacts to the
other case: the descriptor claims more bytes than were read, so curr_pkt_len
is already past rx_len and this same branch hands the entry to
rtw_sdio_rx_skb() with a pkt_len the transfer does not have. I would rather
drop that entry and the rest of the transfer than pass it up, and after a
descriptor like that the offset of the next one cannot be trusted anyway.

Best regards,
Luka Gejak

RE: [PATCH rtw-next] wifi: rtw88: sdio: reject oversized RX packets

[Ping-Ke Shih]

Luka Gejak <luka.gejak@linux.dev> wrote:
> October 12, 2026 at 3:53 AM, "Ping-Ke Shih" <pkshih@realtek.com> wrote:
> 
> >
> > Luka Gejak <luka.gejak@linux.dev> wrote:
> >
> > >
> > > --- a/drivers/net/wireless/realtek/rtw88/sdio.c
> > >  +++ b/drivers/net/wireless/realtek/rtw88/sdio.c
> > >  @@ -1389,6 +1389,14 @@ 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);
> > >
> > >  + if (unlikely(pkt_offset + pkt_stat.pkt_len > rx_len)) {
> > >
> > Can you use real hardware to test this patch to see if there is actual case
> > falling into this branch? (see below)
> >
> 
> Sure, it does happen. I ran it on the 8723BS with debug_mask=0x40000 for 3
> hours 27 minutes, cycling iperf3 RX traffic, scans and reassociations, and
> the message came out twice. Both times the descriptor claimed more bytes
> than were left in that transfer, so this is not just a theoretical bound.
> The aggregate gets dropped, the link stays up, and nothing else stood out
> at the time.
> 
> > >
> > > + rtw_dbg(rtwdev, RTW_DBG_SDIO,
> > >  + "oversized RX packet: %u, %u bytes left\n",
> > >  + pkt_offset + pkt_stat.pkt_len, rx_len);
> > >  + dev_kfree_skb_any(skb);
> > >  + return;
> > >  + }
> > >  +
> > >  if ((curr_pkt_len + pkt_desc_sz) >= rx_len) {
> > >
> > Because I wonder if this condition is right? In my perspective, the
> > curr_pkt_len = pkt_offset + pkt_stat.pkt_len + align_bytes
> >  = pkt_desc_sz + ... + pkt_stat.pkt_len + align_bytes
> >
> > Why does the if-condition plus pkt_desc_sz twice and compare with rx_len?
> >
> 
> pkt_desc_sz turns up twice because two descriptors are involved. The one
> inside pkt_offset belongs to this entry:
> 
>         pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz + pkt_stat.shift;
>         curr_pkt_len = ALIGN(pkt_offset + pkt_stat.pkt_len,
>                              RTW_SDIO_DATA_PTR_ALIGN);
> 
> rx_len is what is left of the transfer from this descriptor on, since the
> loop subtracts curr_pkt_len from it each time round. The second pkt_desc_sz
> is the header of the entry after this one, so
> 
>         (curr_pkt_len + pkt_desc_sz) >= rx_len
> 
> means there is no room left for another descriptor. That makes this the last
> entry, and the original skb can go up instead of a copy so the stack frees
> it. Without that term, whenever less than pkt_desc_sz is left behind this
> entry, the loop would come round once more and read a descriptor out of
> padding. curr_pkt_len is rounded up to 8, so it stops up to 7 bytes early,
> which only decides whether the last packet is copied or not.
> 
> > However, it should enter to handle/free the last aggregated packet.
> > Can you enlighten me how it looks like that?
> >
> 
> For a sane aggregate that is exactly what happens. My check reacts to the
> other case: the descriptor claims more bytes than were read, so curr_pkt_len
> is already past rx_len and this same branch hands the entry to
> rtw_sdio_rx_skb() with a pkt_len the transfer does not have. I would rather
> drop that entry and the rest of the transfer than pass it up, and after a
> descriptor like that the offset of the next one cannot be trusted anyway.

Thanks for the clarification. 

Acked-by: Ping-Ke Shih <pkshih@realtek.com>