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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 2 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch adds a transport-header check only to the GSO length
  validators in net/core/gso.c.
- [High] net: gso: xfrm crypto offload tunnel TSO packets dropped in
  xfrm_output()

--

net: gso: validate transport header before computing segment length

This patch adds a check to the GSO segment length helpers in
net/core/gso.c. For TCP GSO packets, the (inner) transport header must
now be set, and the TCP header must sit in the linear data, before
tcp_hdrlen() is read. If the check fails, skb_gso_validate_network_len()
and skb_gso_validate_mac_len() report the packet as not fitting, and a
new helper, skb_transport_header_linear(), is added to skbuff.h for the
check.

> 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

[ ... ]

> @@ -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 {
> +		thoff = skb_transport_offset(skb);
> +	}
> +
> +	return thoff >= 0 && thoff + hlen <= skb_headlen(skb);
> +}

[Severity: High]
Does this break TSO over IPsec tunnel mode with crypto offload?

In xfrm_output(), when xfrm_dev_offload_ok() succeeds for an
XFRM_DEV_OFFLOAD_CRYPTO state, encapsulation is set on the skb before
any inner header offsets have been recorded:

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

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

The xfrm_output_gso() branch elided above is only taken when
skb->inner_protocol is already set. A plain TCP skb, whether locally
generated or forwarded, still has inner_protocol and
inner_transport_header at 0 here, so it goes to xfrm_output2(). The
inner offsets are only filled in by the tunnel encap helpers, and that
happens after the size check has run:

xfrm_output2()
  xfrm_output_resume()
    xfrm_output_one()
      xfrm_outer_mode_output()
        xfrm4_prepare_output()
          xfrm_inner_extract_output()
            xfrm4_tunnel_check_size()
              skb_gso_validate_network_len()
          xfrm4_tunnel_encap_add()
            skb_set_inner_transport_header()

The IPv6 path through xfrm6_prepare_output() has the same ordering.

So skb_gso_validate_network_len() is called with encapsulation set
and inner_transport_header == 0. With this patch, the following
happens:

- skb_inner_transport_header_was_set() returns false.
- skb_gso_transport_seglen() returns -EINVAL.
- The packet is reported as not fitting.

xfrm4_tunnel_check_size() (when DF is set, as it normally is for TCP)
and xfrm6_tunnel_check_size() then report the MTU to the local socket
or send an ICMP too big error, and return -EMSGSIZE. xfrm_output_one()
then frees the skb. As a result, every TSO packet over such an SA is
dropped.

The commit message expects the caller to fall back to software
segmentation, but here a false return is treated as a hard MTU
failure. The MTU reported back is the one the flow is already using,
so it is not clear what would make the sender stop building these
packets.

Before this patch, the same call read inner_tcp_hdrlen() from
skb->head and computed thlen against a zero inner offset. The result
was meaningless, but in practice it let these packets through.

Should xfrm_output() record the inner network and transport offsets
at the point where it sets encapsulation for the crypto offload case?
Or is there another way you would prefer to handle this, before the
new check starts requiring a valid inner transport header whenever
encapsulation is set?

[Severity: High]

The commit message says:

  Qdisc length estimation has carried a guard against exactly this since

  commit a0dce8752193 ("net: Skip GSO length estimation if transport
  header is not set");

Does that hold for the bpf_skb_adjust_room() case named later in the
commit message?

The guard in qdisc_pkt_len_segs_init() in net/core/dev.c only rejects an
unset transport header on a non-encapsulated skb:

net/core/dev.c:qdisc_pkt_len_segs_init() {
    ...
	if (!skb->encapsulation) {
		if (unlikely(!skb_transport_header_was_set(skb)))
			return SKB_NOT_DROPPED_YET;
		hdr_len = skb_transport_offset(skb);
	} else {
		hdr_len = skb_inner_transport_offset(skb);
	}
	/* + transport layer */
	if (likely(shinfo->gso_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6))) {
		const struct tcphdr *th;

		if (!pskb_may_pull(skb, hdr_len + sizeof(struct tcphdr)))
			return SKB_DROP_REASON_SKB_BAD_GSO;

		th = (const struct tcphdr *)(skb->data + hdr_len);
		tlen = __tcp_hdrlen(th);
    ...
}

It does not reject a transport offset that is set but negative.
bpf_skb_net_hdr_pop() in net/core/filter.c can leave one behind. After
bpf_skb_generic_pop() moves skb->data forward, only mac_header and
network_header are moved too:

net/core/filter.c:bpf_skb_net_hdr_pop() {
    ...
	ret = bpf_skb_generic_pop(skb, off, len);
	if (likely(!ret)) {
		skb->mac_header += len;
		skb->network_header += len;
		if (trans_same)
			skb->transport_header = skb->network_header;
	}
    ...
}

Two cases leave transport_header in front of the new skb->data:

- MAC-mode decap of a locally encapsulated VXLAN packet with
  BPF_F_ADJ_ROOM_DECAP_L4_UDP. transport_header still points at the
  outer UDP header, and encapsulation is cleared.
- A NET-mode shrink with len > off.

If the tc program then calls bpf_redirect() to egress, the skb reaches
the estimator before any of the newly guarded validators run:

__bpf_redirect_common()
  __bpf_tx_skb()
    dev_queue_xmit()
      __dev_queue_xmit()
        qdisc_pkt_len_segs_init()

In qdisc_pkt_len_segs_init(), an offset of -16 does the following:

- It is stored in the unsigned hdr_len as 0xFFFFFFF0.
- hdr_len + sizeof(struct tcphdr) is truncated to 4 when passed to
  pskb_may_pull(), so the check passes.
- th ends up about 4GiB past skb->data on 64-bit, and __tcp_hdrlen(th)
  then reads th->doff from it.

Could this oops, or read unrelated memory and compute a garbage length?

The encapsulated branch also calls skb_inner_transport_offset() without
any of the rules this patch adds: inner header not 0 or ~0U, outer
header set, offset >= 0, and header inside the linear data. As a
result, the qdisc estimator and the GSO validators now apply different
rules to the same TSO skb.

Should qdisc_pkt_len_segs_init() also use skb_transport_header_linear(),
or should the commit message describe this as a gap that remains?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/52b8bfe4ee848aff8ace47752352de486171d45f.1792596586.git.mst%40redhat.com