Re: [PATCH ipsec v2 4/7] xfrm: prevent AES-GCM nonce reuse after early GSO

[netdev-bot+sashiko]

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 code change is correct, but the commit message for "xfrm:
  prevent AES-GCM nonce reuse after early GSO" describes a narrower…

--

xfrm: prevent AES-GCM nonce reuse after early GSO

This commit calls secpath_set() on each segment in xfrm_output_gso() when
xfrm_offload() is set. As a result, segments produced by skb_gso_segment()
no longer share a single xo->seq. Each segment's IV is then built from its
own sequence number.

> xfrm: prevent AES-GCM nonce reuse after early GSO
>
> With software ESP offload, xfrm_output() attaches a secpath before
> calling xfrm_output_gso() for an encapsulated GSO packet, but
> skb_gso_segment() leaves the splitted skbs sharing the same secpath,
> including xo->seq.

[ ... ]

> segments. When validate_xmit_xfrm() calls esp*_xmit() for each
> segment, the IV is constructed from the same value N+t-1, causing
> nonce reuse in AES-GCM. Yet, each ESP header correctly keeps using
> value N+i from its private XFRM_SKB_CB.

[Severity: Medium]
The code change looks correct. Is the commit message narrower than the bug
it fixes, though?

Is the problem limited to software ESP offload? In xfrm_output(), the early
GSO path runs for any state where xfrm_dev_offload_ok() is true. That
includes XFRM_DEV_OFFLOAD_CRYPTO states bound to a NIC:

net/xfrm/xfrm_output.c:xfrm_output() {
    ...
	if (xfrm_dev_offload_ok(skb, x)) {
		...
		if (skb_is_gso(skb)) {
			if (skb->inner_protocol && x->props.mode == XFRM_MODE_TUNNEL)
				return xfrm_output_gso(net, sk, skb);
    ...
}

In that case, xfrm_replay_overflow_offload() writes N+i into the shared xo
for each segment. If the segments are queued before validate_xmit_xfrm()
runs, esp_xmit() reads the shared value N+t-1. On the hw_offload branch it
then copies that value into the private xo:

net/ipv4/esp4_offload.c:esp_xmit() {
    ...
	seq = xo->seq.low;
    ...
	if (hw_offload && encap_type == UDP_ENCAP_ESPINUDP) {
		...
		esph->seq_no = htonl(seq);
	}
    ...
	if (hw_offload) {
		if (!skb_ext_add(skb, SKB_EXT_SEC_PATH))
			return -ENOMEM;
		...
		xo->seq.low = seq;
    ...
}

Drivers build the IV from this xo->seq. For example, mlx5e_ipsec_set_iv()
does:

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

and nfp copies xo->seq.low and xo->seq.hi into offload_info. Wouldn't
hardware crypto offload users hit the same nonce reuse?

Is the statement "each ESP header correctly keeps using value N+i"
accurate? For hardware offload with UDP_ENCAP_ESPINUDP, the
esph->seq_no = htonl(seq) assignment above overwrites the header with the
shared value. Every segment would then carry the same sequence number on
the wire, and the peer's anti-replay check would drop t-1 of the t
segments.

Is the problem specific to AES-GCM? In the software fallback, esp.seqno
feeds the seqiv-based IV for any counter-nonce AEAD the SA uses. That
includes rfc4106(gcm), rfc4309(ccm), rfc4543(gmac) and
rfc7539esp(chacha20,poly1305).

The new secpath_set() call in xfrm_output_gso() is guarded only by
xfrm_offload(segs), so the fix appears to cover all of these cases. Could
the subject and commit message be updated to mention hardware crypto
offload, the UDP encapsulation ESP header sequence number, and the non-GCM
AEAD modes?

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