Re: [PATCH v3] net: gso: validate transport header before computing segment length

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net: gso: xfrm crypto offload tunnel TSO packets dropped in xfrm_output()
--

commit 2d1a1b977a36e13803c1238022fb971e13c956c9
Author: Michael S. Tsirkin <mst@redhat.com>

net: gso: validate transport header before computing segment length

This commit validates that the TCP transport header is set and lies within
the linear data area of the skb before reading it to compute GSO segment
lengths, returning an error so the caller treats the packet as not fitting
when the check fails.

> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index d6894f463bf43..11c8b295bd260 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -3041,6 +3041,13 @@ static inline bool skb_inner_network_header_was_set(const struct sk_buff *skb)
>  	return skb->inner_network_header > 0;
>  }
>  
> +static inline bool skb_inner_transport_header_was_set(const struct sk_buff *skb)
> +{
> +	typeof(skb->inner_transport_header) th = skb->inner_transport_header;
> +
> +	return th > 0 && th != (typeof(th))~0U;
> +}
> +
>  static inline unsigned char *skb_inner_mac_header(const struct sk_buff *skb)
>  {
>  	return skb->head + skb->inner_mac_header;
> @@ -3250,6 +3257,29 @@ static inline int skb_transport_offset(const struct sk_buff *skb)
>  	return skb_transport_header(skb) - skb->data;
>  }
>  
> +/* Is the (inner) transport header, as specified by
> + * skb->transport_header/inner_transport_header, in the linear data?
> + */
> +static inline bool skb_transport_header_linear(const struct sk_buff *skb,
> +					       unsigned int hlen)
> +{
> +	int thoff;
> +
> +	if (!skb_transport_header_was_set(skb))
> +		return false;
> +
> +	if (skb->encapsulation) {
> +		if (!skb_inner_transport_header_was_set(skb))
> +			return false;
> +
> +		thoff = skb_inner_transport_offset(skb);
> +	} else {

[Severity: High]
Does requiring skb_inner_transport_header_was_set(skb) whenever
skb->encapsulation is set cause a regression for IPsec tunnel mode TSO
packets using hardware crypto offload (XFRM_DEV_OFFLOAD_CRYPTO), causing
them to fail MTU validation and be dropped with -EMSGSIZE?

When an unencapsulated TCP GSO packet (where skb->inner_protocol is 0 and
skb->inner_transport_header is 0) enters xfrm_output() for a tunnel-mode
state with hardware crypto offload, skb->encapsulation is set before the
inner headers are recorded:

net/xfrm/xfrm_output.c:xfrm_output() {
    ...
		xfrm_get_inner_ipproto(skb, x);
		skb->encapsulation = 1;

		if (skb_is_gso(skb)) {
			if (skb->inner_protocol && x->props.mode == XFRM_MODE_TUNNEL)
				return xfrm_output_gso(net, sk, skb);

			skb_shinfo(skb)->gso_type |= SKB_GSO_ESP;
			goto out;
		}
    ...
}

From there, the call chain proceeds through:

  xfrm_output2()
    -> xfrm_output_resume()
      -> xfrm_output_one()
        -> xfrm_outer_mode_output()
          -> xfrm4_prepare_output() / xfrm6_prepare_output()

In xfrm4_prepare_output() (and similarly in xfrm6_prepare_output()),
xfrm_inner_extract_output() runs before xfrm4_tunnel_encap_add():

net/xfrm/xfrm_output.c:xfrm4_prepare_output() {
	int err;

	err = xfrm_inner_extract_output(x, skb);
	if (err)
		return err;

	IPCB(skb)->flags |= IPSKB_XFRM_TUNNEL_SIZE;
	skb->protocol = htons(ETH_P_IP);

	switch (x->props.mode) {
	case XFRM_MODE_BEET:
		return xfrm4_beet_encap_add(x, skb);
	case XFRM_MODE_TUNNEL:
		return xfrm4_tunnel_encap_add(x, skb);
	}
    ...
}

xfrm_inner_extract_output() invokes xfrm4_tunnel_check_size() (or
xfrm6_tunnel_check_size()), which calls skb_gso_validate_network_len():

net/xfrm/xfrm_output.c:xfrm4_tunnel_check_size() {
    ...
	if ((!skb_is_gso(skb) && skb->len > mtu) ||
	    (skb_is_gso(skb) &&
	     !skb_gso_validate_network_len(skb, ip_skb_dst_mtu(skb->sk, skb)))) {
		skb->protocol = htons(ETH_P_IP);

		if (skb->sk && sk_fullsock(skb->sk))
			xfrm_local_error(skb, mtu);
		else
			icmp_send(skb, ICMP_DEST_UNREACH,
				  ICMP_FRAG_NEEDED, htonl(mtu));
		ret = -EMSGSIZE;
	}
    ...
}

However, skb->inner_transport_header is only initialized afterwards in
xfrm4_tunnel_encap_add() (and xfrm6_tunnel_encap_add()):

net/xfrm/xfrm_output.c:xfrm4_tunnel_encap_add() {
    ...
	if (!skb->inner_protocol) {
		skb_set_inner_network_header(skb, skb_network_offset(skb));
		skb_set_inner_transport_header(skb, skb_transport_offset(skb));
	}
    ...
}

Because skb->encapsulation is already 1 while skb->inner_transport_header
is still 0 during xfrm4_tunnel_check_size() and xfrm6_tunnel_check_size(),
won't skb_transport_header_linear() return false, causing
skb_gso_validate_network_len() to fail and dropping the packet with bogus
fragmentation needed / packet too big errors?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/52b8bfe4ee848aff8ace47752352de486171d45f.1792596586.git.mst@redhat.com?part=1