crypto: core: Remove ce_aes_ctr_encrypt() to fix AES-GCM compliance - #7833
Conversation
| while (num_blocks) { | ||
| pmull_ghash_update(1, dg, src, &state->ghash_key, NULL); | ||
| ce_aes_ctr_encrypt(dst, src, (const uint8_t *)ek->data, | ||
| ce_aes_gcm_ctr_encrypt(dst, src, (const uint8_t *)ek->data, |
There was a problem hiding this comment.
For Aarch64, update_payload_2block() does the heavy lifting, and this function is only called for a single block. We could replace the ce_aes_ctr_encrypt() call with the counterpart of what's done in encrypt_pl() without noticeable impact on the performance.
The drawback is that for Aarch32 (still on Armv8-A hardware), we'd get slightly slower performance. But we're still processing block by block, so it can't be that great to start with.
Do we care enough about Aarch32 to duplicate the two assembly functions ce_aes_ctr_encrypt()? If yes, please rename ce_aes_gcm_ctr_encrypt() to ce_aes_ctr_inc32_encrypt() and see if it can be refactored together with ce_aes_ctr_encrypt() to avoid some duplicated code.
There was a problem hiding this comment.
Replaced ce_aes_ctr_encrypt() with the counterpart of what's done in encrypt_pl().
6b59804 to
bd1a59f
Compare
MingyenHung
changed the title
| ek->rounds, 1, (uint8_t *)state->ctr, 1); | ||
|
|
||
| ce_aes_ecb_encrypt(buf_cryp, (const uint8_t *)state->ctr, | ||
| (const uint8_t *)ek->data, ek->rounds, |
There was a problem hiding this comment.
Please align with the opening ( on the row above.
| 1, 1); | ||
| internal_aes_gcm_inc_ctr(state); | ||
|
|
||
| ce_aes_xor_block(dst, buf_cryp, src); |
There was a problem hiding this comment.
I'm a bit concerned that we read src twice. I don't know whether it can be exploited right now, but I'd like to protect against this scenario:
src is untrusted memory and can be updated by an untrusted CPU, but dst is in trusted memory and can only be updated by trusted CPUs. What if the untrusted CPU updates the src content after the GHASH stage, so something else is decrypted? We'd think that dst is alright since the tag is correct.
There was a problem hiding this comment.
Please review the new patchset d686b25 and check whether it meets the requirements.
bd1a59f to
d686b25
Compare
| pmull_ghash_update(1, dg, src, &state->ghash_key, NULL); | ||
| ce_aes_ctr_encrypt(dst, src, (const uint8_t *)ek->data, | ||
| ek->rounds, 1, (uint8_t *)state->ctr, 1); | ||
| /* Copy from untrusted memory 'src' into a temporary buffer |
There was a problem hiding this comment.
/*
* Please format multi-
* line comments like
* this.
*/
| const uint8_t *src, size_t num_blocks, uint8_t *dst) | ||
| { | ||
| void *buf_cryp = state->buf_cryp; | ||
| uint8_t src_tmp[TEE_AES_BLOCK_SIZE]; |
There was a problem hiding this comment.
Coding guidelines (https://optee.readthedocs.io/en/latest/general/coding_standards.html) require this to be initialized.
d686b25 to
614221d
Compare
|
Looks good, please apply: |
AES-GCM (per NIST SP 800-38D) requires the Inc32 operation for the counter, meaning only the least significant 32 bits are incremented modulo 2^32, and the increment must not carry over into the upper 96 bits. However, ce_aes_ctr_encrypt() performs a full 128-bit increment, making it unsuitable for AES-GCM. Remove ce_aes_ctr_encrypt() and replace its usage with a compliant counter increment implementation. Signed-off-by: Mingyen Hung <mingyen.hung@amlogic.com> Reviewed-by: Jens Wiklander <jenswi@kernel.org>
614221d to
c93b585
Compare
|
I'm waiting for CI tests to pass before merging this. |
2 participants
AES-GCM (as per NIST SP 800-38D) specifically requires an Inc32 operation. This means only the least significant 32 bits are incremented modulo 2^32, and the increment must not carry over into the upper 96 bits.
Introduce ce_aes_gcm_ctr_encrypt() to implement this behavior instead of using the generic ce_aes_ctr_encrypt().
Add a test case for ARM64 to reproduce this issue
OP-TEE/optee_test#817