Re: [PATCH net] macsec: prevent AES-GCM nonce reuse after packet number wrap

[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 code change is correct.

--

macsec: prevent AES-GCM nonce reuse after packet number wrap

With 32-bit packet numbers, the TX SA wraps next_pn to zero after
allocating 0xffffffff. tx_sa_update_pn() used to advance the counter
past zero, so later frames could reuse PN 1, 2, ... under the same key.
This patch makes tx_sa_update_pn() return early when the stored PN is
zero, so next_pn stays at zero after a wrap.

> diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
> index 78a19b1346321..233391acebb0f 100644
> --- a/drivers/net/macsec.c
> +++ b/drivers/net/macsec.c
> @@ -486,6 +486,9 @@ static pn_t tx_sa_update_pn(struct macsec_tx_sa *tx_sa,
>  	spin_lock_bh(&tx_sa->lock);
>  
>  	pn = tx_sa->next_pn_halves;
> +	if (unlikely(pn.full64 == 0))
> +		goto out;
> +

[Severity: Medium]
The code change looks right. The commit message may describe less than
the patch actually fixes and changes, though. It says:

    Packets already in flight can still be processed after that.
    [ ... ]
    Keep next_pn at zero after wrap so all late packets are dropped.

Is late in-flight packets the only way to hit the nonce reuse? A wrapped
SA can also be reactivated from userspace without a new PN. In
macsec_upd_txsa(), MACSEC_SA_ATTR_PN is optional:

drivers/net/macsec.c:macsec_upd_txsa() {
    ...
	if (tb_sa[MACSEC_SA_ATTR_ACTIVE])
		tx_sa->active = nla_get_u8(tb_sa[MACSEC_SA_ATTR_ACTIVE]);

	was_operational = secy->operational;
	if (assoc_num == tx_sc->encoding_sa)
		secy->operational = tx_sa->active;
    ...
}

macsec_start_xmit() only checks secy->operational. macsec_encrypt()
never checks tx_sa->active, and its only guard is the PN check:

drivers/net/macsec.c:macsec_encrypt() {
    ...
	pn = tx_sa_update_pn(tx_sa, secy);
	if (pn.full64 == 0) {
		macsec_txsa_put(tx_sa);
		kfree_skb(skb);
		return ERR_PTR(-ENOLINK);
	}
    ...
}

Before this patch, the first frame after an UPD_TXSA that only sets
ACTIVE would get PN 0 and be dropped. Every frame after it would go out
with PN 1, 2, 3 ... under the same key and salt, for as long as the SA
stayed in use. That is sustained nonce reuse, not a short race window.
Should the commit message mention this path, especially since the patch
is tagged for stable?

There is also a user-visible behaviour change here. After this patch,
reactivating a wrapped SA without a new PN drops every frame and counts
it in tx_dropped. The goto out also skips __macsec_pn_wrapped(), so
nothing clears tx_sa->active or secy->operational again. The SA and
SecY keep reporting active and operational while all TX is dropped,
until a new PN or SA is installed.

Could the commit message document this as well?

>  	if (secy->xpn)
>  		tx_sa->next_pn++;
>  	else

[ ... ]

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