Re: [PATCH net] esp: fix out-of-bounds transport-header read after exthdr walk

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 1 · Low: 1

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] Both new bounds checks compare a negative, data-relative offset
  against skb->len using unsigned arithmetic, so the check is always…
- [Medium] Where the computed offset is non-negative, the new checks
  bound it only by skb->len (total length including page frags and…
- [Low] The new comments and the commit message do not describe the main
  receive paths accurately.

--

esp: fix out-of-bounds transport-header read after exthdr walk

This adds a bounds check to esp_input_done2() and to esp6_input_done2()
before the 2-byte source port is read from the encap transport header.
If the computed offset plus two bytes is past skb->len, the packet is
rejected with -EINVAL.

> diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
> index e76db5817e78e..aaff5a2b659db 100644
> --- a/net/ipv4/esp4.c
> +++ b/net/ipv4/esp4.c
> @@ -744,6 +744,18 @@ int esp_input_done2(struct sk_buff *skb, int err)
>  		struct udphdr *uh = (void *)(skb_network_header(skb) + ihl);
>  		__be16 source;
>  
> +		/*
> +		 * ihl is taken from the decrypted inner packet and is not
> +		 * re-validated after esp_remove_trailer() shrank the skb;
> +		 * make sure the 2-byte source port we read below is within
> +		 * the packet.
> +		 */

[Severity: Low]
Is this comment accurate for the UDP encap path?

In esp_input_done2(), ihl comes from:

	iph = ip_hdr(skb);
	ihl = iph->ihl * 4;

At this point that is the outer, unencrypted IPv4 header that
ip_rcv_core() validated. The inner network header is only reset later,
by the mode input callback.

The UDP header at network_header + ihl was already linearized by
__xfrm4_udp_encap_rcv():

	if (!pskb_may_pull(skb, sizeof(struct udphdr) + min(len, 8)))

That header sits before skb->data, so esp_remove_trailer() trimming the
tail can't move it out of range.

The esp6 comment has the same problem. On the UDP path,
ipv6_skip_exthdr() walks outer extension headers that IPv6 input has
already validated, stops at the UDP header, and returns a negative
offset relative to skb->data.

The scenario might really happen on espintcp paths where the network
header is stale. tcp_collapse() leaves network_header at 0 for a new
head, and alloc_skb_for_msg() copies first->network_header into a
0-byte head through __copy_skb_header(). That would fit the Fixes: tag,
but neither the comments nor the commit message describe it.

Could the comments and the commit message describe how the offsets
actually work, and which path produces the bad header?

> +		if (skb_network_offset(skb) + ihl + sizeof(__be16) >
> +		    skb->len) {
> +			err = -EINVAL;
> +			goto out;
> +		}

[Severity: High]
Can this check ever pass for a valid encapsulated packet?

When esp_input_done2() runs, skb->data points at the ESP header. The
outer IP header and the UDP or TCP encap header both sit before
skb->data. For UDP, __xfrm4_udp_encap_rcv() pulls the UDP header off:

net/ipv4/xfrm4_input.c:__xfrm4_udp_encap_rcv() {
    ...
	if (pull) {
		__skb_pull(skb, len);
		skb_reset_transport_header(skb);
    ...
}

For espintcp, espintcp_rcv() does:

	if (!pskb_pull(skb, rxm->offset + 2)) {

So with UDP encap, skb_network_offset(skb) is -(ihl + 8), and
skb_network_offset(skb) + ihl is -8. Adding sizeof(__be16) converts
that to size_t, so the left side becomes a huge unsigned value and the
comparison is always true.

Wouldn't that drop every UDP_ENCAP_ESPINUDP and TCP_ENCAP_ESPINTCP
packet with -EINVAL after a successful decrypt? That would break both
NAT-T and espintcp receive.

The new check in esp6_input_done2() looks like it has the same problem:

	if (offset + sizeof(__be16) > skb->len) {

offset starts at skb_network_offset(skb) + sizeof(*ip6h).
ipv6_skip_exthdr() stops at the UDP or TCP header, which gives -8 for
UDP. The existing code already depends on offset being negative there:

	hdr_len += offset;

Also, skb->len counts bytes from skb->data onward, but the port being
read is before skb->data. Even a signed comparison against skb->len
would not bound this read.

Would something like skb_header_pointer_careful(), or a signed check
against the headroom and headlen, be needed instead of extending this
check?

> +
>  		switch (x->encap->encap_type) {
>  		case TCP_ENCAP_ESPINTCP:
>  			source = th->source;
> diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
> index b1c9b36f76dc4..89e8cdf5a26b2 100644
> --- a/net/ipv6/esp6.c
> +++ b/net/ipv6/esp6.c
> @@ -782,6 +782,17 @@ int esp6_input_done2(struct sk_buff *skb, int err)
>  			goto out;
>  		}
>  
> +		/*
> +		 * ipv6_skip_exthdr() can advance past the packet end when the
> +		 * last extension header claims an oversized length; the
> +		 * transport header below would then be read out of bounds.
> +		 * Make sure there is room for the 2-byte source port we read.
> +		 */
> +		if (offset + sizeof(__be16) > skb->len) {
> +			err = -EINVAL;
> +			goto out;
> +		}
> +
>  		uh = (void *)(skb->data + offset);
>  		th = (void *)(skb->data + offset);
>  		hdr_len += offset;

[Severity: Medium]
When offset is non-negative, does this check bound the read that
follows?

skb->len includes page frags and the frag_list. The port, however, is
read straight from the linear buffer through skb->data + offset. Any
offset with skb_headlen(skb) <= offset + 2 <= skb->len passes the
check, and the read then lands past the linear data, possibly past the
end of the head allocation.

Non-negative offsets seem possible on the espintcp paths where the
network header is stale. alloc_skb_for_msg() does:

	n = alloc_skb(0, GFP_ATOMIC);
	...
	skb_shinfo(n)->frag_list = first;

	__copy_skb_header(n, first);

and tcp_collapse() leaves network_header at 0. In both cases
ipv6_hdr() points at bytes that are not a real IPv6 header.
ipv6_skip_exthdr() walks with skb_header_pointer(), which can read
frags, and each step can advance up to 2048 bytes. esp6_input() does
not linearize the skb, so the offset can end up well inside skb->len
but past skb_headlen().

The esp4 check has the same skb->len bound in front of the direct
th->source and uh->source reads at skb_network_header(skb) + ihl.

I haven't confirmed the whole path end to end. Would reading the port
with skb_header_pointer_careful() handle both the negative-offset case
and the frag case?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009135233.4084239-1-bsdhenrymartin%40gmail.com