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

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

manizada wrote:
> 
> 
> 
> 
> On Sunday, August 2nd, 2026 at 8:24 AM, Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:
> 
> > 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.
> 
> 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.

Yes, I agree with your previous approach to hardcode those lower bounds.