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

[Jérémy Jean]

On 2026-10-02 13:35, netdev-bot+sashiko@xxxxxxxxxx wrote:
> 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
>
> [ ... ]


AFAICT, sashiko is partly right. Reactivating the SA after PN wrap
without a PN update could indeed also lead to nonce reuse: before the
fix, the counter could advance from 0 to 1. Yet, the security fix
provided in the diff is the correct one: further PN allocations return
0 and the corresponding packets are dropped. However, I believe that
the statement "macsec_encrypt() never checks tx_sa->active" is
misleading: the check is performed in macsec_txsa_get(), which is
called by macsec_encrypt(), but reactivation makes that check pass.

The observation about active and operational remaining true after
reactivation is also correct, although packets are dropped because
next_pn remains 0.

Here is a rewording of the final part that should cover the comment:

---

After allocating the last valid packet number, MACsec wraps next_pn to
zero and deactivates the transmit SA. This happens for both 32- and
64-bit types of packet numbers (at values 0xffffffff and
0xffffffffffffffff, respectively).

TX packets that were still getting processed during deactivation keep
being processed and then receive packet numbers. The first gets 0 and
is correctly dropped, but next_pn is incremented to 1, which makes the
next packet take number 1. It then does not get dropped and may induce
a reuse of the AES-GCM nonce corresponding to value 1.

This race affects TX packets that have already passed the SA activity
check: packets that observe the inactive SA are correctly dropped.
Reactivating the SA after PN wrap without a packet number can also
cause nonce reuse. Keep next_pn at 0 after wrap, even if the SA is
reactivated, so further packet number allocations return 0 and the
corresponding packets are dropped.

---

Please tell me whether I should submit this as a v2.

Jérémy