Port the cryptocell 310 cmac driver #10947
Conversation
Add support for CC310 CMAC driver returning `MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED` for key size other than 128 bits, and for crypto algorithms other than AES( e.g. DES).
| cmac_ctx->CC_keySizeInBytes = ( keybits / 8 ); | ||
| memcpy( cmac_ctx->CC_Key, key, cmac_ctx->CC_keySizeInBytes ); | ||
| } | ||
| break; |
There was a problem hiding this comment.
suggest removing the {} or putting break inside the {}
There was a problem hiding this comment.
I would advocate for removing all the brackets as the cases in this switch statement are very simple.
| @@ -0,0 +1,50 @@ | |||
| /* | |||
| * aes_alt.h | |||
There was a problem hiding this comment.
This should say cmac_alt.h here
There was a problem hiding this comment.
yes, a copy\paste error
| goto exit; | ||
| } | ||
|
|
||
| if( SaSi_AesFinish( &ctx.cmac_ctx->CC_Context, ilen, ( uint8_t * ) input, |
There was a problem hiding this comment.
Why not call mbedtls_cipher_cmac_update() and `mbedtls_cipher_cmac_finish()?
If AES_ALT is set, won't the software CMAC implementation use it?
There was a problem hiding this comment.
It can be done, but it adds additional redundant code. In this API, it's a one call CMAC functionality, without storing unprocessed_block, and call directly to the CC AES driver to calculate the CMAC.
The alternative implementation of mbedtls_cipher_cmac_update() and mbedtls_cipher_cmac_finish() has support for the unprocessed data, to support sequential calls with sizes that are not multiple of AES block size
|
@RonEld, thank you for your changes. |
There was a problem hiding this comment.
@RonEld: I did an initial review of the changes and wrote down a number of comments/questions. Please let me know what you think
| */ | ||
| cmac_ctx = mbedtls_calloc( 1, sizeof( mbedtls_cmac_context_t ) ); | ||
| if( cmac_ctx == NULL ) | ||
| return( MBEDTLS_ERR_CIPHER_ALLOC_FAILED ); |
There was a problem hiding this comment.
Is it necessary to allocate this before making the remaining keybits check? Perhaps it would be more optimal to move the allocation into the case 128: or move the switch check before this allocation.
| } | ||
|
|
||
| ctx->cmac_ctx = cmac_ctx; | ||
| return 0; |
There was a problem hiding this comment.
Please add brackets around the argument to return
| cmac_ctx->CC_keySizeInBytes = ( keybits / 8 ); | ||
| memcpy( cmac_ctx->CC_Key, key, cmac_ctx->CC_keySizeInBytes ); | ||
| } | ||
| break; |
There was a problem hiding this comment.
I would advocate for removing all the brackets as the cases in this switch statement are very simple.
| int ret = 0; | ||
| SaSiAesUserKeyData_t CC_KeyData; | ||
| if( SaSi_AesInit( &cmac_ctx->CC_Context, SASI_AES_ENCRYPT, | ||
| SASI_AES_MODE_CMAC, SASI_AES_PADDING_NONE) != 0 ) |
| } | ||
|
|
||
| CC_KeyData.pKey = cmac_ctx->CC_Key; | ||
| CC_KeyData.keySize = cmac_ctx->CC_keySizeInBytes; |
There was a problem hiding this comment.
If I understand correctly, this is just a temporary struct with a little bit of metadata a pointer to the key. Why not just make that struct part of the mbedtls_cmac_context_t? Does SaSiAesUserKeyData_t contain a lot more stuff that we do not want?
There was a problem hiding this comment.
This struct only holds the pointer to the key and the key size. It is used being set in one CC function so I don't see an added value of putting it as part of the context structure.
In addition, if you have this structure as part of the context, which will only point to data being given by user, it might point to data that doesn't exist anymore
| * \return \c 0 on success. | ||
| * \return #MBEDTLS_ERR_MD_BAD_INPUT_DATA | ||
| * if parameter verification fails. | ||
| */ |
There was a problem hiding this comment.
Why do we have this documentation here? Surely its the same as in the header files. But probably nobody is going to see it as its inside this source file.
There was a problem hiding this comment.
A mistake. I will remove
| } | ||
|
|
||
| exit: | ||
| if( SaSi_AesFree( &cmac_ctx->CC_Context ) != 0 && ret == 0 ) |
There was a problem hiding this comment.
Does this function need to be called somewhere else? What happens if I am the application programmer and decide that somewhere in the middle of a computation I no longer want to continue doing CMAC (even though all the calls to starts and update have terminated correctly). How do I release the resources from that CMAC context and the accelerator?
There was a problem hiding this comment.
Since there isn't any terminate function to deallocate all of the cmac resources, I don't see another location
There was a problem hiding this comment.
Well, but how does this API work then? how does the user release the resources from a context? What happens when I decided that I do not want to continue a computation? Who releases the dynamic memory allocated?
There was a problem hiding this comment.
To be honest, I don't have a good answer, but I really don't see another option of where to clear the resources, the way our cmac is implemented.
The cmac context is cleared in the cipher module, which I don't think is correct, but this is the way it behaves.
Anyway, as mentioned in mbedtls_cipher_free , this is not a real memory leak, as the CC function only clears the context,
There was a problem hiding this comment.
To free the resources, reset should be called on the context. SaSi_AesFree should be called as part of mbedtls_cmac_reset()
There was a problem hiding this comment.
But if you look at implementation of mbedtls_cipher_cmac(), you will see that mbedtls_cmac_reset() is not called, causing resources not to be freed
|
|
||
|
|
||
| exit: | ||
| if( SaSi_AesFree( &ctx.cmac_ctx->CC_Context ) != 0 && ret == 0 ) |
There was a problem hiding this comment.
What happens if we fail before calling init_cc? Is that ok/acceptable?
There was a problem hiding this comment.
Nothing will really happen, because SaSi_AesFree() mostly memsets to zero. However I see your point, as this is not correct behavior. i'll fix this
| /* | ||
| * aes_alt.h | ||
| * | ||
| * Copyright (C) 2018, ARM Limited, All Rights Reserved |
There was a problem hiding this comment.
Note that the dates and branding of this license header are out-of-date
There was a problem hiding this comment.
yes, copy\paste error as well
| cmac_ctx->unprocessed_len = 0; | ||
| mbedtls_platform_zeroize( cmac_ctx->unprocessed_block, | ||
| sizeof( cmac_ctx->unprocessed_block ) ); | ||
|
|
There was a problem hiding this comment.
Why doesnt this function need to reset the accelerator and clear the initialized flag.
There was a problem hiding this comment.
* \brief This function prepares the authentication of another
* message with the same key as the previous CMAC
* operation.
Since same key is used, no need to reset the accelerator
Fix minor style fixes and typos: 1. Change file name to correct one. 2. Change copyright year. 3. Insert whitespaces before and after paranthesis. 4. Put `*` next to pointer name.
FIx some functionality issues for better visibility: 1. Allocate the context only for 128 bit key 2. Change oreder of freeing the resources.
Have the alternative cmac undefined by default, in order not to break backwards compatability.
|
I made the alternative cmac optional. I will make a separate PR, updating the readme file |
| mbedtls_platform_zeroize( cmac_ctx->unprocessed_block, | ||
| sizeof( cmac_ctx->unprocessed_block ) ); | ||
|
|
||
| return( 0 ); |
There was a problem hiding this comment.
reset() should also call SaSi_AesFree, to free up the cmac_ctx->CC_Context. I'm not sure if we need to check that CC_Context was initialized (as we initialize it in update()) before we reset. Alternatively, we could consider allocating the CC_Context in starts().
There was a problem hiding this comment.
I thought about allocating in starts(), but then in use cases of starts() ->update()->finish() ->reset() update()->finish() we might fail
There was a problem hiding this comment.
So, both starts and reset would need to allocate a new CC_Context context as well in that flow. But, reset is supposed to free all resources, not make new ones. This means starts is not a good place to allocate resources, agreed.
| goto exit; | ||
| } | ||
|
|
||
| if( SaSi_AesFinish( &ctx.cmac_ctx->CC_Context, ilen, ( uint8_t * ) input, |
| extern "C" { | ||
| #endif | ||
|
|
||
| struct mbedtls_cmac_context_t |
There was a problem hiding this comment.
Do we need to typedef this so it works with C? It looks like perhaps this header was only tested with C++ builds.
There was a problem hiding this comment.
This was tested with our on target tests, which I believe is in c
There was a problem hiding this comment.
Mbed OS always builds with C++, so it's always C++ for on-target testing. We should use typedef here if we want to be C compatible. Otherwise, when you later refer to mbedtls_cmac_context_t blah; instead of struct mbedtls_cmac_context_t blah, you'll get compilation failures. I see many places in this PR where we don't say struct mbedtls_cmac_context_t.
There was a problem hiding this comment.
sure, I'll change but this implementation is for Mbed OS anyway
Initiate the CC context in the starts function and in the reset. In the reset function, free aes context before. Free the context in the finish function and reset function.
Make the cmac context a typedef, to be compatible with c code.
RonEld
force-pushed
the
cc310_cmac_porting
branch
from
f89eaab to
4e29c8f
Compare
|
@Patater I addressed your comments |
|
|
||
| if( ctx == NULL || ctx->cipher_info == NULL ) | ||
| return( MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA ); | ||
|
|
| exit: | ||
| if( ret != 0 ) | ||
| { | ||
| SaSi_AesFree( &cmac_ctx->CC_Context ); |
There was a problem hiding this comment.
Is it always safe to call SaSi_AesFree()? or do we need to check cmac_ctx->is_cc_initiated == 1 like we do in mbedtls_cipher_cmac_finish()? You might want to make deinit_cc() to avoid repeating the same is_cc_initiated check everywhere.
There was a problem hiding this comment.
Is it always safe to call SaSi_AesFree()?
Well, yes, because internally it just memsets the buffer to zero, but It's better to change as you suggested. It's a residue from when we initiated the context also in update
| ret = mbedtls_cipher_cmac_starts( &ctx, key, keylen ); | ||
| if( ret != 0 ) | ||
| goto exit; | ||
|
|
Add a deinit function that will be called and check inside whether context is initialized. This function is called for freeing the CC context, instead of every time check that it's initizliaed and free it.
|
@Patater I have addressed your comments. Please review |
I have run the cmac on target test |
| SASI_AES_MODE_CMAC, SASI_AES_PADDING_NONE ) != 0 ) | ||
| { | ||
| return( MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED ); | ||
| } |
There was a problem hiding this comment.
Not sure why Astyle does not complain, but this is not really Mbed Coding style.
We should follow more K&R style. See https://os.mbed.com/docs/mbed-os/v5.13/contributing/style.html
There was a problem hiding this comment.
This is an alternative implementation for code in Mbed TLS, which has its own style rules. Mbed TLS and alternative implementations for it are deliberately excluded from the astyle check in Mbed OS. I don't know if there's a formal rule stated somewhere, but most alternative implementations follow the Mbed TLS style.
Test run: SUCCESSSummary: 11 of 11 test jobs passed |
SeppoTakalo
added
ready for merge
release-version: 5.14.0-rc1
and removed
needs: CI
labels
Description
Add support for CC310 CMAC driver returning
MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTEDfor key size other than 128 bits,and for crypto algorithms other than AES( e.g. DES).
According to the generated map file, using GCC_ARM:
ROM: 640 bytes smaller
RAM: No Change
Benchmark information, built with GCC_ARM:
Pull request type
Reviewers
@ARMmbed/mbed-os-crypto
Release Notes
This is a target update, adding hw accelerated CMAC, however, it can also be considered as a breaking change, as it removes support for key sizes other than 128 bit keys, and removes support for DES CMAC.