Page MenuHomeFreeBSD

Various cleanups to the software encryption transform interface.
ClosedPublic

Authored by jhb on May 16 2020, 12:08 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Apr 7, 9:37 PM
Unknown Object (File)
Mar 7 2024, 8:42 PM
Unknown Object (File)
Feb 16 2024, 10:36 PM
Unknown Object (File)
Feb 16 2024, 10:35 PM
Unknown Object (File)
Feb 16 2024, 10:35 PM
Unknown Object (File)
Feb 16 2024, 10:35 PM
Unknown Object (File)
Feb 16 2024, 8:59 PM
Unknown Object (File)
Feb 8 2024, 6:46 PM
Subscribers

Details

Summary
  • Consistently use 'void *' for key schedules / key contexts instead of a mix of 'caddr_t', 'uint8_t *', and 'void *'.
  • Add a ctxsize member to enc_xform similar to what auth transforms use and require callers to malloc/zfree the context. The setkey callback now supplies the caller-allocated context pointer and the zerokey callback is removed. Callers now always use zfree() to ensure key contexts are zeroed.
  • Consistently use C99 initializers for all statically-initialized instances of 'struct enc_xform'.
  • Change the encrypt and decrypt functions to accept separate in and out buffer pointers. Almost all of the backend crypto functions already supported separate input and output buffers and this makes it simpler to support separate buffers in OCF.
  • Remove xform_userland.h shim to permit transforms to be compiled in userland. Transforms no longer call malloc/free directly.
Test Plan
  • cryptocheck -a all -d soft -z

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jhb requested review of this revision.May 16 2020, 12:08 AM

All my feedback below are minor style nits; functionally it looks good.

I did not look at the camellia or null xform changes as they should be disused / deleted, respectively.

sys/crypto/chacha20/chacha-sw.c
18 ↗(On Diff #71840)

Can drop the cast now.

71 ↗(On Diff #71840)

Do any xforms need something more sophisticated than zfree(sched, M_CRYPTO_DATA)?

After reviewing the other changes, it doesn't look like it. Maybe we can shed a method from the vtable and just use zfree? I'm warming up to that function.

sys/opencrypto/xform_aes_icm.c
171 ↗(On Diff #71840)

Maybe just malloc() and sizeof(*ctx) while you're here?

sys/opencrypto/xform_aes_xts.c
103 ↗(On Diff #71840)

s/u_int/bool/?

134 ↗(On Diff #71840)

true/false

151 ↗(On Diff #71840)

malloc?

sys/opencrypto/xform_null.c
56 ↗(On Diff #71840)

stab stab.

we shouldn't have IPsec specific crap like a "null" cipher in OCF.

sys/opencrypto/xform_rijndael.c
64 ↗(On Diff #71840)

The other AES modes have names like, e.g., "AES-CBC" (or maybe it should be "AES128-CBC"?
there are a lot of 128s here). Should we canonicalize on something like that (probably the more common naming scheme today) rather than the pre-standardization Rijndael name?

99–104 ↗(On Diff #71840)

This would be a bit more straightforward with the common early ENOMEM return pattern.

This revision is now accepted and ready to land.May 16 2020, 2:27 AM
jhb marked 2 inline comments as done.May 18 2020, 8:44 PM
jhb added inline comments.
sys/crypto/chacha20/chacha-sw.c
71 ↗(On Diff #71840)

I think if we did the malloc elsewhere (e.g. if enc_xform exported a "size" member and it was allocated by the framework similar to softy's) I'd be fine doing the zfree outside, but I think it's better to have the malloc and free in the same place. I'm not sure if all of the xform's use a static size for their context. If they did, I'd be happy to move the entire malloc out and just have setkey() take a pointer to the pre-allocated space. I'm not really worried about zerokey being a hot path either.

sys/opencrypto/xform_aes_icm.c
171 ↗(On Diff #71840)

So the KMALLOC stuff is some combat shim to let you compile this code into userland or the kernel. I've kind of already broken that with zfree, so I should perhaps remove it entirely.

sys/opencrypto/xform_aes_xts.c
103 ↗(On Diff #71840)

I considered that, but I'm not really sure 'true' and 'false' are any more readable than '0' or '1'. If anything, what might make sense is an enum so it can be named in the caller.

sys/opencrypto/xform_rijndael.c
64 ↗(On Diff #71840)

I would like to call this AES-CBC, yes. Perhaps as a followup. I have changes for userland to do this for IPsec bits already that I just haven't posted for review.

  • Fixes from Conrad.
  • Simplify memory management for ciphers.
This revision now requires review to proceed.May 18 2020, 10:20 PM
jhb marked an inline comment as done.May 18 2020, 10:25 PM
This revision was not accepted when it landed; it landed in state Needs Review.May 20 2020, 9:21 PM
This revision was automatically updated to reflect the committed changes.