* Re: [PATCH net v3 1/1] net: cap advertised IP tunnel headroom
2026-08-06 13:21 [PATCH net v3 1/1] net: cap advertised IP tunnel headroom Zhiling Zou
@ 2026-08-07 13:22 ` sashiko-bot
2026-08-10 12:45 ` Ido Schimmel
1 sibling, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-08-07 13:22 UTC (permalink / raw)
To: Zhiling Zou; +Cc: bpf
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 <zhilinz@nebusec.ai>
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH net v3 1/1] net: cap advertised IP tunnel headroom
2026-08-06 13:21 [PATCH net v3 1/1] net: cap advertised IP tunnel headroom Zhiling Zou
2026-08-07 13:22 ` sashiko-bot
@ 2026-08-10 12:45 ` Ido Schimmel
1 sibling, 0 replies; 3+ messages in thread
From: Ido Schimmel @ 2026-08-10 12:45 UTC (permalink / raw)
To: Zhiling Zou
Cc: bpf, netdev, daniel, razor, andrew+netdev, davem, edumazet, kuba,
pabeni, dsahern, horms, yuehaibing, kuniyu, aleksander.lobakin,
maoyixie.tju, thorsten.blum, kees, kylebot, vega
On Thu, Aug 06, 2026 at 09:21:52PM +0800, Zhiling Zou wrote:
> IP tunnel devices derive their advertised needed_headroom and, for IP6GRE
> devices with header_ops, hard_header_len from lower output devices. A stack
> of user-created devices can make the derived value larger than the 16-bit
> skb header offsets can represent. Once IP output reserves it, skb head
> expansion can wrap those offsets.
>
> The runtime transmit path already caps a growing needed_headroom at 512.
> Apply the cap when configuration publishes headroom or header lengths
> derived from a lower output device.
>
> Capping the advertised value is safe: IP tunnel transmit still expands the
> skb when the packet needs more headroom. A nonsensical stacked configuration
> can therefore incur an extra reallocation, but it cannot publish an unbounded
> reservation to upper layers.
>
> Fixes: 1a37e412a022 ("net: Use 16bits for *_headers fields of struct skbuff")
> Cc: stable@vger.kernel.org
> Reported-by: Vega <vega@nebusec.ai>
> Signed-off-by: Zhiling Zou <zhilinz@nebusec.ai>
> ---
> changes in v3:
> - Split netkit handling into a separate follow-up.
> - Explain why capping advertised headroom is safe.
> - Use local variables for derived headroom.
> - v2 Link: https://lore.kernel.org/all/0ae4aa29223b89049727aec4d36f144bad41537e.1785476387.git.zhilinz@nebusec.ai/
>
> changes in v2:
> - Move the fix from IP send paths to tunnel and netkit device control paths.
> - Cap advertised IP tunnel headroom at 512 and reject netkit headroom
> values above that limit at device creation.
> - v1 Link: https://lore.kernel.org/all/0c6c64e9bbd71a0decc8504a384061e0e631be13.1785054561.git.zhilinz@nebusec.ai/
>
> include/net/ip_tunnels.h | 11 +++++++++--
> net/ipv4/ip_tunnel.c | 2 +-
> net/ipv6/ip6_gre.c | 10 ++++++----
> net/ipv6/ip6_tunnel.c | 7 +++++--
> net/ipv6/sit.c | 2 +-
> 5 files changed, 22 insertions(+), 10 deletions(-)
>
> 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;
> +}
> +
> +static inline void ip_tunnel_adj_headroom(struct net_device *dev,
> + unsigned int headroom)
> +{
> + headroom = ip_tunnel_limit_headroom(headroom);
> +
> if (headroom > READ_ONCE(dev->needed_headroom))
> WRITE_ONCE(dev->needed_headroom, headroom);
> }
> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
> index 9d114bd575f92..5b1f180485d42 100644
> --- a/net/ipv4/ip_tunnel.c
> +++ b/net/ipv4/ip_tunnel.c
> @@ -317,7 +317,7 @@ static int ip_tunnel_bind_dev(struct net_device *dev)
> mtu = min(tdev->mtu, IP_MAX_MTU);
> }
>
> - dev->needed_headroom = t_hlen + hlen;
> + dev->needed_headroom = ip_tunnel_limit_headroom(t_hlen + hlen);
> mtu -= t_hlen + (dev->type == ARPHRD_ETHER ? dev->hard_header_len : 0);
>
> if (mtu < IPV4_MIN_MTU)
> diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
> index b843116e9b703..cc757586be90a 100644
> --- a/net/ipv6/ip6_gre.c
> +++ b/net/ipv6/ip6_gre.c
> @@ -1137,13 +1137,15 @@ static void ip6gre_tnl_link_config_route(struct ip6_tnl *t, int set_mtu,
> return;
>
> if (rt->dst.dev) {
> - unsigned short dst_len = rt->dst.dev->hard_header_len +
> - t_hlen;
> + unsigned int headroom;
> +
> + headroom = rt->dst.dev->hard_header_len + t_hlen;
> + headroom = ip_tunnel_limit_headroom(headroom);
>
> if (t->dev->header_ops)
> - dev->hard_header_len = dst_len;
> + dev->hard_header_len = headroom;
> else
> - dev->needed_headroom = dst_len;
> + dev->needed_headroom = headroom;
There is a pre-existing issue here that results in the hardware header
length being accumulated over stacked devices and it should be fixed
before capping the headroom.
Commit 832ba596494b ("net: ip6_gre: set dev->hard_header_len when using
header_ops") is correct that when "header_ops->create is used,
dev->hard_header_len should reflect the length of the header created".
For ip6gretap and ip6erspan that's the Ethernet header (14 bytes). For
ip6gre tunnels without a remote (NBMA tunnels), that's what
ip6gre_header() is pushing: GRE header + possible FOU / GUE header +
outer IPv6 header.
So the problems are:
1. hard_header_len shouldn't include the hard_header_len of the lower
device. IOW, for NBMA tunnels it should be 't_hlen', not
'rt->dst.dev->hard_header_len + t_hlen'.
2. hard_header_len must not be changed for ip6gretap and ip6erspan which
have a fixed hardware header length (Ethernet header, 14 bytes).
Following diff should fix it. Please test with both locally generated
traffic and forwarded traffic out of an NBMA tunnel. This should be the
first patch in the series and it should blame commit 832ba596494b ("net:
ip6_gre: set dev->hard_header_len when using header_ops") and carry a
stable tag:
diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index b843116e9b70..70c171091020 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -1137,13 +1137,8 @@ static void ip6gre_tnl_link_config_route(struct ip6_tnl *t, int set_mtu,
return;
if (rt->dst.dev) {
- unsigned short dst_len = rt->dst.dev->hard_header_len +
- t_hlen;
-
- if (t->dev->header_ops)
- dev->hard_header_len = dst_len;
- else
- dev->needed_headroom = dst_len;
+ dev->needed_headroom = rt->dst.dev->hard_header_len +
+ t_hlen;
if (set_mtu) {
int mtu = rt->dst.dev->mtu - t_hlen;
@@ -1171,8 +1166,8 @@ static int ip6gre_calc_hlen(struct ip6_tnl *tunnel)
t_hlen = tunnel->hlen + sizeof(struct ipv6hdr);
- if (tunnel->dev->header_ops)
- tunnel->dev->hard_header_len = LL_MAX_HEADER + t_hlen;
+ if (tunnel->dev->header_ops && tunnel->dev->type == ARPHRD_IP6GRE)
+ tunnel->dev->hard_header_len = t_hlen;
else
tunnel->dev->needed_headroom = LL_MAX_HEADER + t_hlen;
There is a pre-existing bug / assumption in the code that NBMA tunnels
don't change to point-to-point tunnels (and vice-versa), so the fix
ignores it as well.
>
> if (set_mtu) {
> int mtu = rt->dst.dev->mtu - t_hlen;
> diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
> index bf8e40af60b08..2c941acb081fa 100644
> --- a/net/ipv6/ip6_tunnel.c
> +++ b/net/ipv6/ip6_tunnel.c
> @@ -1522,8 +1522,11 @@ static void ip6_tnl_link_config(struct ip6_tnl *t)
> tdev = __dev_get_by_index(t->net, p->link);
>
> if (tdev) {
> - dev->needed_headroom = tdev->hard_header_len +
> - tdev->needed_headroom + t_hlen;
> + unsigned int headroom;
> +
> + headroom = tdev->hard_header_len + tdev->needed_headroom;
> + headroom += t_hlen;
> + dev->needed_headroom = ip_tunnel_limit_headroom(headroom);
> mtu = min_t(unsigned int, tdev->mtu, IP6_MAX_MTU);
>
> mtu = mtu - t_hlen;
> diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
> index a38b24fb83842..19b7fa8d1a2a0 100644
> --- a/net/ipv6/sit.c
> +++ b/net/ipv6/sit.c
> @@ -1131,7 +1131,7 @@ static void ipip6_tunnel_bind_dev(struct net_device *dev)
> WRITE_ONCE(dev->mtu, mtu);
> hlen = tdev->hard_header_len + tdev->needed_headroom;
> }
> - dev->needed_headroom = t_hlen + hlen;
> + dev->needed_headroom = ip_tunnel_limit_headroom(t_hlen + hlen);
> }
>
> static void ipip6_tunnel_update(struct ip_tunnel *t,
> --
> 2.43.0
^ permalink raw reply related [flat|nested] 3+ messages in thread