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

["Luka Gejak" <luka.gejak@linux.dev>]

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