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

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

[Jérémy Jean]

When MACsec uses 32-bit packet numbers, 0xffffffff is the last valid
packet number, and after allocating it, MACsec deactivates the
transmit SA and wraps the next packet number to zero. Packets already
in flight can still be processed after that.

The first late packet gets packet number zero and is dropped, but
tx_sa_update_pn() has already advanced the stored counter to one before
macsec_encrypt() drops it. A second late packet can then be sent with
packet number one again, reusing the AES-GCM nonce from the start of the
SA.

Keep next_pn at zero after wrap so all late packets are dropped.

Fixes: c09440f7dcb3 ("macsec: introduce IEEE 802.1AE driver")
Cc: stable@vger.kernel.org
Assisted-by: Codex:gpt-5
Signed-off-by: Jérémy Jean <Jeremy.Jean@oss.cyber.gouv.fr>
---
 drivers/net/macsec.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 78a19b134632..233391acebb0 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;
+
 	if (secy->xpn)
 		tx_sa->next_pn++;
 	else
@@ -493,6 +496,8 @@ static pn_t tx_sa_update_pn(struct macsec_tx_sa *tx_sa,
 
 	if (tx_sa->next_pn == 0)
 		__macsec_pn_wrapped(secy, tx_sa);
+
+out:
 	spin_unlock_bh(&tx_sa->lock);
 
 	return pn;
-- 
2.47.3

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[Sabrina Dubroca]

2026-09-30, 20:33:33 +0000, Jérémy Jean wrote:
> When MACsec uses 32-bit packet numbers, 0xffffffff is the last valid
> packet number

This makes it sound like it's only a problem for 32b packet numbers,
but I think it affects both? Sure it's unlikely with 64b unless we
start from a large offset, but a well-behaved userspace should also
rekey and switch SAs before we ever wrap to avoid losing packets while
the rekey completes (but yes, "losing packets" is not as bad as
"breaking crypto").

> and after allocating it, MACsec deactivates the
> transmit SA and wraps the next packet number to zero. Packets already
> in flight can still be processed after that.

So this is more of a race condition than a full "forever" bug. I think
commit messages should be clear about that (and that doesn't mean it's
not bad or not worth patching, but it's worth mentioning).

Once TX operations hit macsec_encrypt -> macsec_txsa_get and see
!sa->active, packets will be dropped and nonce reuse stops.


> The first late packet gets packet number zero and is dropped, but
> tx_sa_update_pn() has already advanced the stored counter to one before
> macsec_encrypt() drops it. A second late packet can then be sent with
> packet number one again, reusing the AES-GCM nonce from the start of the
> SA.
>
> Keep next_pn at zero after wrap so all late packets are dropped.

"late packet" is defined on the RX side, but it doesn't make sense on
the TX path.

The diff looks good to me.

-- 
Sabrina

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

[Jérémy Jean]

On 2026-10-01 11:43, Sabrina Dubroca wrote:
> 2026-09-30, 20:33:33 +0000, Jérémy Jean wrote:
>> When MACsec uses 32-bit packet numbers, 0xffffffff is the last valid
>> packet number
> 
> This makes it sound like it's only a problem for 32b packet numbers,
> but I think it affects both? Sure it's unlikely with 64b unless we
> start from a large offset, but a well-behaved userspace should also
> rekey and switch SAs before we ever wrap to avoid losing packets while
> the rekey completes (but yes, "losing packets" is not as bad as
> "breaking crypto").
> 
>> and after allocating it, MACsec deactivates the
>> transmit SA and wraps the next packet number to zero. Packets already
>> in flight can still be processed after that.
> 
> So this is more of a race condition than a full "forever" bug. I think
> commit messages should be clear about that (and that doesn't mean it's
> not bad or not worth patching, but it's worth mentioning).
> 
> Once TX operations hit macsec_encrypt -> macsec_txsa_get and see
> !sa->active, packets will be dropped and nonce reuse stops.
> 
> 
>> The first late packet gets packet number zero and is dropped, but
>> tx_sa_update_pn() has already advanced the stored counter to one 
>> before
>> macsec_encrypt() drops it. A second late packet can then be sent with
>> packet number one again, reusing the AES-GCM nonce from the start of 
>> the
>> SA.
>> 
>> Keep next_pn at zero after wrap so all late packets are dropped.
> 
> "late packet" is defined on the RX side, but it doesn't make sense on
> the TX path.
> 
> The diff looks good to me.

Below is suggestion of a hopefully better description.

---

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 only affects TX packets that have already passed the SA
activity check: new packets after deactivation are correctly dropped.
Keep next_pn at 0 after wrap and return early so that the packets that
finishes getting processed are dropped.

---

I can send a v2 if needed.

Jérémy

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

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

--

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

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@kernel.org 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