Re: [PATCH] xfrm: esp6: fix off-by-one IV counter causing AES-GCM nonce reuse

["Jérémy Jean" <jeremy.jean@oss.cyber.gouv.fr>]

On 2026-09-29 15:00, Sabrina Dubroca wrote:
> 2026-09-29, 11:47:47 +0200, Jérémy Jean wrote:
>> Hello Sabrina,
>> 
>> On 2026-09-29 01:48, Sabrina Dubroca wrote:
>> > 2026-09-28, 21:33:41 +0200, Jérémy Jean wrote:
>> > > On 2026-09-28 18:19, Sabrina Dubroca wrote:
>> > > > 2026-09-25, 09:51:06 +0000, Jérémy Jean wrote:
>> > > > > An off-by-one error in esp6_xmit() advances the IV counter before
>> > > > > encrypting each software-GSO segment. For N segments with sequence
>> > > > > numbers X through X+N-1, the IV counters are therefore X+1 through
>> > > > > X+N.
>> > > > > The following non-GSO packet uses X+N for both its sequence number and
>> > > > > IV counter, repeating the last segment's AES-GCM nonce under the same
>> > > > > key.
>> > > >
>> > > > I find this description very unclear. All I'm managing to understand
>> > > > from this is "there's some situation where a packet isn't getting the
>> > > > seqno it should". I don't know where the "+1" comes from since for GSO
>> > > > the function does +N (xo->seq.low += skb_shinfo(skb)->gso_segs).
>> > >
>> > > I tried to be as explicit as possible, but I apologize if it was not
>> > > good enough. My understanding on the full GSO processing isn't as
>> > > deep as yours, so here is another try at explaining.
>> > >
>> > > The bug happens after software segmentation in GSO. When a large
>> > > amount of data needs to span across several packets, software
>> > > segmentation splits it into N smaller skb. After this split, each
>> > > smaller skb holding the individual packets has skb_is_gso(skb)
>> > > returning false, yet each skb keeps the flag XFRM_GSO_SEGMENT stating
>> > > that this skb resulted from a segmentation. Consequently, the skb
>> > > goes through the increment below in esp6_xmit():
>> > >
>> > > net/ipv6/esp6_offload.c:
>> > >    355		if (xo->flags & XFRM_GSO_SEGMENT) {
>> > >    356			esp.esph->seq_no = htonl(seq);
>> > >    357
>> > >    358			if (!skb_is_gso(skb))
>> > >    359				xo->seq.low++;      // <<< increment here
>> > >    360			else
>> > >    361				xo->seq.low += skb_shinfo(skb)->gso_segs;
>> > >    362		}
>> > >
>> > > There are N calls to esp6_xmit() for all the smaller packets, and for
>> > > each of them, the current sequence number is first written into the
>> > > header, and then the shared counter for the next packet is
>> > > incremented. However, the value esp.seqno used to construct the IV is
>> > > derived from the counter value _after_ the increment.
>> >
>> > Ok, I see now. One call to validate_xmit_xfrm() that calls
>> > skb_gso_segment() and feeds those N non-GSO skbs to ->xmit one by one.
>> 
>> Yes, precisely.
>> 
>> > In that case, the seqno used in the IV wouldn't match the one that the
>> > peer will reconstruct using the bottom 32b of seqno present in the
>> > header, and it would never manage to decrypt anything we sent.
>> 
>> I don't think the peer uses its internal counter to reconstruct the 
>> IV?
> 
> For some reason when looking at this last night I thought it was
> rebuilding it from the seqno in the esp header.
> 
>> The IV is part of the GCM ciphertext and the peer uses that value it
>> received to decrypt the payload. AFAICT, the decryption is ultimately 
>> done
>> in seqiv_aead_decrypt(), where one can see the IV copy (121), and the
>> actual decryption call (123).
>> 
>> crypto/seqiv.c:
>>     99	static int seqiv_aead_decrypt(struct aead_request *req)
>>    100	{
>>                 // ...
>>    116		aead_request_set_callback(subreq, req->base.flags, compl, 
>> data);
>>    117		aead_request_set_crypt(subreq, req->src, req->dst,
>>    118				       req->cryptlen - ivsize, req->iv);
>>    119		aead_request_set_ad(subreq, req->assoclen + ivsize);
>>    120
>>    121		scatterwalk_map_and_copy(req->iv, req->src, req->assoclen, 
>> ivsize,
>> 0);
>>    122
>>    123		return crypto_aead_decrypt(subreq);
>>    124	}
>> 
>> > But luckily, it seems commenting out the memcpy(iv, seqno) line has no
>> > effect (I think that's because all algorithms rely on either seqiv or
>> > echainiv).
>> 
>> I may misunderstand, but which memcpy() do you refer to ?
>> If you do not memcpy, then IV is always null, and the nonce reuse is 
>> even
>> worse, no?
> 
> Yeah right. I thought there was something dodgy there.
> 
>> > > For example, if
>> > > the last packet produced by segmentation has sequence number 100, the
>> > > IV is constructed using counter value 101. Then, a subsequent
>> > > ordinary packet not going through segmentation is allocated sequence
>> > > number 101, yet since it does not have the flag XFRM_GSO_SEGMENT,
>> > > there is no increment, and its value is constructed from value 101 as
>> > > well. Hence the nonce repetition.
>> >
>> > I don't think that happens? The other packet will go through ->xmit
>> > too and use the wrong seqno too.
>> 
>> Which "wrong seqno"? The other packet will indeed, go through ->xmit,
>> but its lack of XFRM_GSO_SEGMENT will make it skip the increment and
>> reuse the previous counter value.
> 
> Eh, ok. I thought you were saying one goes through ->xmit (with the
> wrong seqno because it has been incremented) and the other through
> ->output.
> 
> Then I guess this makes sense.

Good that we agree then, thanks :)

> Could you:
> 
>  1. fix both bugs you found so that the code looks similar, probably
>     bundled as a small series (this stuff is not ipv*-specific, so
>     there's no reason for it to be implemented differently)
> 
>  2. rewrite the commit messages based on this thread to be much more
>     precise and also less verbose

Yes, I will try do this shortly.

> I'd also reduce the amount of crypto detail about GCM, I don't think
> it's super relevant. Or move it to a cover letter.

I purposedly emphasized the crypto part about GCM to make it clear to
non-crypto people that the nonce reuse impact is catastrophic (all zero 
or
even a single reuse). I will move it to the cover letter then.

Regards,
Jérémy