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

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

[Jérémy Jean]

esp_xmit() saves the low half of the current packet sequence number
before advancing the GSO sequence state, but builds esp.seqno with the
high half after the advance. When the low half wraps, the packet whose
transmitted sequence number is 0xffffffff is encrypted as though it
belonged to the next sequence-number cycle. With AES-GCM, this assigns
the boundary packet the same nonce as the packet sent one complete
32-bit sequence-number cycle later. esp_output_set_extra() also reads
the advanced high half when constructing the associated data, so the
two packets use identical associated data.

Because GCM uses CTR mode for encryption, known plaintext from either
record reveals the corresponding plaintext in the other. More
importantly, reusing the nonce makes the GHASH authentication key
recoverable, allowing an attacker to forge valid tags for arbitrary
ciphertexts under that nonce and key.

Starting from sequence number zero, reaching the faulty packet requires
2^32 - 1 outbound ESP packet sequence increments. Reusing its nonce
requires another 2^32 increments under the same AES-GCM key, for
2^33 - 1 increments in total.

Snapshot both halves of the current sequence before changing the GSO
state. Derive the authenticated high half from that same immutable
sequence value so the IV and associated data cannot diverge.

Fixes: 4b549ccce941 ("xfrm: replay: Fix ESN wrap around for GSO")
Assisted-by: LLM
Signed-off-by: Jérémy Jean <Jeremy.Jean@oss.cyber.gouv.fr>
---
 net/ipv4/esp4.c         | 13 ++++---------
 net/ipv4/esp4_offload.c |  6 ++++--
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
index e76db5817e78..04f27c41ea50 100644
--- a/net/ipv4/esp4.c
+++ b/net/ipv4/esp4.c
@@ -271,20 +271,15 @@ static void esp_output_restore_header(struct sk_buff *skb)
 static struct ip_esp_hdr *esp_output_set_extra(struct sk_buff *skb,
 					       struct xfrm_state *x,
 					       struct ip_esp_hdr *esph,
-					       struct esp_output_extra *extra)
+					       struct esp_output_extra *extra,
+					       __be64 seqno)
 {
 	/* For ESN we move the header forward by 4 bytes to
 	 * accommodate the high bits.  We will move it back after
 	 * encryption.
 	 */
 	if ((x->props.flags & XFRM_STATE_ESN)) {
-		__u32 seqhi;
-		struct xfrm_offload *xo = xfrm_offload(skb);
-
-		if (xo)
-			seqhi = xo->seq.hi;
-		else
-			seqhi = XFRM_SKB_CB(skb)->seq.output.hi;
+		__u32 seqhi = upper_32_bits(be64_to_cpu(seqno));
 
 		extra->esphoff = (unsigned char *)esph -
 				 skb_transport_header(skb);
@@ -543,7 +538,7 @@ int esp_output_tail(struct xfrm_state *x, struct sk_buff *skb, struct esp_info *
 	else
 		dsg = &sg[esp->nfrags];
 
-	esph = esp_output_set_extra(skb, x, esp->esph, extra);
+	esph = esp_output_set_extra(skb, x, esp->esph, extra, esp->seqno);
 	esp->esph = esph;
 
 	sg_init_table(sg, esp->nfrags);
diff --git a/net/ipv4/esp4_offload.c b/net/ipv4/esp4_offload.c
index abd77162f5e7..79f7d08325c5 100644
--- a/net/ipv4/esp4_offload.c
+++ b/net/ipv4/esp4_offload.c
@@ -272,7 +272,7 @@ static int esp_xmit(struct xfrm_state *x, struct sk_buff *skb,  netdev_features_
 	struct crypto_aead *aead;
 	struct esp_info esp;
 	bool hw_offload = true;
-	__u32 seq;
+	__u32 seq, seq_hi;
 	int encap_type = 0;
 
 	esp.inplace = true;
@@ -315,7 +315,9 @@ static int esp_xmit(struct xfrm_state *x, struct sk_buff *skb,  netdev_features_
 			return esp.nfrags;
 	}
 
+	/* Keep the sequence used by this packet before advancing GSO state. */
 	seq = xo->seq.low;
+	seq_hi = xo->seq.hi;
 
 	esph = esp.esph;
 	esph->spi = x->id.spi;
@@ -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++;
 
-	esp.seqno = cpu_to_be64(seq + ((u64)xo->seq.hi << 32));
+	esp.seqno = cpu_to_be64(seq + ((u64)seq_hi << 32));
 
 	if (hw_offload && encap_type == UDP_ENCAP_ESPINUDP) {
 		/* In the XFRM stack, the encapsulation protocol is set to iphdr->protocol by
-- 
2.47.3

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

[netdev-bot+sashiko]

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