Page MenuHomeFreeBSD

Update opencrypto for ZFS crypto
AbandonedPublic

Authored by mmacy on Dec 11 2018, 10:59 PM.

Details

Summary

Below are the changes needed to support updating to ZoL as upstream or integrating encrypted volumes support.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 21556

Event Timeline

mmacy created this revision.Dec 11 2018, 10:59 PM
mmacy removed a subscriber: allanjude.
jhb added a subscriber: jhb.Dec 11 2018, 11:17 PM

It would be great to add "ccm" to tools/crypto/cryptocheck.c as well. It does comparisons of /dev/crypto vs OpenSSL's software algorithms. If the NIST-KAT tests include tests for AES-CCM it would also be good to update the nist-kat port to install those (if it doesn't already) and update the python opencrypto tests to run the CCM tests.

sys/crypto/aesni/aesni.c
197

Does CCM support different tag sizes ala GCM or does it always use a 16 byte tag? If the latter we don't need the _16 in its name.

198

Ugh, I really hate that GCM has 3 different constants for GMAC hashes for no good reason (the sessions already get the key length, so there's really no reason for the three constants and it makes drivers do extra parameter checking). In my OCF rework I've mostly killed this though I permit it at the user API to avoid breaking compatibility. If we can just have a single CCM_CBC_MAC that would be great.

441

This particular optimization should probably be a separate commit from adding AES-CCM.

sys/opencrypto/cryptodev.h
211

You can't renumber this, this is part of the ABI.

sef added a comment.Dec 11 2018, 11:26 PM

mmacy, there should be changes for cryptocheck as well.

sef added inline comments.Dec 11 2018, 11:30 PM
sys/crypto/aesni/aesni.c
197

It supports multiple sizes of tags; the tag size is related to the maximum size of the message.

198

The CCM/CCB code was modeled on the GCM code, so similar changes can be applied, no doubt.

441

I'd sent that in via email, but never made a differential for it. I can probably do that. It's included here because the ZFS crypto performance without it is simply untenable.

sys/opencrypto/cryptodev.h
211

Yeah, that's related to the age; CRYPTO_POLY1305 was added after I had done the changes. They should clearly start numbering at 39 now.

mmacy updated this revision to Diff 51880.Dec 11 2018, 11:58 PM
  • fix numbering
  • pull in cryptocheck change
mmacy marked 2 inline comments as done.Dec 11 2018, 11:59 PM
cem added a comment.Dec 12 2018, 12:14 AM

I'd like this broken up into a few revisions. This revision is both too big to commit and too big to review.

  1. the aesni iov optimization
  2. adding the CBC-MAC soft implementations (AES_CBC_MAC_foo, "ccm-cbc.c"). I don't think there's any real reason to call it ccm-cbc.c instead of cbc-mac, since it is a independent algorithm in its own right.
  3. adding the CCM soft implementations (I'm not really clear where these are or how this works; the exf struction points to aes_icm for everything except reinit).
  4. adding CCM/CBC to cryptodev along with some basic tests
  5. adding CCM mode to aesni

I don't have any familiarity with CCM itself; most nitpicks below are outside of aes_ccm.c because I skipped past it. But (1), (2), and (4) above don't require much understanding of CCM at all and I'd be happy to help get those reviewed.

sys/conf/files.amd64
178–183

consider sorting aesni_ccm before aesni_ghash, unless there is some good reason to have them non-alphabetical. same for i386, of course.

sys/crypto/aesni/aesni.c
135

I would suggest alpha ordering here too, although XTS is out of place already. The rest are in-place, though.

198

+1

255–262

I think this might be clearer as a goto + label.

case CRYPTO_AES_CCM_16:
    ccm = true;
    goto setencini;
case ...GCM...:
    gcm = true;
    /* FALLTHROUGH */
...
case CRYPTO_AES_XTS:
setencini:
...
278–289

This bit was already sort of missing a authini != NULL check like below, and now both do. Not a regression in this change.

330–331

Probably worth a comment about CCM here, or expanding the GCM comment above.

334–335

It isn't totally clear, but the encini check plus these equality checks should be sufficient to prove that gcm && ccm is false. I'd add an assertion making that clear: KASSERT((gcm && ccm) ==false, ...);

441

I don't think this optimization is specific to aesni — it's probably something that belongs with generic OCF code, at least. (It could also be done in ZFS instead — constructing single-iov uio's to sent to OCF — but it is a reasonable optimization for OCF to have given it acts on uio objects.)

I agree it should be separated from this revision.

444–447

This could be restructured slightly, I think.

  1. Add a stack variable addr_offset and initialize it to crd_skip.
  2. Pass a pointer to addr_offset to find_vector and use it as an in/out parameter.
  3. if (addr == NULL) goto alloc;, and fallthrough to the ordinary non-allocated path otherwise.
  4. addr += addr_offset; instead of crd_skip.

That would maybe make it cleaner to apply basically the same optimization to mbuf chains.

Anyway, it's mostly fine as-is, except 0 should be false.

743–746

I think this is already well-covered by aesni_cipher_process (which is why it's an assert, not an EINVAL return, I guess) but I'm not sure we need to keep it.

If we do keep it, please remove the second space added in the assertion text.

852–857

It seems odd to use exactly the same condition for both of these, with opposite sense. It also seems odd not to reduce it to a single if-else statement.

854

Probably GMAC_DIGEST_LEN is an inappropriate name for a CCM CBC-MAC digest, even if it happens to be the same length.

sys/crypto/aesni/aesni_ccm.c
449–451

Usually spelled CRYPTDEB() and __func__.

sys/modules/aesni/Makefile
26

I think this comment is stale and refers to gcc4. x86 hasn't used gcc4 since BSD9 or 10. gcc8's manual page documents -mpclmul, and I build all of my kernels with GCC, including aesni.ko, so... maybe just drop the comment along with the original.

sys/opencrypto/xform_aes_icm.c
85

AES_CCM_IV_LEN?

mmacy marked an inline comment as done.Dec 13 2018, 5:24 AM
mmacy updated this revision to Diff 51944.
  • rebase post D18522 commit
  • incorporate some of cem's feedback
mmacy marked 8 inline comments as done and an inline comment as not done.Dec 13 2018, 5:25 AM
mmacy added inline comments.
sys/crypto/aesni/aesni.c
854

so CCM_CBCMAC_DIGEST_LEN?

mmacy marked 4 inline comments as done.Dec 13 2018, 5:34 AM
mmacy updated this revision to Diff 51945.
  • incorporate two missing bits of feedback
mmacy added a comment.Dec 13 2018, 5:35 AM

I think I've incorporated all feedback but the MAC len name change.

mmacy updated this revision to Diff 51946.Dec 13 2018, 5:41 AM

replace GMAC_DIGEST_LEN with CCM_CBC_MAX_DIGEST_LEN

seanc added a subscriber: seanc.Dec 13 2018, 5:47 AM
cem added a comment.Dec 13 2018, 6:11 PM

Some of the stuff I asked for was done, and I'm pleased with r342024 / D18522, but most of what I asked for was not done, and most importantly, this review is still way too large.

sys/crypto/aesni/aesni.c
739

I'd suggest either using MAX() here, or a CTASSERT that GMAC_DIGEST_LEN matches CCM's MAC.

sys/opencrypto/cryptodev.h
383

Yeah, I was going to just make this a straight-up sysctl eventually. Haven't gotten around to it.

cem requested changes to this revision.Dec 13 2018, 6:12 PM
This revision now requires changes to proceed.Dec 13 2018, 6:12 PM
mmacy updated this revision to Diff 51983.Dec 14 2018, 1:53 AM

fix build

sef added a comment.Dec 17 2018, 6:45 PM
In D18520#394737, @cem wrote:
  1. adding the CBC-MAC soft implementations (AES_CBC_MAC_foo, "ccm-cbc.c"). I don't think there's any real reason to call it ccm-cbc.c instead of cbc-mac, since it is a independent algorithm in its own right.
  2. adding the CCM soft implementations (I'm not really clear where these are or how this works; the exf struction points to aes_icm for everything except reinit).
  3. adding CCM/CBC to cryptodev along with some basic tests

So I'm unsure why those would really be broken up -- they're integrated, and the code can't be used (as I recall) unless all three of those items are present.

(To answer another question you had, the aes-ccm uses the same algorithms as the other ciphers, but they differ in how the tag is computed, thus requiring only the one member function difference in that file.)

cem added a comment.Dec 17 2018, 8:56 PM
In D18520#396394, @sef wrote:

So I'm unsure why those would really be broken up -- they're integrated, and the code can't be used (as I recall) unless all three of those items are present.

They should be fully independent algorithms / steps, even if they don't have a consumer until the last step. If they're integrated in a way that they can't be separated and at least compile, I think that's a problem.

sef added a comment.Dec 17 2018, 8:58 PM

They should be fully independent algorithms / steps, even if they don't have a consumer until the last step. If they're integrated in a way that they can't be separated and at least compile, I think that's a problem.

Ok. I can get them split up such that they compile, at least.

cem added a comment.Dec 17 2018, 8:59 PM
In D18520#396447, @sef wrote:

Ok. I can get them split up such that they compile, at least.

Thanks.

grahamperrin_gmail.com added inline comments.
sys/opencrypto/cryptosoft.c
1336

Nit: ragged. (White space?)

emaste added a subscriber: emaste.Feb 7 2019, 2:25 PM
mmacy abandoned this revision.Mar 24 2019, 7:22 PM

All changes have gone in and been MFC’d through smaller reviews.