Page MenuHomeFreeBSD

Simplify compat shims for /dev/crypto.
ClosedPublic

Authored by jhb on Aug 24 2020, 11:57 PM.

Details

Summary
  • Make session handling always use the CIOGSESSION2 structure. CIOGSESSION requests use a thunk similar to COMPAT_FREEBSD32 session requests. This permits the ioctl handler to use the 'crid' field uncoditionally.
  • Move COMPAT_FREEBSD32 handling out of the main ioctl handler body and instead do conversions in/out of thunk structures in dedicated blocks at the start and end of the ioctl function.

Sponsored by: Netflix

Test Plan
  • tested with cryptocheck -d soft -a all -z

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 33182
Build 30531: arc lint + arc unit

Event Timeline

jhb requested review of this revision.Aug 24 2020, 11:57 PM
jhb created this revision.

This looks ok to me, my comments are just nit-picking.

sys/opencrypto/cryptodev.c
408

It makes me slightly uneasy that we're passing void pointers to these functions which have similar names but operate on different-sized structures. Explicit pointer casting might be worth doing here, and you could use the stack variables names. For example, the lines above could be:

session2_op_from_32((struct session2_op32 *)data32, &sopc);
data = &sopc;
741

Why not use else if?

827

It's kind of weird that this is not conditional on error == 0, but I see that it was that way before.

This revision is now accepted and ready to land.Aug 25 2020, 2:13 PM
sys/opencrypto/cryptodev.c
408

I can change it. I mostly did this so that the code followed a more regular pattern. It was easier to copy and paste from one to generate the next. I do the same at the end of always using (data, data32) for similar reasons.

I do think that the casting of data32 is a bit verbose, but I could use the stack variable at least and getting the type right for at least one of the arguments will probably catching any meaningful mismatches.

I should also probably put the compat structures in a union.

741

I guess I could. I had probably been trying to isolate the compat shim here from the regular body of the code, but it can't hurt.

827

Agreed. I'd actually like to axe all the asym crypto stuff. OpenSSL 1.1.0 and later don't use it, it's under-documented, and the only implementation is for some MIPS parts no one probably uses. I do think qat(4) could actually do it in theory, but meh.

sys/opencrypto/cryptodev.c
408

I'm fine with the change as it is, really just noting that I had to double check carefully when reading this bit.

  • Avoid void types for the first set of conversion routines.
This revision now requires review to proceed.Aug 26 2020, 8:14 PM
sys/opencrypto/cryptodev.c
408

I went ahead and changed these to make it easier to avoid mistakes in the future.

I also added a union to conserve stack space.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 26 2020, 9:17 PM
This revision was automatically updated to reflect the committed changes.