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

On Sunday, August 2nd, 2026 at 8:24 AM, Willem de Bruijn <[email protected]> wrote:

> manizada wrote:
> >
> >
> >
> >
> > On Saturday, July 25th, 2026 at 2:37 PM, Willem de Bruijn <[email protected]> wrote:
> >
> > > manizada wrote:
> > > >
> > > >
> > > >
> > > >
> > > > On Thursday, July 23rd, 2026 at 10:14 AM, Jakub Kicinski <[email protected]> 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.

Yep, I think it makes sense to have this as an extra defensive check.

> 
> > 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.

Unfortunately, yes -- I don't think clamping good_linear to zero restores
that bound on the ordinary tun_alloc_skb() path. With good_linear ==
linear == 0, align remains prepad. tun_alloc_skb() passes prepad + linear
as header_len through sock_alloc_send_pskb() and alloc_skb_with_frags() to
alloc_skb(), and then skb_reserve(prepad) leaves skb_headroom() equal to
align. Pulling packet bytes linear doesn't reduce that headroom. The path
is basically this:

  good_linear = 0
  linear = 0

  tun_alloc_skb(prepad=align, linear=0)
      sock_alloc_send_pskb(header_len=align, data_len=len)
          alloc_skb_with_frags(header_len=align, ...)
              alloc_skb(align)

  skb_reserve(skb, align)

I actually just tested the exact zero clamp plus both pulls on the same 7.2
rc3. With effective align 4160, skb_headroom() was 4160 both before and
after the pull. And with effective align 65535, it was 65535 both before
and after the pull; the subsequent mac and network header resets stored
0xffff. So 96f8d9ecf227's expected 1-page allocation invariant isn't
respected.

So I think the pulls can remain as defensive parsing, but the ordinary
allocation path still needs some explicit TUN-side bound -- or a rejection
derived from the available skb-head budget and the required linear bytes:
1 byte for raw TUN and ETH_HLEN for TAP, accounting for NET_IP_ALIGN.

> 
> > 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?
> >
> 
> 
>