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

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

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>