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

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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
409 ↗(On Diff #76188)

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;
738 ↗(On Diff #76188)

Why not use else if?

824 ↗(On Diff #76188)

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
409 ↗(On Diff #76188)

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.

738 ↗(On Diff #76188)

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.

824 ↗(On Diff #76188)

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
409 ↗(On Diff #76188)

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
409 ↗(On Diff #76188)

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.