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

[Paolo Abeni]

On 10/11/26 00:36, Theodor Arsenij Larionov Trichkine wrote:
> 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@xxxxxxxxx>
> ---
> 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;

This looks like the wrong solution?!? I suggest instead adding the needed
skb_cow_head() in {fou,gue}_build_header().

/P