Re: [PATCH ipsec v2 3/7] xfrm: esp: use the current sequence number for AAD and offload

[netdev-bot+sashiko@kernel.org]

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

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

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).

- [Medium] The commit message says the bug is a wrong ESN high word at
  the 2^32 boundary: (H, 0xffffffff) gets authenticated with H+1.

--

xfrm: esp: use the current sequence number for AAD and offload

This commit passes esp->seqno into esp_output_set_extra() and
esp_output_set_esn(), so the software ESN AAD takes its high bits from the
segment's own sequence number instead of the already advanced xo->seq.hi.

For hardware offload, esp_xmit() and esp6_xmit() now write the current
sequence number into both xo->seq.low and xo->seq.hi of the private
secpath copy after skb_ext_add().

> diff --git a/net/ipv4/esp4_offload.c b/net/ipv4/esp4_offload.c
> index 3a0aafe991f47..6f6ce9ea5c491 100644
> --- a/net/ipv4/esp4_offload.c
> +++ b/net/ipv4/esp4_offload.c
> @@ -358,6 +358,10 @@ static int esp_xmit(struct xfrm_state *x, struct sk_buff *skb,  netdev_features_
>  		if (!xo)
>  			return -EINVAL;
>  
> +		/* Keep the current sequence number in the private copy. */
> +		xo->seq.low = seq;
                ^^^^

[Severity: Medium]
Does the commit message cover everything this line fixes?

The message talks about the ESN high word being wrong at the 2^32
boundary. Restoring xo->seq.low here also changes the low half for every
GSO segment that is split in software and then encrypted in hardware. No
ESN wrap is needed for that.

Before this patch, esp_xmit() saved seq, advanced the shared counter and
only then called skb_ext_add():

    seq = xo->seq.low;
    ...
    if (xo->flags & XFRM_GSO_SEGMENT) {
        esph->seq_no = htonl(seq);

        if (!skb_is_gso(skb))
            xo->seq.low++;
    ...
    if (!skb_ext_add(skb, SKB_EXT_SEC_PATH))

skb_ext_add()->skb_ext_maybe_cow() memcpy()s the already advanced
xfrm_offload into the private copy, so each segment carried seq + 1. The
last segment keeps the shared copy, and that one has been advanced too.

mlx5e_ipsec_set_iv() then builds the on-wire IV from that value:

    seqno = cpu_to_be64(xo->seq.low + ((u64)xo->seq.hi << 32));
    iv_offset = skb_transport_offset(skb) + sizeof(struct ip_esp_hdr);
    skb_store_bits(skb, iv_offset, &seqno, 8);

As a result, segment L+i went out with IV L+i+1, and the last segment
(L+n-1) used IV L+n. The next non-GSO packet gets sequence L+n from
xfrm_replay_overflow_offload_esn(), so its IV is also L+n.

Isn't that AES-GCM nonce reuse under the same key and salt?

This path is reachable when a device has NETIF_F_HW_ESP set but not
NETIF_F_GSO_ESP. One example is mlx5 without swp_lso, where
mlx5e_ipsec_build_netdev() returns before it enables GSO_ESP. Another is
running ethtool -K tx-esp-segmentation off.

The only related text in the commit message is:

  "Restore both halves because drivers can also use xo->seq to generate
  the IV."

That sentence does not say that the low half was off by one for every
segment, or that IVs were being reused.

The low-half off-by-one appears to come from c7dbf4c08868 ("xfrm: Provide
private skb extensions for segmented and hw offloaded ESP packets"), which
added the skb_ext_add() call after the increment. That commit is not in
the Fixes: tags.

The code change itself looks correct. Could the commit message describe
the per-segment IV reuse and add a Fixes: tag for c7dbf4c08868, so stable
and security triage can tell what is being fixed?

The same applies to the identical hunk in esp6_xmit() in
net/ipv6/esp6_offload.c.

> +		xo->seq.hi = upper_32_bits(be64_to_cpu(esp.seqno));
> +
>  		xo->flags |= XFRM_XMIT;
>  		return 0;
>  	}

[ ... ]

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