Re: [PATCH net v3 1/1] net: cap advertised IP tunnel headroom

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] The patch introduces a setup-time cap for IP tunnel headroom to prevent 16-bit SKB header offset wrapping, but relies on `pskb_expand_head()` to handle headroom growth at runtime. This fix is incomplete. An attacker can still wrap the 16-bit offsets at runtime by stacking thousands of tunnels (e.g., using user namespaces) and bypassing the recursion limits via delayed queueing disciplines (`netem`). Since `pskb_expand_head()` adds padded headroom (e.g., 64 bytes) that often exceeds the pushed header size (e.g., 40 bytes for IPv6), the total accumulated headroom wraps the 16-bit offset (65535) well before `skb->len` reaches the 65535 IP maximum MTU limit that would otherwise trigger fragmentation. When the 16-bit `mac_header` wraps to a small value, it points into the uninitialized memory of the headroom allocated via `kmalloc_reserve`. Subsystems like `AF_PACKET` will then deliver this uninitialized kernel memory to userspace.
--

--- Patch [1]: [PATCH net v3 1/1] net: cap advertised IP tunnel headroom ---
commit 19f57a4a48f9a12f973f74a2e052b267cba3faed
Author: Zhiling Zou <[email protected]>
Subject: net: cap advertised IP tunnel headroom

This commit caps the advertised needed_headroom for IP tunnel devices to
512 bytes. This is intended to prevent the 16-bit skb header offsets from
overflowing when large headrooms are derived from a stack of lower devices.

> diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
> index d708b66e55cda..85e3455cea259 100644
> --- a/include/net/ip_tunnels.h
> +++ b/include/net/ip_tunnels.h
> @@ -629,8 +629,7 @@ struct metadata_dst *iptunnel_metadata_reply(struct metadata_dst *md,
>  int skb_tunnel_check_pmtu(struct sk_buff *skb, struct dst_entry *encap_dst,
>  			  int headroom, bool reply);
>  
> -static inline void ip_tunnel_adj_headroom(struct net_device *dev,
> -					  unsigned int headroom)
> +static inline unsigned int ip_tunnel_limit_headroom(unsigned int headroom)
>  {
>  	/* we must cap headroom to some upperlimit, else pskb_expand_head
>  	 * will overflow header offsets in skb_headers_offset_update().
> @@ -640,6 +639,14 @@ static inline void ip_tunnel_adj_headroom(struct net_device *dev,
>  	if (headroom > max_allowed)
>  		headroom = max_allowed;
>  
> +	return headroom;
> +}

[Severity: Critical]
This is a pre-existing issue, but does capping the headroom at configuration
time fully prevent the 16-bit offsets from wrapping at runtime?

The commit message notes that capping the value is safe because the transmit
path will still expand the skb if more headroom is needed.

However, couldn't a user stack thousands of tunnels (e.g., in a user
namespace) and bypass recursion limits using delayed queueing disciplines like
netem?

If this happens, pskb_expand_head() would be called repeatedly. Because it
adds padded headroom that often exceeds the pushed header size, the accumulated
headroom offsets could still overflow the 16-bit fields before skb->len
reaches the 65535 IP maximum MTU limit.

When the offset wraps during the runtime update:

net/core/skbuff.c:skb_headers_offset_update() {
    ...
    if (skb_mac_header_was_set(skb))
        skb->mac_header += off;
    ...
}

Could this cause the mac_header to point into uninitialized kmalloc memory
that might then be leaked to userspace via subsystems like af_packet?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0ac01576f92412e8fa35cc3eb44336797a9d11d0.1786021595.git.zhilinz@nebusec.ai?part=1