Re: net: thunderbolt: tbnet_poll() can overflow skb_shinfo()->frags[]

[Mika Westerberg <mika.westerberg@linux.intel.com>]

Hi,

On Tue, Jun 16, 2026 at 03:34:52PM +0800, Maoyi Xie wrote:
> Hi all,
> 
> After the recent skb frags[] overflow fixes (t7xx, cdc-phonet, f_phonet), I
> went looking for the same pattern. I think tbnet_poll() in
> drivers/net/thunderbolt/main.c has it too. I would appreciate it if you could
> take a look.
> 
> tbnet_poll() reassembles a ThunderboltIP packet that spans several frames into
> one skb. It adds one rx fragment per frame.
> 
> 	skb = net->skb;
> 	if (!skb) {
> 		skb = build_skb(...);
> 		...
> 		net->skb = skb;
> 	} else {
> 		skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
> 				page, hdr_size, frame_size,
> 				TBNET_RX_PAGE_SIZE - hdr_size);
> 	}
> 
> Nothing checks skb_shinfo(skb)->nr_frags against MAX_SKB_FRAGS here. The frame
> count comes from the peer, in the frame header. tbnet_check_frame() only bounds
> it at the start of a packet.
> 
> 	if (frame_count == 0 || frame_count > TBNET_RING_SIZE / 4) {
> 		net->stats.rx_length_errors++;
> 		return false;
> 	}
> 
> TBNET_RING_SIZE is 256, so frame_count can be as large as 64. MAX_SKB_FRAGS is 17
> by default. Frame 0 builds the skb and every frame after it adds a fragment, so
> nr_frags can reach 63. Once nr_frags hits MAX_SKB_FRAGS, skb_add_rx_frag() writes
> one entry past skb_shinfo()->frags[]. The frame_size and MTU checks do not stop
> this. With small frames, 64 fragments stay well under TBNET_MAX_MTU.
> 
> So a malicious or buggy peer can send a packet with frame_count between 19 and
> 64. The frames only need to increment the way tbnet_check_frame() wants. That
> drives nr_frags past frags[] and overruns skb_shared_info.

I agree this can happen.

> The fix I had in mind mirrors f0813bcd2d9d ("net: wwan: t7xx: fix potential
> skb->frags overflow in RX path") and 600dc40554dc ("net: usb: cdc-phonet: fix
> skb frags[] overflow in rx_complete()"). Add the fragment only while there is
> room, and drop the packet otherwise.
> 
> 	-	} else {
> 	+	} else if (skb_shinfo(skb)->nr_frags < MAX_SKB_FRAGS) {
> 			skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
> 					page, hdr_size, frame_size,
> 					TBNET_RX_PAGE_SIZE - hdr_size);
> 	+	} else {
> 	+		net->stats.rx_length_errors++;
> 	+		__free_pages(page, TBNET_RX_PAGE_ORDER);
> 	+		dev_kfree_skb_any(net->skb);
> 	+		net->skb = NULL;
> 	+		continue;
> 		}
> 
> I do not have two Thunderbolt hosts, so this is from reading the code. I can put
> together a focused reproducer if that helps.
> 
> Does this look like a real overflow? And is the MAX_SKB_FRAGS guard the right
> place, or would you rather tighten the frame_count bound in tbnet_check_frame()?
> It has been there since the driver was added (e69b6c02b4c3), so it is a stable
> candidate. Happy to send a proper patch once you confirm.

I would prefer do this in tbnet_check_frame(). Thanks!