What it means is that the salt/nonce is started as the last 4 bytes of the encryption key (sav->key_enc->key_data) provided via setkey(8) or an IKE daemon rather than being stored as the last four bytes in crp_iv. It does seem like the cntr variable here isn't used?

That's the "Salt is ..." comment and memcpy().

I'm talking about the "Nonce is ..." comment and be64enc(). Although now that I think about it, be64enc seems very wrong for a 4-byte value?

Here we (sometimes?) overwrite it with a 4-byte be32enc "1" value. The XXX comment is, uh, "very strong yes." Nonce reuse with other parameters fixed absolutely breaks CTR algorithms. Unless I'm really misunderstanding how this code is used (definite possibility, I have not investigated), this is a SA-type finding.

Both comments are about the same thing (the memcpy). The "salt" comment is for GCM (RFC 4106), the "nonce" comment is for AES-CTR (RFC 3686).

This is the non-GCM case when using AES-CTR. Note that AES-GCM always uses a counter value of '2' here for the start of each packet, and I read the cntr bit wrong. The way this works is that both AES-GCM and AES-CTR are using a 16-byte IV split into 3 fields:

--------------------------------------------------------------
| 4 byte salt | 8 byte packet counter | 4 byte block counter |
--------------------------------------------------------------

This 4 byte salt/nonce is the last 4 bytes of the encryption "key" copied as the first for bytes via the memcpy(). The 8 byte packet counter is cntr that is incremented for each packet (and the IKE daemon is supposed to generate a new key + salt before it rolls over). The last 4 bytes is a per-packet block counter. For AES-GCM this always starts at '2' as part of how AES-GCM works when using a 12-byte IV and a 4 byte counter. This comment is just about using '1' instead of '2' for that block counter for AES-CTR. Using the value '1' is mandated by section 4 of RFC 3686 as the first comment suggests, and it is per-packet, so the XXXAE comment really just needs to be removed. It has to be standardized and not random since this value isn't passed on the wire (only the 8 byte counter is on the wire) and both sides have to use the same 16-byte IV value.

Ah, ok.

Ok, the design seems fine (the packet counter isn't reused without bumping the salt, I take it?) and the comment should be dropped (not necessarily in this revision, but if you wouldn't mind doing it as a driveby, I would appreciate it — you've already written the commit log ;-)).