Re: [PATCH ipsec v2 6/7] xfrm: segment untrusted GSO packets before sequence allocation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] xfrm: skb->encapsulation set in xfrm_output_one() before
  inner headers
- [Medium] xfrm: early IPv6 UFO segmentation corrupts xo->proto in
  esp6_gso_encap()
--

commit 0279671c95c12bb5bd51f663fc7acf801cb6d765
Author: Jérémy Jean <Jeremy.Jean@oss.cyber.gouv.fr>

xfrm: segment untrusted GSO packets before sequence allocation

This commit segments SKB_GSO_DODGY packets in xfrm_output() before
allocating sequence numbers and moves setting skb->encapsulation into
xfrm_output_one().

> diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
> index 99d1b33db52a6..a1fb60971c6bc 100644
> --- a/net/xfrm/xfrm_output.c
> +++ b/net/xfrm/xfrm_output.c
> @@ -505,6 +505,9 @@ static int xfrm_output_one(struct sk_buff *skb, int err)
>  	if (err <= 0 || x->xso.type == XFRM_DEV_OFFLOAD_PACKET)
>  		goto resume;
>  
> +	if (xfrm_offload(skb))
> +		skb->encapsulation = 1;
> +
>  	do {
>  		err = xfrm_skb_check_space(skb);
>  		if (err) {

[Severity: Medium]
Does setting skb->encapsulation = 1 before xfrm_outer_mode_output() cause
xfrm4_tunnel_check_size() and xfrm6_tunnel_check_size() to read inner
headers before skb->inner_network_header and skb->inner_transport_header
are initialized?

In tunnel and BEET modes, xfrm_outer_mode_output() runs
xfrm_inner_extract_output() before xfrm4_tunnel_encap_add() or
xfrm6_tunnel_encap_add() sets the inner header offsets.  When
skb->encapsulation is already set on a packet whose inner header offsets
are still zero:

net/core/gso.c:skb_gso_transport_seglen() {
	...
	if (skb->encapsulation) {
		thlen = skb_inner_transport_header(skb) -
			skb_transport_header(skb);

		if (likely(shinfo->gso_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6)))
			thlen += inner_tcp_hdrlen(skb);
	}
	...
}

For non-dodgy GSO packets with ESP offload enabled,
skb_gso_validate_network_len() reads inner_tcp_hdrlen(skb) from skb->head
and subtracts skb->transport_header, underflowing the unsigned segment
length when gso_size is small and dropping valid packets with -EMSGSIZE.
If the size check fails on a socket-owned packet, xfrm4_local_error() and
xfrm6_local_error() also take the skb->encapsulation branch and read
hdr->daddr from skb->head into the socket error queue.

Setting skb->encapsulation = 1 inside the if (xfrm_offload(skb)) branch
right before x->type_offload->encap(x, skb), matching where the else
branch clears it, avoids setting it before xfrm_outer_mode_output() has
initialized the inner headers.

> @@ -814,10 +817,10 @@ int xfrm_output(struct sock *sk, struct sk_buff *skb)
>  		xfrm_state_hold(x);
>  
>  		xfrm_get_inner_ipproto(skb, x);
> -		skb->encapsulation = 1;
>  
>  		if (skb_is_gso(skb)) {
> -			if (skb->inner_protocol && x->props.mode == XFRM_MODE_TUNNEL)
> +			if ((skb_shinfo(skb)->gso_type & SKB_GSO_DODGY) ||
> +			    (skb->inner_protocol && x->props.mode == XFRM_MODE_TUNNEL))
>  				return xfrm_output_gso(net, sk, skb);
>  
>  			skb_shinfo(skb)->gso_type |= SKB_GSO_ESP;

[Severity: Medium]
Can segmenting an IPv6 UFO packet (SKB_GSO_UDP | SKB_GSO_DODGY) here
after xfrm_dev_offload_ok() has already attached an offload sec_path
corrupt xo->proto in esp6_gso_encap()?

When xfrm_dev_offload_ok() runs on the unsegmented UFO packet,
ipv6_hdr(skb)->nexthdr is IPPROTO_UDP, so offload is enabled.
xfrm_output_gso() then calls udp6_ufo_fragment(), which inserts a
struct frag_hdr and sets ipv6_hdr(seg)->nexthdr = NEXTHDR_FRAGMENT on
each segment.  In transport mode, xfrm6_transport_output() stops at
NEXTHDR_FRAGMENT and moves only the 40-byte struct ipv6hdr backwards to
make room for the ESP header, leaving the old iph->daddr bytes in the
uninitialized ESP header slot at offset 40:

net/ipv6/esp6_offload.c:esp6_gso_encap() {
	...
	proto = iph->nexthdr;

	if (x->outer_mode.encap == XFRM_MODE_TRANSPORT) {
		__be16 frag;

		ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr), &proto, &frag);
	}
	...
}

Because proto is NEXTHDR_FRAGMENT, ipv6_skip_exthdr() parses offset 40 as
a fragment header before the ESP header is written and overwrites
xo->proto with leftover destination address bytes whenever those bytes
have a zero fragment offset.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108142652.1330952-1-Jeremy.Jean@oss.cyber.gouv.fr?part=6