nrf_security: drivers: cracen: make single-part AEAD operations single-part #20827
Conversation
github-actions
bot
added
the
changelog-entry-required
CI InformationTo view the history of this post, clich the 'edited' button above Inputs:Sources:sdk-nrf: PR head: 080ecefe3e56df73ea57de4b3b43661808afada9 more detailssdk-nrf:
Github labels
List of changed files detected by CI (2)Outputs:ToolchainVersion: 4ffa2202d5 Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
tomi-font
force-pushed
the
security_cracen_single-part_AEAD
branch
2 times, most recently
from
e655e6b to
f7180a3
Compare
tomi-font
removed
the
changelog-entry-required
| @@ -57,8 +55,10 @@ static bool is_nonce_length_supported(psa_algorithm_t alg, size_t nonce_length) | |||
| case PSA_ALG_GCM: | |||
| case PSA_ALG_CHACHA20_POLY1305: | |||
| return nonce_length == 12u; | |||
| #ifdef PSA_NEED_CRACEN_CCM_AES | |||
There was a problem hiding this comment.
Since you did that for CCM in this function I would say it makes sense to do it for the other algorithms as well. Just for the consistency.
| @@ -195,12 +195,12 @@ static void cracen_writebe(uint8_t *out, uint64_t data, uint16_t targetsz) | |||
| } | |||
| } | |||
|
|
|||
| static psa_status_t create_aead_ccmheader(cracen_aead_operation_t *operation) | |||
| static void create_aead_ccmheader(cracen_aead_operation_t *operation, | |||
| uint8_t header[static CCM_HEADER_MAX_LENGTH], | |||
There was a problem hiding this comment.
Is the static keyword inside the array argument valid C? Whats up with this?
There was a problem hiding this comment.
There was a problem hiding this comment.
Cool, I was not aware of this, thanks!
| /* Create the CCM header */ | ||
| if (operation->alg == PSA_ALG_CCM) { | ||
| return create_aead_ccmheader(operation); | ||
| if (IS_ENABLED(PSA_NEED_CRACEN_CCM_AES) && operation->alg == PSA_ALG_CCM) { |
There was a problem hiding this comment.
If the CCM is not enabled but the operation set the CCM algorithm it will return success with this logic. Please update.
There was a problem hiding this comment.
No it won't because set_nonce() is called before, which calls is_nonce_length_supported(), which does the checking.
There was a problem hiding this comment.
You are right on this, I missed that part. You need to go two functions down and check an ifdef to realize that this is not possible though. So if you keep the logic please explain what you are doing there.
| /* Data fed to CRACEN needs to remain untouched until it's been consumed | ||
| * (sx_aead_wait()), so don't put the CCM header buffer on the stack. | ||
| */ | ||
| static uint8_t ccm_header_aad[ROUND_UP(CCM_HEADER_MAX_LENGTH, |
There was a problem hiding this comment.
This is not thread safe I think. How would this work with multiple threads?
There was a problem hiding this comment.
Indeed it's not thread-safe but I've been told that there is some synchronization mechanism at a higher level which makes it so that only a single thread can be here at the same time?
| if (*ciphertext_length + tag_length > ciphertext_size) { | ||
| *ciphertext_length = 0; | ||
| status = PSA_ERROR_BUFFER_TOO_SMALL; | ||
| status = cracen_aead_finish(&operation, NULL, 0, NULL, |
There was a problem hiding this comment.
It is a bit strange and error prone that you are using a function which may resume the context depending on the operation state. If you insist to do that please add a comment to explain here.
There was a problem hiding this comment.
I see your point. My goal with that was to use already existing functions where possible to avoid code duplication in order to minimize code size increases. No strong opinion on that. I'll rework how it's done.
| if (status != PSA_SUCCESS) { | ||
| *ciphertext_length = 0; |
There was a problem hiding this comment.
Why did you remove that? This will for sure cause issues since the caller will have no way to distinguish what happened with the ciphertext buffer.
Please re add this in all cases.
There was a problem hiding this comment.
I removed that because with my changes ciphertext_length is not touched anymore unless the function returns PSA_SUCCESS. Previously it could be set by cracen_aead_update() and so in case of error we'd erase that value by setting it to 0.
See https://arm-software.github.io/psa-api/crypto/1.2/api/ops/aead.html#c.psa_aead_encrypt:
ciphertext_length
On success, the size of the output in the ciphertext buffer.
There was a problem hiding this comment.
I see your point but I see that in the decrypt function we set the plaintext length as 0. And I still believe that we don't break the spec if we set that to 0 as well. So I would still suggest to do that.
tomi-font
force-pushed
the
security_cracen_single-part_AEAD
branch
from
f7180a3 to
0d87b24
Compare
github-actions
bot
added
the
changelog-entry-required
…e-part Make the single-part AEAD operations not use HW context switching (save/resume). As a bonus, make algorithm-specific code (especially for CCM) depend on its respective `PSA_NEED_CRACEN` so that it gets compiled out if the algorithm is disabled. ref: NCSDK-31362 Signed-off-by: Tomi Fontanilles <tomi.fontanilles@nordicsemi.no>
tomi-font
force-pushed
the
security_cracen_single-part_AEAD
branch
from
0d87b24 to
080ecef
Compare
|
tomi-font
removed
the
changelog-entry-required
Make the single-part AEAD operations not use HW context switching (save/resume).
As a bonus, make algorithm-specific code (especially for CCM) depend on its respective
PSA_NEED_CRACENso that it gets compiled out if the algorithm is disabled.ref: NCSDK-31362