ossl: Add AES-GCM implementation for ARMv7 NEON
Needs ReviewPublic
Actions

Authored by kd on Nov 17 2022, 2:51 PM.

Details

Reviewers

wma
mw
jhb
mkoz_semihalf.com

Summary

Add GCM implementation based on OSSL source code.
It uses dedicated openssl NEON assembly functions: gcm_init_neon, gcm_ghash_neon, gcm_gmult_neon.
Works with 12 byte IV and 16 byte tag only.
Tested on armada388-clearfog with NISTs vector set.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

mkoz_semihalf.com created this revision.Nov 17 2022, 2:51 PM

Herald added a subscriber: andrew. · View Herald TranscriptNov 17 2022, 2:51 PM

mkoz_semihalf.com requested review of this revision.Nov 17 2022, 2:51 PM

mw added a reviewer: jhb.Nov 23 2022, 8:58 AM

jhb added inline comments.Nov 28 2022, 11:41 PM

sys/crypto/openssl/ossl.c
160	style(9) wants to keep this blank line
sys/crypto/openssl/ossl_aes.c
61	Given the amount of modifications needed for GCM vs CBC compared to the duplication, I think it would be cleaner to have a separate osal_aes_gcm and leave osal_aes_cbc as-is. It's also worth it I think given the point is accelerated crypto to avoid at least some branches that way by using dedicated functions for the different modes.
112	This will not work with stream ciphers like GCM where payloads are not a multiple of the block size.
163
166
167	Tag mismatches should fail with `EBADMSG` not `EINVAL`

Please remove the file mode change to tests/sys/opencrypto/runtests.sh.

sys/crypto/openssl/ossl.h
70	I'm not sure if we need the Htable to be this big. It's main use case is to precompute different values of H, so that later we can apply calculate ghash on multiple blocks at the same time. From what I can see `gcm_init_neon` uses only a single value of H, so `uint128_t Htable[1];` should be sufficient. With a smaller context we might not need to increase the size kernel stack. (D37441)

Fix typo
Separate function for ossl_aes_cbc and ossl_aes_gcm
Revert tests/sys/opencrypto/runtests.sh
Change return EINVAL to EBADMSG
Decrease Htable size to single 128bit value

TODO: fix to work with stream ciphers

The biggest use case of AES-GCM is in the ipsec stack. Actually, that layer should be responsible for providing valid data to ossl. It is expected for the ossl function to fail if lengths are not aligned to block size (similar to what AES CBC does) as only the upper layers have the knowledge what algorithm has to be used for padding, if any. The AES-GCM RFC does not define any standard padding mechanism as this is the "stream encryption", but RFCs which make use of AES-GCM might have.

@mkoz_semihalf.com please ensure if all code paths have implemented the len % blocksize check and return EINVAL in that case.

In D37421#854943, @wma wrote:

The biggest use case of AES-GCM is in the ipsec stack. Actually, that layer should be responsible for providing valid data to ossl. It is expected for the ossl function to fail if lengths are not aligned to block size (similar to what AES CBC does) as only the upper layers have the knowledge what algorithm has to be used for padding, if any. The AES-GCM RFC does not define any standard padding mechanism as this is the "stream encryption", but RFCs which make use of AES-GCM might have.

@mkoz_semihalf.com please ensure if all code paths have implemented the len % blocksize check and return EINVAL in that case.

Huh? Rejecting that would be utterly wrong. AES-GCM as a stream cipher permits payloads that are not a multiple of a block size. tools/tools/crypto/cryptocheck explicitly checks for that for stream ciphers like AES-CTR and AES-GCM in its tests with -z as well. There is no padding for IPsec when using a stream cipher.

kd commandeered this revision.Dec 13 2022, 11:25 AM

kd updated this revision to Diff 114096.

kd edited reviewers, added: mkoz_semihalf.com; removed: kd.

kd marked 5 inline comments as done.

kd edited the summary of this revision. (Show Details)

In D37421#855702, @jhb wrote:

Huh? Rejecting that would be utterly wrong. AES-GCM as a stream cipher permits payloads that are not a multiple of a block size. tools/tools/crypto/cryptocheck explicitly checks for that for stream ciphers like AES-CTR and AES-GCM in its tests with -z as well. There is no padding for IPsec when using a stream cipher.

I've fixed that, please take a look.
I've also added support for 192 and 256bit keys.

With those changed the NIST test suite(tests/sys/opencrypto) seems to work just fine.

markj mentioned this in D39783: ossl: Add a VAES-based AES-GCM implementation for amd64.Apr 24 2023, 9:54 PM

markj mentioned this in rG9a3444d91c70: ossl: Add a VAES-based AES-GCM implementation for amd64.Jun 2 2023, 4:19 PM