netdev - Re: [PATCH net] xfrm: Force software GSO only in tunnel mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <305606b805fa2bb4725bcbd8c5ee88b88dfff7c5.camel@nvidia.com>
Date: Thu, 20 Mar 2025 15:24:46 +0000
From: Cosmin Ratiu <cratiu@...dia.com>
To: Mark Bloch <mbloch@...dia.com>, "lucien.xin@...il.com"
	<lucien.xin@...il.com>
CC: "chopps@...n.net" <chopps@...n.net>, "davem@...emloft.net"
	<davem@...emloft.net>, "herbert@...dor.apana.org.au"
	<herbert@...dor.apana.org.au>, Dragos Tatulea <dtatulea@...dia.com>,
	"steffen.klassert@...unet.com" <steffen.klassert@...unet.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>, "pabeni@...hat.com"
	<pabeni@...hat.com>, "horms@...nel.org" <horms@...nel.org>,
	"edumazet@...gle.com" <edumazet@...gle.com>, "kuba@...nel.org"
	<kuba@...nel.org>, Yael Chemla <ychemla@...dia.com>, "wangfe@...gle.com"
	<wangfe@...gle.com>
Subject: Re: [PATCH net] xfrm: Force software GSO only in tunnel mode

On Mon, 2025-03-17 at 15:30 -0400, Xin Long wrote:
> For UDP tunnels, there are two types:
> 
> - ENCAP_TYPE_ETHER encaps an ether packet (e.g., VXLAN, Geneve).
> - ENCAP_TYPE_IPPROTO encaps an ipproto packet (e.g., SCTP over UDP).
> 
> When performing GSO via skb_udp_tunnel_segment():
> 
> - ENCAP_TYPE_ETHER relies on inner_network_header to locate the
>   network header.
> - ENCAP_TYPE_IPPROTO relies on inner_transport_header to locate
>   the transport header.
> 
> However, both IPsec transport and tunnel modes modify
> inner_transport_header. This patch raises a concern that GSO may
> not work correctly for ENCAP_TYPE_IPPROTO UDP tunnels over IPsec
> in transport mode.

skb_udp_tunnel_segment -> __skb_udp_tunnel_segment does:

tnl_hlen = skb_inner_mac_header(skb) - skb_transport_header(skb);
__skb_pull(skb, tnl_hlen);
skb_reset_mac_header(skb);
skb_set_network_header(skb, skb_inner_network_offset(skb));
skb_set_transport_header(skb, skb_inner_transport_offset(skb));

As a concrete example, in case of TCP over Geneve over IPsec in
transport mode, this is the sequence of header manipulations done:
geneve_build_skb: mh 190 nh 204 th 224 imh 190 inh 204 ith 224 data 182
iptunnel_xmit: mh 190 nh 154 th 174 imh 190 inh 204 ith 224 data 154
xfrm4_transport_output: mh 147 nh 138 th 158 imh 190 inh 204 ith 174
data 174
skb_mac_gso_segment: mh 124 nh 138 th 158 imh 190 inh 204 ith 174 data
124
inet_gso_segment: mh 124 nh 138 th 158 imh 190 inh 204 ith 174 data 138
esp4_gso_segment: mh 124 nh 138 th 158 imh 190 inh 204 ith 174 data 158
__skb_udp_tunnel_segment: mh 124 nh 138 th 174 imh 190 inh 204 ith 174
data 174
__skb_udp_tunnel_segment: tnl_hlen 16
__skb_udp_tunnel_segment: inner skb mh 190 nh 204 th 174 imh 190 inh
204 ith 174 data 190
skb_mac_gso_segment: mh 190 nh 204 th 174 imh 190 inh 204 ith 174 data
190
inet_gso_segment: mh 190 nh 204 th 174 imh 190 inh 204 ith 174 data 204
tcp_gso_segment: mh 190 nh 204 th 224 imh 190 inh 204 ith 174 data 224

All numbers are offsets from skb->head printed at function start
(except for '__skb_udp_tunnel_segment: inner', printed after the code
block mentioned above).
I see that xfrm4_transport_output moves the inner transport header
forward (to 174) and that __skb_udp_tunnel_segment incorrectly sets
transport header to it, but fortunately inet_gso_segment resets it to
the correct value after pulling the ip header.

In case of ENCAP_TYPE_IPPROTO, inet_gso_segment/ipv6_gso_segment would
be invoked directly by __skb_udp_tunnel_segment and it would see the
network header set correctly. But both compute nhoff like this:
nhoff = skb_network_header(skb) - skb_mac_header(skb);
Which would be 0 given mac_header is set to the same offset as the ip
header.
But that only makes the pskb_may_pull check & the skb_pull not do
anything. The functions then proceed to set up the transport header
correctly
I think the code might still work but I haven't verified with a
ENCAP_TYPE_IPPROTO protocol.

In general, while staring at this code, I got the impression that these
functions are brittle, relying on assumptions made in completely
different areas that might easily be broken given a different
combination of protocols.

I think that the code block in __skb_udp_tunnel_segment could be made
less brittle if it stops relying on the saved inner headers (which
might not be set to the actual inner protocols about to be handled),
and instead parse the mac or network headers and set the next headers
accordingly. This might even allow back HW GSO for XFRM in tunnel mode
with crypto offload, like before your original 2020 patch. WDYT?

Cosmin.