Re: [PATCH net] tun: prevent underflow in rx headroom calculation

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

manizada wrote:
> 
> 
> 
> 
> On Thursday, July 23rd, 2026 at 10:14 AM, Jakub Kicinski <kuba@kernel.org> wrote:
> 
> > This is an AI-generated review of your patch. The human sending this
> > email has considered the AI review valid, or at least plausible.
> > ---
> > tun: prevent underflow in rx headroom calculation
> > 
> > This patch clamps good_linear to zero in tun_get_user() so that a large
> > tun->align value coming from ndo_set_rx_headroom() does not cause
> > SKB_MAX_HEAD(align) to underflow into a huge size_t when assigned to the
> > linear variable, and later cause the skb data pointer to end up outside
> > the allocated head.
> > 
> > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > > index ffbe6f13fb1f..d3be0f2d5df8 100644
> > > --- a/drivers/net/tun.c
> > > +++ b/drivers/net/tun.c
> > > @@ -1780,6 +1780,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
> > >  	}
> > >
> > >  	good_linear = SKB_MAX_HEAD(align);
> > > +	good_linear = max(good_linear, 0);
> > 
> > [High]
> > Does this fully cover the IFF_TAP path when align is large enough that
> > SKB_MAX_HEAD(align) is <= 0?
> > 
> > With the clamp, good_linear becomes 0, and later in tun_get_user() the
> > non-zerocopy path does:
> > 
> >     copylen = len;
> >     linear = min(hdr_len, good_linear);
> >     ...
> >     if (!linear)
> >         linear = min_t(size_t, good_linear, copylen);
> >     skb = tun_alloc_skb(tfile, align, copylen, linear, noblock);
> > 
> > so linear ends up 0 and the resulting skb has skb_headlen(skb) == 0.
> > 
> > For IFF_TAP without IFF_NAPI_FRAGS the code then calls:
> > 
> >     skb->protocol = eth_type_trans(skb, tun->dev);
> > 
> > eth_type_trans() -> eth_skb_pull_mac() -> __skb_pull(skb, ETH_HLEN),
> > and __skb_pull() has:
> > 
> >     if (unlikely(skb->len < skb->data_len))
> >         BUG();
> > 
> > Since skb->len and skb->data_len both start at the packet length and
> > skb->len is decremented by ETH_HLEN in the pull, would this reach the
> > BUG() when a user writes to /dev/net/tun with tun->align pushed above
> > the SKB_MAX_HEAD threshold via the OVS/netkit/VXLAN stacking described
> > in the commit message?
> > 
> > The pre-existing behavior at align == 3776 appears to hit the same
> > BUG() and align > 3776 pre-patch corrupted memory before reaching it,
> > so this change is an improvement, but would forcing linear >= ETH_HLEN
> > in the IFF_TAP path (or calling pskb_may_pull() before eth_type_trans()
> > regardless of the frags flag) be needed to close the remaining panic?
> 
> Oh this is a good catch. But while making the TAP pskb_may_pull
> (ETH_HLEN) check unconditional would address the eth_type_trans() case,
> raw TUN with IFF_NO_PI also directly reads the first protocol byte from
> skb->data, so we'd have the same issue there.
> 
> Rather than add consumer-side handling for a fully nonlinear skb state
> introduced by the repair, maybe the cleanest is preventing TUN from
> creating this state?
> 
> 	#define TUN_MAX_HEADROOM 512
> 
> 	tun->align = clamp(new_hr, NET_SKB_PAD, TUN_MAX_HEADROOM);
> 
> This fixes the original SKB_MAX_HEAD() arithmetic issue, and also preserves
> linear space for both the raw-TUN protocol byte and the TAP Ethernet
> header. The 512-byte value matches the existing ceiling in
> ip_tunnel_adj_headroom(), so requests above the cap can require later

Is that a ceiling specific to ip_tunnel or universal for tuntap? I
suspect only the second. In which case this would add a new condition.

> skb expansion instead of unsafe preallocation by TUN.
> 
> Does this sound reasonable? If so, I can post v2 using the TUN-side cap.

Why NET_SKB_PAD, aside from that it happens to be larger than both
IFF_TAP and IFF_NO_PI cases?  (good catch on that NO_PI btw.)

The IFF_TAP case already has a pskb_may_pull, but only for frags.
Perhaps we should just always enable that. And a similar check before
the NO_PI case reads skb->data[0]. That is in line with standard rx
protocol parsing logic.

Alternatively indeed clamp, but to the true minimum required values in
these cases, which coincide with the values tested in pskb_may_pull.