Page MenuHomeFreeBSD

Add support for IPSec ESN and pass relevant information to crypto layer

Authored by on Nov 14 2019, 12:31 PM.



Implement support for including IPSec ESN (Extended Sequence Number) to
both encrypt and authenticate mode (eg. AES-CBC and SHA256) and combined
mode (eg. AES-GCM). Both ESP and AH protocols are updated. Additionally
pass relevant information about ESN to crypto layer.

For the ETA mode the ESN is stored in separate crp_esn buffer because
the high-order 32 bits of the sequence number are appended after the
Next Header (RFC 4303).

For the AEAD modes the high-order 32 bits of the sequence number
[e.g. RFC 4106, Chapter 5 AAD Construction] are included as part of
crp_aad (SPI + ESN (32 high order bits) + Seq nr (32 low order bits)).

Diff Detail

Lint Skipped
Unit Tests Skipped

Event Timeline retitled this revision from Pass information about ESN to crypto layer to Add support for IPSec ESN and pass relevant information to crypto layer. edited the summary of this revision. (Show Details)

Overall looks fine to me. I can't recall if I had suggested an explicit crp_flag to know when crp_esn was valid (rather than assuming that all operations in an ESN session have it, but perhaps it's ok to use the session flag. With some other flags like SEPARATE_AAD or separate output buffers clients want the option on a per-request basis, but with ESN the client (IPsec) will always be setting it.


Moving this is fine but looks like a no-op?


Alternatively, you could move the free(crp->crp_aad, M_XDATA) under the existing if (crp != NULL) below since free(NULL, ..) is defined to be a nop.


As below, you can just call free() unconditionally. There is also no need to clear crp_aad.


This can just be unconditional like the rest of this block since free(NULL) is ok.

FYI, I've tried to set parent/child relationships for all the ESN-related reviews (you can see the graph in the "Stack" tab).

Address @jhb comments mostly regarding to freeing crp->crp_aad.

By mistake previous diff was updated without full context- fix this by uploading diff with full context. added inline comments.

This was leftover from previous approach - I've undo this change. Thank you.

This patch contains many magic numbers 4, 8 and 32 that should either be removed by the proper use of sizeof() or turned into #defines that are named.


Please change the magic number (4) to either a #define or sizeof(seqh)


Same as previous memcpy


Push else up to } else and please check indenting in this section.


Add braces to make this clearer as to which operation goes where.


Convert magic number (4) to a #define or const.


#dedfine for magic number marked an inline comment as done.

Address @gnn comments: get rid of various magic numbers and improve formatting.

jhb added inline comments.

Minor nit: blank line before here.


Here as well.

This revision is now accepted and ready to land.Oct 6 2020, 5:52 PM