[PATCH net] ip_tunnel: reserve headroom for the encap header before pushing it

[PATCH net] ip_tunnel: reserve headroom for the encap header before pushing it

[Theodor Arsenij Larionov Trichkine]

ip_tunnel_xmit() and ip_md_tunnel_xmit() push the FOU/GUE header in
ip_tunnel_encap() before skb_cow_head() reserves headroom and unshares
the header. A packet redirected into the tunnel by tc mirred or an nft
netdev fwd only has the headroom of the device it came from, and
fou_build_udp() hits skb_under_panic(). With a mirred mirror, the push
overwrites the Ethernet header of the original packet instead.

Reproducer, run in an unprivileged user and network namespace:

  ip link add dummy0 type dummy
  ip addr add 10.2.2.1/24 dev dummy0
  ip link set dummy0 up
  ip link add ipip0 type ipip local 10.1.1.1 remote 10.1.1.2 \
          encap gue encap-sport auto encap-dport 5555 encap-remcsum
  ip link set ipip0 up
  tc qdisc add dev dummy0 clsact
  tc filter add dev dummy0 egress matchall \
          action mirred egress redirect dev ipip0
  # send a UDP packet to 10.2.2.2

skbuff: skb_under_panic: text:ffffffffad28b5cf len:49 put:8 head:ffff888010217740 data:ffff88801021773c tail:0x2d end:0x180 dev:ipip0
kernel BUG at net/core/skbuff.c:214!
Call Trace:
 <TASK>
 skb_push+0xcc/0xf0
 fou_build_udp+0x2f/0x380
 gue_build_header+0xfa/0x150
 ip_tunnel_xmit+0x87f/0x21c0
 ipip_tunnel_xmit+0x472/0x4f0
 dev_hard_start_xmit+0x139/0x680
 __dev_queue_xmit+0x11e4/0x3520
 sch_frag_xmit_hook+0x10a/0x190
 tcf_dev_queue_xmit+0x44/0x60
 tcf_mirred_to_dev+0xbf8/0x1060
 tcf_mirred_act+0x612/0x11b0
 tcf_action_exec+0x1d9/0x8b0
 mall_classify+0x16d/0x1e0
 __tcf_classify.constprop.0+0x121/0x710
 tcf_classify+0x8d/0xc0
 tc_run+0x342/0x590
 __dev_queue_xmit+0x9a7/0x3520
 neigh_resolve_output+0x44c/0x780
 ip_finish_output2+0x6e8/0x16f0
 __ip_finish_output.part.0+0x1bb/0x350
 ip_output+0x2ad/0x4a0
 ip_send_skb+0x195/0x1f0
 udp_send_skb+0x758/0xcc0
 udp_sendmsg+0x1634/0x2050
 </TASK>

Call skb_cow_head() for the encap length at the start of both
functions. ip_tunnel_xmit() reads tunnel->encap once, so a concurrent
changelink cannot change the length between the reservation and the
push. The GRE callers and ip6_tunnel already reserve this headroom.

Fixes: 56328486539d ("net: Changes to ip_tunnel to support foo-over-udp encapsulation")
Fixes: ac931d4cdec3 ("ipip,ip_tunnel,sit: Add FOU support for externally controlled ipip devices")
Signed-off-by: Theodor Arsenij Larionov Trichkine <theodorlarionov@gmail.com>
---
Found with a syzkaller-based fuzzer.

syzbot has an older report with the same title, fixed by c88f8d5cd95f
("sit: update dev->needed_headroom in ipip6_tunnel_bind_dev()"). This
is a different path: redirected packets do not get needed_headroom.

Tested on net af32da41b032: the reproducer above and an nft netdev
egress fwd variant no longer crash, and a GUE ipip tunnel between two
namespaces still passes traffic.

 net/ipv4/ip_tunnel.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index e6bcf01411d0..38ab3c3bf5c9 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -579,6 +579,7 @@ void ip_md_tunnel_xmit(struct sk_buff *skb, struct net_device *dev,
 	struct rtable *rt = NULL;
 	struct flowi4 fl4;
 	__be16 df = 0;
+	int encap_hlen;
 	u8 tos, ttl;
 	bool use_cache;
 
@@ -587,6 +588,11 @@ void ip_md_tunnel_xmit(struct sk_buff *skb, struct net_device *dev,
 		     ip_tunnel_info_af(tun_info) != AF_INET))
 		goto tx_error;
 	key = &tun_info->key;
+	encap_hlen = ip_encap_hlen(&tun_info->encap);
+	if (encap_hlen < 0)
+		goto tx_error;
+	if (skb_cow_head(skb, encap_hlen))
+		goto tx_dropped;
 	memset(&(IPCB(skb)->opt), 0, sizeof(IPCB(skb)->opt));
 	inner_iph = (const struct iphdr *)skb_inner_network_header(skb);
 	tos = key->tos;
@@ -671,6 +677,7 @@ void ip_tunnel_xmit(struct sk_buff *skb, struct net_device *dev,
 {
 	struct ip_tunnel *tunnel = netdev_priv(dev);
 	struct ip_tunnel_info *tun_info = NULL;
+	struct ip_tunnel_encap encap;
 	const struct iphdr *inner_iph;
 	unsigned int max_headroom;	/* The extra header space needed */
 	struct rtable *rt = NULL;		/* Route to the other host */
@@ -679,11 +686,23 @@ void ip_tunnel_xmit(struct sk_buff *skb, struct net_device *dev,
 	struct flowi4 fl4;
 	bool md = false;
 	bool connected;
+	int encap_hlen;
 	int err_count;
 	u8 tos, ttl;
 	__be32 dst;
 	__be16 df;
 
+	/* Size and push the encap header from one copy of the config. */
+	encap = READ_ONCE(tunnel->encap);
+	encap_hlen = ip_encap_hlen(&encap);
+	if (encap_hlen < 0)
+		goto tx_error;
+	if (skb_cow_head(skb, encap_hlen)) {
+		DEV_STATS_INC(dev, tx_dropped);
+		kfree_skb(skb);
+		return;
+	}
+
 	inner_iph = (const struct iphdr *)skb_inner_network_header(skb);
 	connected = (tunnel->parms.iph.daddr != 0);
 	payload_protocol = skb_protocol(skb, true);
@@ -765,7 +784,7 @@ void ip_tunnel_xmit(struct sk_buff *skb, struct net_device *dev,
 			    tunnel->net, READ_ONCE(tunnel->parms.link),
 			    tunnel->fwmark, skb_get_hash(skb), 0);
 
-	if (ip_tunnel_encap(skb, &tunnel->encap, &protocol, &fl4) < 0)
+	if (ip_tunnel_encap(skb, &encap, &protocol, &fl4) < 0)
 		goto tx_error;
 
 	if (connected && md) {
@@ -834,7 +853,7 @@ void ip_tunnel_xmit(struct sk_buff *skb, struct net_device *dev,
 	}
 
 	max_headroom = LL_RESERVED_SPACE(rt->dst.dev) + sizeof(struct iphdr)
-			+ rt->dst.header_len + ip_encap_hlen(&tunnel->encap);
+			+ rt->dst.header_len + encap_hlen;
 
 	if (skb_cow_head(skb, max_headroom)) {
 		ip_rt_put(rt);

base-commit: af32da41b0327b9c6a37856ba82b6760d6c8d10e
-- 
2.34.1

Re: [PATCH net] ip_tunnel: reserve headroom for the encap header before pushing it

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 · Medium: 0 · Low: 1 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch adds skb_cow_head(skb, encap_hlen) at the top of
  ip_tunnel_xmit() (net/ipv4/ip_tunnel.c:700), before the first…
- [High] In ip_md_tunnel_xmit(), the headroom is sized from one live read
  of the shared tun_info->encap: `encap_hlen =…
- [Low] The new early skb_cow_head(skb, encap_hlen) in ip_tunnel_xmit()
  (net/ipv4/ip_tunnel.c:700) runs on every packet, including tunnels…

Pre-existing issues:
- [Low] ip_tunnel_xmit() now does `encap = READ_ONCE(tunnel->encap);`
  (net/ipv4/ip_tunnel.c:696) under the comment 'Size and push the encap…

--

ip_tunnel: reserve headroom for the encap header before pushing it

This patch adds an early skb_cow_head() for the FOU/GUE encap header length
at the start of ip_tunnel_xmit() and ip_md_tunnel_xmit(). The goal is that
ip_tunnel_encap() no longer pushes the encap header into headroom that was
never reserved. ip_tunnel_xmit() also takes one READ_ONCE() snapshot of
tunnel->encap and uses it for both the reservation and the push.

> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
> index e6bcf01411d0b..38ab3c3bf5c9a 100644
> --- a/net/ipv4/ip_tunnel.c
> +++ b/net/ipv4/ip_tunnel.c

[ ... ]

> @@ -587,6 +588,11 @@ void ip_md_tunnel_xmit(struct sk_buff *skb, struct net_device *dev,
>  		     ip_tunnel_info_af(tun_info) != AF_INET))
>  		goto tx_error;
>  	key = &tun_info->key;
> +	encap_hlen = ip_encap_hlen(&tun_info->encap);
> +	if (encap_hlen < 0)
> +		goto tx_error;
> +	if (skb_cow_head(skb, encap_hlen))
> +		goto tx_dropped;

[Severity: High]
Is tun_info->encap stable between this reservation and the push?

The headroom is sized here from one read of tun_info->encap. The header is
then pushed later in ip_md_tunnel_xmit() using separate reads:

	if (!tunnel_hlen)
		tunnel_hlen = ip_encap_hlen(&tun_info->encap);

	if (ip_tunnel_encap(skb, &tun_info->encap, &proto, &fl4) < 0)
		goto tx_error;

The fou/gue build_header callbacks also read e->flags again.

tun_info is not necessarily private to this skb. bpf_skb_set_tunnel_key()
attaches the per-CPU md_dst to every skb it handles on that CPU, and then
rewrites it in place:

net/core/filter.c:bpf_skb_set_tunnel_key() {
	struct metadata_dst *md = this_cpu_ptr(md_dst);
    ...
	skb_dst_drop(skb);
	dst_hold((struct dst_entry *) md);
	skb_dst_set(skb, (struct dst_entry *) md);

	info = &md->u.tun_info;
	memset(info, 0, sizeof(*info));
    ...
}

bpf_skb_set_fou_encap() also writes info->encap.type/flags/sport/dport in
place.

Suppose an skb is tagged on CPU0, queued on a qdisc of a collect_md ipip
device, and sent from CPU1. Meanwhile CPU0's tc BPF program rewrites the
same md_dst (memset to TUNNEL_ENCAP_NONE, then GUE). Could CPU1 reserve 0
or 8 bytes here and then push a 12-20 byte GUE header?

That would hit the same skb_under_panic() in fou_build_udp() that this
patch fixes. In the mirred mirror case it would overwrite the bytes in
front of skb->data instead.

ip_tunnel_xmit() takes a local copy of the encap config for this reason.
Would a local snapshot here close the same gap on the md path? For example:

	struct ip_tunnel_encap encap = tun_info->encap;

used for the reservation, tunnel_hlen and ip_tunnel_encap().

>  	memset(&(IPCB(skb)->opt), 0, sizeof(IPCB(skb)->opt));
>  	inner_iph = (const struct iphdr *)skb_inner_network_header(skb);
>  	tos = key->tos;

[ ... ]

> @@ -679,11 +686,23 @@ void ip_tunnel_xmit(struct sk_buff *skb, struct net_device *dev,

[ ... ]

> +	/* Size and push the encap header from one copy of the config. */
> +	encap = READ_ONCE(tunnel->encap);

[Severity: Low]
This is a pre-existing issue, not one introduced by this patch, but the
writer side has no matching marked store for this READ_ONCE().
ip_tunnel_encap_setup() zeroes t->encap and then fills it in with plain
stores:

net/ipv4/ip_tunnel.c:ip_tunnel_encap_setup() {
    ...
	memset(&t->encap, 0, sizeof(t->encap));

	hlen = ip_encap_hlen(ipencap);
	if (hlen < 0)
		return hlen;

	t->encap.type = ipencap->type;
	t->encap.sport = ipencap->sport;
	t->encap.dport = ipencap->dport;
	t->encap.flags = ipencap->flags;
    ...
}

So a snapshot taken during changelink can still be half-written. It might
see type NONE right after the memset, or a new type with sport/dport still
0.

The length and the push now both come from the local copy. Because of
that, the worst case is a few packets sent without encap or with UDP port
0, plus a possible KCSAN report. The old plain reads could already see
these same states.

Would it make sense to build the new config in a local struct and store it
once on the writer side, as a follow-up?

> +	encap_hlen = ip_encap_hlen(&encap);
> +	if (encap_hlen < 0)
> +		goto tx_error;
> +	if (skb_cow_head(skb, encap_hlen)) {
> +		DEV_STATS_INC(dev, tx_dropped);
> +		kfree_skb(skb);
> +		return;
> +	}

[Severity: High]
Can this new skb_cow_head() free the memory that tnl_params points to?

For GRE devices with header_ops (NBMA or a multicast remote), ipgre_xmit()
passes ip_tunnel_xmit() a tnl_params pointer that lives inside the skb
head:

net/ipv4/ip_gre.c:ipgre_xmit() {
    ...
	if (dev->header_ops) {
		int pull_len = tunnel->hlen + sizeof(struct iphdr);

		if (skb_cow_head(skb, 0))
			goto free_skb;
		...
		tnl_params = (const struct iphdr *)skb->data;

		/* ip_tunnel_xmit() needs skb->data pointing to gre header. */
		skb_pull(skb, pull_len);
    ...
}

That pointer reaches ip_tunnel_xmit() through __gre_xmit():

	ip_tunnel_xmit(skb, dev, tnl_params, tnl_params->protocol);

If skb_cow_head(skb, encap_hlen) ends up in pskb_expand_head(), the old head
is freed. Every later read of tnl_params (->daddr, ->tos, ->saddr,
->frag_off, ->ttl) then reads freed memory. Those values feed the route
lookup and the outer IP header.

Before this patch, the only skb_cow_head() in ip_tunnel_xmit() ran after the
last tnl_params read.

For the reallocation to happen, the headroom at entry has to be smaller than
encap_hlen. In the GRE header_ops path the tunnel config is read three times
without locking:

  - tunnel->hlen in ipgre_xmit(), for pull_len
  - tunnel->tun_hlen in __gre_xmit()
  - tunnel->encap here

ipgre_changelink() writes t->encap and t->hlen in
ipgre_newlink_encap_setup(). It later writes tun_hlen and hlen in
ipgre_link_update(). There is no synchronize_net() in between.

For example, take a single "ip link set" that changes encap from none to GUE
with remcsum (0 -> 20 bytes) and o_flags to csum|key|seq (tun_hlen 4 -> 16).
If a packet is between the hlen read and the tun_hlen read, it has about
H0 + 8 bytes of headroom, where H0 is the headroom it arrived with. For
small H0 that is below 20.

Before the patch, that race ended in skb_under_panic(). With the patch it
looks like it becomes a silent use-after-free read.

Doesn't this contradict this part of the commit message?

    ip_tunnel_xmit() reads tunnel->encap once, so a concurrent
    changelink cannot change the length between the reservation and the
    push.

Would either of these avoid it: copying *tnl_params to a local struct before
the early skb_cow_head(), or moving the reservation after the last
tnl_params read?

[Severity: Low]
Should this skb_cow_head() be skipped when encap_hlen is 0?

__skb_cow() calls pskb_expand_head() whenever skb_header_cloned() is true,
even when delta is 0. ipip_tunnel_xmit() and sit pass skbs through without
uncloning them.

Take a header-cloned skb going into a plain ipip tunnel with no encap, such
as the mirred mirror case from the commit message. It is now uncloned here.
If its headroom is still too small, the later skb_cow_head(skb,
max_headroom) reallocates it a second time.

Before the patch, one pskb_expand_head() in the max_headroom call handled
both. Either guarding this call with if (encap_hlen), or reserving the full
estimate in one call, would avoid the extra allocation and copy per packet.

> +
>  	inner_iph = (const struct iphdr *)skb_inner_network_header(skb);
>  	connected = (tunnel->parms.iph.daddr != 0);
>  	payload_protocol = skb_protocol(skb, true);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261010223601.1193655-1-theodorlarionov%40gmail.com