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
Details
Details
Tested on Ampere Computing Altra platform
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 36193 Build 33082: arc lint + arc unit
Event Timeline
Comment Actions
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.
Comment Actions
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).
Comment Actions
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?
Comment Actions
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,
Comment Actions
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.
Comment Actions
Hi @mhorne
No concerns, please go ahead with your patch. I'll resolve conflicts later.
| sys/crypto/armv8/armv8_crypto.c | ||
|---|---|---|
| 213 | 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. | |
| 236 | 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? | |
| 318 | 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. | |
| 327 | Perhaps call this error as that is more common in the kernel? | |
| 451 | 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. | |
| 475 | 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). | |
| 480 | 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 ↗ | (On Diff #80684) | 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. |
Comment Actions
Sync to the latest HEAD:
- Update with the allanjude's patch rebased on the latest main
- Address jhb's comments
| sys/modules/armv8crypto/Makefile | ||
|---|---|---|
| 35 | Doesn't sys/conf/files.arm64 need to be updated as well? With this patch a kernel compiled with "device armv8crypto" will fail to link. | |
| sys/crypto/armv8/armv8_crypto.c | ||
|---|---|---|
| 116 | This can just use 'device_set_desc'. It's not a dynamic string, so a copy isn't required. That is an old bug, but not worth perpetuating further. | |