netdev - Re: [PATCH ipsec v2 2/2] xfrm: Determine inner GSO type from packet inner protocol

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: <aP-QvtNf-Vp5oHLX@krikkit>
Date: Mon, 27 Oct 2025 16:33:18 +0100
From: Sabrina Dubroca <sd@...asysnail.net>
To: Jianbo Liu <jianbol@...dia.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
	steffen.klassert@...unet.com, Cosmin Ratiu <cratiu@...dia.com>,
	Herbert Xu <herbert@...dor.apana.org.au>,
	David Ahern <dsahern@...nel.org>,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
	Simon Horman <horms@...nel.org>
Subject: Re: [PATCH ipsec v2 2/2] xfrm: Determine inner GSO type from packet
 inner protocol

2025-10-27, 04:40:59 +0200, Jianbo Liu wrote:
> The GSO segmentation functions for ESP tunnel mode
> (xfrm4_tunnel_gso_segment and xfrm6_tunnel_gso_segment) were
> determining the inner packet's L2 protocol type by checking the static
> x->inner_mode.family field from the xfrm state.
> 
> This is unreliable. In tunnel mode, the state's actual inner family
> could be defined by x->inner_mode.family or by
> x->inner_mode_iaf.family. Checking only the former can lead to a
> mismatch with the actual packet being processed, causing GSO to create
> segments with the wrong L2 header type.
> 
> This patch fixes the bug by deriving the inner mode directly from the
> packet's inner protocol stored in XFRM_MODE_SKB_CB(skb)->protocol.
> 
> Fixes: 26dbd66eab80 ("esp: choose the correct inner protocol for GSO on inter address family tunnels")
> Signed-off-by: Jianbo Liu <jianbol@...dia.com>
> Reviewed-by: Cosmin Ratiu <cratiu@...dia.com>
> ---
> V2:
>  - Change subject prefix, and send to "ipsec".
>  - Add Fixes tag.
> 
>  net/ipv4/esp4_offload.c | 6 ++++--
>  net/ipv6/esp6_offload.c | 6 ++++--
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/net/ipv4/esp4_offload.c b/net/ipv4/esp4_offload.c
> index e0d94270da28..05828d4cb6cd 100644
> --- a/net/ipv4/esp4_offload.c
> +++ b/net/ipv4/esp4_offload.c
> @@ -122,8 +122,10 @@ static struct sk_buff *xfrm4_tunnel_gso_segment(struct xfrm_state *x,
>  						struct sk_buff *skb,
>  						netdev_features_t features)
>  {
> -	__be16 type = x->inner_mode.family == AF_INET6 ? htons(ETH_P_IPV6)
> -						       : htons(ETH_P_IP);
> +	const struct xfrm_mode *inner_mode = xfrm_ip2inner_mode(x,
> +					XFRM_MODE_SKB_CB(skb)->protocol);

I don't think this is correct. inner_mode_iaf is not always set by
__xfrm_init_state, only when we have a AF_UNSPEC selector, which is
not always the case for cross-family tunnels. So we would end up with
inner_mode->family = 0 here, and pass the wrong type to
skb_eth_gso_segment.

Other users of xfrm_ip2inner_mode (in ip_vti/ip6_vti, xfrmi) only call
it when we have an AF_UNSPEC selector, then we know inner_mode_iaf is
valid and can be used. Otherwise, the selector (ie x->inner_mode)
should have the right family for the packet (and all callers of
xfrm_ip2inner_mode use x->inner_mode when the selector is not
AF_UNSPEC).


Maybe it would be better to move the AF_UNSPEC check into
xfrm_ip2inner_mode, something like:

static inline const struct xfrm_mode *xfrm_ip2inner_mode(struct xfrm_state *x, int ipproto)
{
	if (x->sel.family != AF_UNSPEC)
		return &x->inner_mode;

	if ((ipproto == IPPROTO_IPIP && x->props.family == AF_INET) ||
	    (ipproto == IPPROTO_IPV6 && x->props.family == AF_INET6))
		return &x->inner_mode;
	else
		return &x->inner_mode_iaf;
}


since that's what all the callers are doing anyway?

Then it would be valid to use xfrm_ip2inner_mode like you're doing.

-- 
Sabrina