Details

Reviewers

emaste
allanjude
jhb
scottph
andrew
jmg

Commits

rGf76393a6305b: armv8crypto: add AES-GCM support
rGed9b7f441466: armv8crypto: add AES-GCM support

Summary

Add ktls_armv8 kernel modules that implements AES-GCM encryption
usign ARMv8-accelerated crypto primitives from OpenSSL.

The module supports:

AES128-GCM encryption
AES256-GCM encryption
TLS v1.2
TLS v1.3

Test Plan

Tested on Ampere Computing Altra platform

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

No Lint Coverage

Unit

No Test Coverage

Build Status

Buildable 35393
Build 32310: arc lint + arc unit

Event Timeline

gonzo created this revision.Dec 3 2020, 2:48 AM

Herald added a reviewer: jmg. · View Herald TranscriptDec 3 2020, 2:48 AM

Herald added a subscriber: imp. · View Herald Transcript

gonzo requested review of this revision.Dec 3 2020, 2:48 AM

Harbormaster completed remote builds in B35165: Diff 80253.Dec 3 2020, 2:48 AM

This is an almost ready version to get the initial discussion going. I'd like to get it in shape for HEAD before 13.0 freeze. There are still some issues.

The requirement of AQUIRE_CTX/RELEASE_CTX is not exactly clear to me, the driver seems to work fine without locking on an 80-CPU platform under heavy load. I'd appreciate some help on this topic. What is the failure mode for a race like this?

Also, the driver wasn't tested on the big-endian platform yet.

You would be better off adding AES-GCM support to an OCF driver for arm64 instead. I'm currently looking at retiring the software interface for KTLS and instead only using OCF for software KTLS. In addition, AES-GCM support in OCF would also benefit other use cases like IPsec and ZFS.

That said, my thoughts for arm64 AES-GCM support was to extend ossl(4) to support AES-GCM and use that. However, you could take your AES-GCM implementation and add it to armv8crypto(4).

In D27454#613768, @jhb wrote:

You would be better off adding AES-GCM support to an OCF driver for arm64 instead. I'm currently looking at retiring the software interface for KTLS and instead only using OCF for software KTLS. In addition, AES-GCM support in OCF would also benefit other use cases like IPsec and ZFS.

That said, my thoughts for arm64 AES-GCM support was to extend ossl(4) to support AES-GCM and use that. However, you could take your AES-GCM implementation and add it to armv8crypto(4).

OK, I'll switch it to OCF and armv8crypto. I wrote it as a kTLS interface to avoid extra indirection levels (kTLS/GCM instead of kTLS/OCF/GCM), do you think switching to OCF can have any performance penalties?

In D27454#613772, @gonzo wrote:

In D27454#613768, @jhb wrote:

You would be better off adding AES-GCM support to an OCF driver for arm64 instead. I'm currently looking at retiring the software interface for KTLS and instead only using OCF for software KTLS. In addition, AES-GCM support in OCF would also benefit other use cases like IPsec and ZFS.

That said, my thoughts for arm64 AES-GCM support was to extend ossl(4) to support AES-GCM and use that. However, you could take your AES-GCM implementation and add it to armv8crypto(4).

OK, I'll switch it to OCF and armv8crypto. I wrote it as a kTLS interface to avoid extra indirection levels (kTLS/GCM instead of kTLS/OCF/GCM), do you think switching to OCF can have any performance penalties?

No, I think the performance should be about the same, and if anything I'm trying to minimize OCF overhead as much as possible in my current set of changes,

Move AES-GCM support to armv8crypto acceleration module

Harbormaster completed remote builds in B35393: Diff 80684.Dec 14 2020, 3:50 AM

emaste added a subscriber: mhorne.Dec 14 2020, 2:55 PM

gbe added a subscriber: gbe.Dec 30 2020, 8:51 AM

Hi @gonzo,

I'm looking at D21017, which adds AES-XTS support to armv8_crypto, and has been sitting in review for some time.

I am hoping to get it committed soon, but wanted to get your attention here as it will create some small conflicts with this patch. Let me know if you have any major concerns about that, I'm happy to accommodate so that we might get both of these patches in before 13 branches.

In D27454#624438, @mhorne wrote:

Hi @gonzo,

I'm looking at D21017, which adds AES-XTS support to armv8_crypto, and has been sitting in review for some time.

I am hoping to get it committed soon, but wanted to get your attention here as it will create some small conflicts with this patch. Let me know if you have any major concerns about that, I'm happy to accommodate so that we might get both of these patches in before 13 branches.

Hi @mhorne

No concerns, please go ahead with your patch. I'll resolve conflicts later.

jhb added inline comments.Jan 8 2021, 8:51 PM

sys/crypto/armv8/armv8_crypto.c
208	This should also check `csp_flags` and only accept the flags it supports (for KTLS you will want CSP_F_SEPARATE_AAD and CSP_F_SEPARATE_OUTPUT IIRC). This makes it future-proof against new flags being added in the future.
228	It seems like you support separate output buffers with AES-CBC in your changes, so you should perhaps just keep the csp_flags check in one place and whitelist the flags supported there the way aesni(4) does?
277	Should some of this logic be conditional on the session? E.g. only initializing H for GCM sessions, and only initializing a decrypt key for AES-CBC (and XTS). AES-CTR (which GCM and CCM both use for their cipher side) does not use a separate decryption key schedule but instead uses the encryption key to generate a keystream xored with the cipher/plain text in both directions.
302	Perhaps call this `error` as that is more common in the kernel?
424	I think you can instead remove the `err == 0` check before the copy for the output buffer and set 'err = 0' just before the out label since all the places that set `err` jump to `out`. I would probably also spell it `error`.
439	It's really nicer to reject this in the probesession callback instead. Then you are only checking once per session (and can fall back to another driver) rather than checking once per-op (and failing every request sent if the check fails).
444	Same, though I think you already check this in probesession, so there's no need to re-check it here.
sys/modules/armv8crypto/arm_arch.h
1	This file is already present in sys/crypto/openssl/aarch64/arm_arch.h for use by ossl(4). You should reuse that header rather than adding a new copy, plus it shouldn't live in sys/modules anyway to support compiling the driver into a kernel. Note that you will want to update sys/conf/files.arm64 as well as I believe we plan to add `device armv8crypto` to GENERIC shortly.