Update opencrypto for ZFS crypto

[mmacy]

Summary

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

[jhb]

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]

mmacy, there should be changes for cryptocheck as well.

[sef]

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]

• fix numbering
• pull in cryptocheck change

[cem]

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]

• rebase post D18522 commit
• incorporate some of cem's feedback

[mmacy]

sys/crypto/aesni/aesni.c

854

so CCM_CBCMAC_DIGEST_LEN?

[mmacy]

• incorporate two missing bits of feedback

[mmacy]

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

[mmacy]

replace GMAC_DIGEST_LEN with CCM_CBC_MAX_DIGEST_LEN

[cem]

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.

[mmacy]

fix build

[sef]

In D18520#394737, @cem wrote:

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

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]

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]

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]

In D18520#396447, @sef wrote:

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

Thanks.

[grahamperrin]

sys/opencrypto/cryptosoft.c

1336

Nit: ragged. (White space?)

[mmacy]

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