Re: [PATCH] xfrm: esp4: use current ESN high bits for IV and AAD

[netdev-bot+sashiko@kernel.org]

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

Critical: 1 · High: 0 · 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).

- [Critical] The patch fixes only the IPv4 ESP transmit path.
- [Medium] In esp_xmit() (net/ipv4/esp4_offload.c), the patch stores the
  pre-advance high half only in the local `seq_hi`.
- [Low] The commit message never says which path is affected, and it
  leaves out part of the impact.

--

xfrm: esp4: use current ESN high bits for IV and AAD

This commit makes esp_xmit() save both halves of the current sequence
number before it advances the GSO sequence state, and builds esp.seqno
from that saved value. esp_output_set_extra() now takes the ESN high
bits for the AAD from esp->seqno, not from the offload or skb cb state.

> With AES-GCM, this assigns
> the boundary packet the same nonce as the packet sent one complete
> 32-bit sequence-number cycle later.

[Severity: Low]
This isn't a bug, but could the commit message say which configurations
are affected?

In esp_xmit(), the sequence only advances under XFRM_GSO_SEGMENT.
esp.seqno and the AAD are only used by esp_output_tail() when hw_offload
is false. That seems to limit this to GSO traffic that is segmented in
software and encrypted through the CRYPTO_FALLBACK path.

Two other details also seem to be missing:

With the old code, the boundary packet carried AAD seqhi H+1. The peer
works out H from its replay state, so it would already fail ICV
verification on that packet.

The collision with the packet one cycle later only happens if that later
packet at low 0xffffffff is not itself a software-segmented GSO segment.
Such a segment would get hi H+2.

> diff --git a/net/ipv4/esp4_offload.c b/net/ipv4/esp4_offload.c
> index abd77162f5e75..79f7d08325c50 100644
> --- a/net/ipv4/esp4_offload.c
> +++ b/net/ipv4/esp4_offload.c

[ ... ]

> @@ -334,7 +336,7 @@ static int esp_xmit(struct xfrm_state *x, struct sk_buff *skb,  netdev_features_
>  	if (xo->seq.low < seq)
>  		xo->seq.hi++;

[Severity: Medium]
The saved seq_hi only goes into esp.seqno, and only esp_output_tail()
reads that, on the !hw_offload path. On the hw_offload path, xo->seq.hi
is still incremented here for the segment whose on-wire low seq is
0xffffffff. skb_ext_add() keeps that value and it goes to the driver
with XFRM_XMIT.

Can the advanced high half still reach the device AAD for ESN SAs?

ch_ipsec sets NETIF_F_HW_ESP but not NETIF_F_GSO_ESP. So TCP GSO over an
offloaded SA is split in software by esp4_gso_segment(), which sets
XFRM_GSO_SEGMENT. xmit_xfrm_check_overflow() in validate_xmit_xfrm()
only runs for skb_is_gso() skbs, so it does not run for these segments.

ch_ipsec supports ESN, and copy_esn_pktxt() builds the AAD like this:

drivers/net/ethernet/chelsio/inline_crypto/ch_ipsec/chcr_ipsec.c:copy_esn_pktxt() {
    ...
	seqlo = ntohl(esphdr->seq_no);
	seqno = cpu_to_be64(seqlo + ((u64)xo->seq.hi << 32));
	memcpy(aadiv->seq_no, &seqno, 8);
    ...
}

For the boundary segment, the device would authenticate over
(H+1, 0xffffffff). The peer expects H and would fail ICV verification.
The AAD would also match the packet one cycle later.

nfp_net_ipsec_tx_prep() also copies xo->seq.hi, but NFP rejects ESN
states at add time, so only ch_ipsec seems to be affected. I could not
confirm whether this also leads to nonce reuse, because ch_ipsec takes
the IV from the packet's IV field.

>  
> -	esp.seqno = cpu_to_be64(seq + ((u64)xo->seq.hi << 32));
> +	esp.seqno = cpu_to_be64(seq + ((u64)seq_hi << 32));

[Severity: Critical]
Commit 4b549ccce941 (the Fixes target) added the same wrap handling to
both esp_xmit() and esp6_xmit(). Is the IPv6 side still affected after
this patch? The commit message does not mention IPv6.

net/ipv6/esp6_offload.c:esp6_xmit() {
    ...
	seq = xo->seq.low;
    ...
	if (xo->flags & XFRM_GSO_SEGMENT) {
		esp.esph->seq_no = htonl(seq);

		if (!skb_is_gso(skb))
			xo->seq.low++;
		else
			xo->seq.low += skb_shinfo(skb)->gso_segs;
	}

	if (xo->seq.low < seq)
		xo->seq.hi++;

	esp.seqno = cpu_to_be64(xo->seq.low + ((u64)xo->seq.hi << 32));
    ...
}

esp_output_set_esn() in net/ipv6/esp6.c also still takes the AAD high
half from the advanced offload state:

	if (xo)
		seqhi = xo->seq.hi;
	else
		seqhi = XFRM_SKB_CB(skb)->seq.output.hi;

For the boundary segment with on-wire seq (H, 0xffffffff), that gives
IV (H+1, 0) and AAD seqhi H+1. This is the same problem this patch fixes
for IPv4.

esp6_xmit() also builds the IV from the advanced low half. A
software-segmented packet with header seq S is therefore encrypted with
IV S+1, even when there is no wrap.

xfrm_replay_overflow_offload_esn() reserves exactly gso_segs numbers
(S..S+n-1) for a GSO skb, so the next packet gets S+n. Suppose that
packet is not GSO: it goes through esp6_xmit() without
XFRM_GSO_SEGMENT, or through esp6_output(). Its IV is then S+n, the
same IV the last segment of the previous batch used.

esp6_output_tail() copies esp->seqno straight into the AEAD IV.

Could this give AES-GCM nonce reuse within a single sequence cycle?
xfrm_dev_offload_ok() returns true when x->xso.dev is NULL. Non-offloaded
SAs with esp6_offload loaded would then go through:

xfrm_output()
  xfrm_replay_overflow_offload_esn()
validate_xmit_xfrm()
  esp6_gso_segment()
    esp6_xmit()   /* CRYPTO_FALLBACK */
      esp6_output_tail()

Should esp6_xmit() also save both halves before the advance and build
esp.seqno from them? Should esp_output_set_esn() then take seqhi from
esp->seqno?

>  
>  	if (hw_offload && encap_type == UDP_ENCAP_ESPINUDP) {

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925095128.446450-2-Jeremy.Jean%40oss.cyber.gouv.fr