Re: [PATCH 12/12] net: xfrm: reject unrepresentable espintcp transport headers

Ccing Author of the patch.

On Tue, Sep 08, 2026 at 03:49:02PM -0700, Jakub Kicinski wrote:

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: xfrm: reject unrepresentable espintcp transport headers

handle_esp() in net/xfrm/espintcp.c switches from the plain transport
header reset to skb_reset_transport_header_careful(), and drops the skb
with LINUX_MIB_XFRMINERROR when the offset does not fit in
skb->transport_header.

quoted

diff --git a/net/xfrm/espintcp.c b/net/xfrm/espintcp.c
index 674aedc5af5a2..3e72b9f067b9b 100644
--- a/net/xfrm/espintcp.c
+++ b/net/xfrm/espintcp.c

@@ -30,7 +30,11 @@ static void handle_esp(struct sk_buff *skb, struct sock *sk)
 {
 	struct tcp_skb_cb *tcp_cb = (struct tcp_skb_cb *)skb->cb;
 
-	skb_reset_transport_header(skb);
+	if (!skb_reset_transport_header_careful(skb)) {
+		XFRM_INC_STATS(sock_net(sk), LINUX_MIB_XFRMINERROR);
+		kfree_skb(skb);
+		return;
+	}

[Severity: High]
Is this gate placed early enough to cover the pulls that the ESP input
path performs on the same skb afterwards?

skb_reset_transport_header_careful() in include/linux/skbuff.h only
rejects offsets that are already unrepresentable:

	long offset = skb->data - skb->head;

	if (unlikely(offset != (typeof(skb->transport_header))offset))
		return false;

	if (unlikely(offset == (typeof(skb->transport_header))~0U))
		return false;

so any offset up to 65534 is accepted here with no margin left.

handle_esp() then calls xfrm4_rcv_encap()/xfrm6_rcv_encap(), and the
chain xfrm_input() -> esp_input() -> esp_input_done2() pulls the ESP
header and IV off the very same skb and resets the transport header with
the unhardened helpers, in net/ipv4/esp4.c:

	skb_pull_rcsum(skb, hlen);
	if (x->props.mode == XFRM_MODE_TUNNEL ||
	    x->props.mode == XFRM_MODE_IPTFS)
		skb_reset_transport_header(skb);
	else
		skb_set_transport_header(skb, -ihl);

with hlen = sizeof(struct ip_esp_hdr) + crypto_aead_ivsize(aead).
esp6_input_done2() in net/ipv6/esp6.c has the same shape for the
AF_INET6 branch of handle_esp().

For an skb whose head offset lands in the window [65536 - hlen, 65534],
the new check passes but the offset after skb_pull_rcsum() no longer
fits, so doesn't the truncation the changelog wants to eliminate simply
move downstream, now on a partially decrypted packet instead of a clean
drop?

Can such near-limit offsets be reached in practice?  espintcp_rcv() does:

	/* remove header, leave non-ESP marker/SPI */
	if (!pskb_pull(skb, rxm->offset + 2)) {

which makes skb->data - skb->head equal to the initial payload offset
plus rxm->offset + 2, and rxm->offset is bounded only by the size of one
coalesced TCP receive-queue skb.  If offsets at or above 65535 are
reachable (which is the premise of this patch), offsets a few bytes below
65536 look equally reachable, and the peer selects them at byte
granularity through the 16-bit espintcp length prefix.

For ESP transport mode there also seems to be a second-order effect.  With
k = (offset + hlen) - 65536 and k >= ihl, esp_input_done2() leaves
transport_header at (k - ihl), i.e. wrapped, and
xfrm4_transport_input()/xfrm6_transport_input() in net/xfrm/xfrm_input.c
consume it directly:

	int ihl = skb->data - skb_transport_header(skb);

	if (skb->transport_header != skb->network_header) {
		memmove(skb_transport_header(skb),
			skb_network_header(skb), ihl);
		...
	skb_reset_transport_header(skb);

Can ihl here become roughly 64 KiB and turn that memmove() into a copy
that runs past the packet data into the trailing skb_shared_info?  With
AES-CBC (ivsize 16, hlen 24) k can be 20..23, which is >= a 20-byte ihl.

Would it make sense either to give the espintcp check a margin covering
the largest downstream ESP header plus IV pull (or avoid the large linear
pull in espintcp_rcv() altogether), or to convert the esp4/esp6 and
xfrm_input transport-mode reset sites to the careful helpers as well?

Conversely, if offsets in that window cannot occur, what makes the check
added here reachable at all?

The patch did not add a regression, but the question if that
can be triggered at all is valid.

Wyatt can you explain how you tiggered this bug?