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

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

manizada wrote:
> 
> 
> 
> 
> On Saturday, July 25th, 2026 at 2:37 PM, Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:
> 
> > 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.
> 
> Yeah you're right, that was specific to ip_tunnel_adj_headroom(), not universal.
> Sorry for the delay here, the more I look into this the more potential issues
> I find (including couple more I'll send a patch for separately):
> 
> > 
> > > 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.
> > 
> 
> With NET_SKB_PAD I just meant to preserve the existing lower-bound behavior in
> tun_set_headroom() for negative/very small requests, not set a protocol
> minimum for either side. 
> 
> RE: pull-based alternative, it sounds reasonable, and making pskb_may_pull(skb, ETH_HLEN)
> unconditional and pulling 1 byte before the IFF_NO_PI read would fix the
> immediate fully non-linear skb. 

This is probably most robust. The network stack in general supports
non-linear skbuffs and uses pskb_may_pull for safe access.

> But, those pulls would still leave
> skb_headroom() unbounded.

Would it? That good_linear limit itself was introduced to bound
headroom, in commit 96f8d9ecf227 ("tuntap: limit head length of skb
allocated").

If we ensure good_linear is safe, then all allocation paths should
create skbs with a bounded skb_headroom.

> mac_header and network_header are stored as 16-bit
> offsets from skb->head, so a large-enough headroom can still truncate
> those offsets, so just pulling the packet bytes linear would not prevent an
> overflow from excessive headroom value.
> 
> How about this instead?
> 
>   max_headroom = min_t(size_t, SKB_MAX_HEAD(0), U16_MAX - 1);
>   
>   if ((tun->flags & TUN_TYPE_MASK) == IFF_TAP)
>           max_headroom -= ETH_HLEN + NET_IP_ALIGN;
>   else
>           max_headroom -= 1;
> 
>   tun->align = clamp_t(int, new_hr, NET_SKB_PAD, max_headroom);
> 
> This leaves 1 linear byte for the IFF_NO_PI version check, or a complete Ethernet
> header for TAP, while accounting for the NET_IP_ALIGN added later via TAP.
> It also keeps the requested headroom within both the usable 1-page skb head and
> the largest valid skb header offset. The 16 bit limit would constrain just the initial
> headroom reserved by TUN, not any packet-relative offsets added in later elsewhere.
> 
> Would this make sense?
>