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

[Sabrina Dubroca <sd@queasysnail.net>]

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.


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

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.


Thanks.

-- 
Sabrina