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

[Jakub Kicinski <kuba@kernel.org>]

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.

> 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?