Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port the cryptocell 310 cmac driver #10947

Merged
merged 7 commits into from
Jul 29, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
294 changes: 294 additions & 0 deletions features/cryptocell/FEATURE_CRYPTOCELL310/cmac_alt.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,294 @@
/*
* cmac_alt.c
*
* Copyright (C) 2019, Arm Limited, All Rights Reserved
* SPDX-License-Identifier: Apache-2.0
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
*/

#include "mbedtls/cmac.h"
#if defined(MBEDTLS_CMAC_ALT)
#include "mbedtls/platform.h"
#include "mbedtls/platform_util.h"
#if defined(MBEDTLS_AES_C)
#include "mbedtls/aes.h"
#endif
#include "ssi_aes_defs.h"
#include <string.h>

static int init_cc( mbedtls_cmac_context_t *cmac_ctx )
{
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 )
{
return( MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED );
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


CC_KeyData.pKey = cmac_ctx->CC_Key;
CC_KeyData.keySize = cmac_ctx->CC_keySizeInBytes;

if( SaSi_AesSetKey( &cmac_ctx->CC_Context, SASI_AES_USER_KEY,
&CC_KeyData, sizeof( CC_KeyData ) ) != 0 )
{
ret = MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED;
goto exit;
}

cmac_ctx->is_cc_initiated = 1;

exit:
return( ret );
}

static int deinit_cc( mbedtls_cmac_context_t *cmac_ctx )
{
if( cmac_ctx->is_cc_initiated == 1 &&
SaSi_AesFree( &cmac_ctx->CC_Context ) != 0 )
return( MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED );

return( 0 );
}

int mbedtls_cipher_cmac_starts( mbedtls_cipher_context_t *ctx,
const unsigned char *key, size_t keybits )
{
mbedtls_cmac_context_t *cmac_ctx;
mbedtls_cipher_type_t type;

if( ctx == NULL || ctx->cipher_info == NULL || key == NULL )
return( MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA );

type = ctx->cipher_info->type;

switch( type )
{
case MBEDTLS_CIPHER_AES_128_ECB:
break;
case MBEDTLS_CIPHER_AES_192_ECB:
case MBEDTLS_CIPHER_AES_256_ECB:
case MBEDTLS_CIPHER_DES_EDE3_ECB:
return( MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED );
default:
return( MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA );
}


switch( keybits )
{
case 128:
/* Allocated and initialise in the cipher context memory for the CMAC
* context
*/
cmac_ctx = mbedtls_calloc( 1, sizeof( mbedtls_cmac_context_t ) );
if( cmac_ctx == NULL )
return( MBEDTLS_ERR_CIPHER_ALLOC_FAILED );
cmac_ctx->CC_keySizeInBytes = ( keybits / 8 );
memcpy( cmac_ctx->CC_Key, key, cmac_ctx->CC_keySizeInBytes );
break;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggest removing the {} or putting break inside the {}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would advocate for removing all the brackets as the cases in this switch statement are very simple.

case 192:
case 256:
return( MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED );
default:
return( MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA );
}

ctx->cmac_ctx = cmac_ctx;
return( init_cc( cmac_ctx ) );
}

int mbedtls_cipher_cmac_update( mbedtls_cipher_context_t *ctx,
const unsigned char *input, size_t ilen )
{
mbedtls_cmac_context_t *cmac_ctx;
int ret = 0;
size_t block_size;

if( ctx == NULL || ctx->cipher_info == NULL || input == NULL ||
ctx->cmac_ctx == NULL )
return( MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA );

if( ctx == NULL || ctx->cipher_info == NULL )
return( MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA );

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove extra newline

block_size = ctx->cipher_info->block_size;
if( block_size != SASI_AES_BLOCK_SIZE_IN_BYTES )
return( MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA );

cmac_ctx = ctx->cmac_ctx;

/* Is there data still to process from the last call?
*/
if( cmac_ctx->unprocessed_len > 0 )
{
const size_t size_to_copy = ilen > ( block_size - cmac_ctx->unprocessed_len ) ?
block_size - cmac_ctx->unprocessed_len : ilen;
memcpy( &cmac_ctx->unprocessed_block[cmac_ctx->unprocessed_len],
input, size_to_copy );
cmac_ctx->unprocessed_len += size_to_copy;
input += size_to_copy;
ilen -= size_to_copy;

/*
* Process the unproccessed data, in case it reached a full AES block,
* and there is still input data.
*/
if( cmac_ctx->unprocessed_len == SASI_AES_BLOCK_SIZE_IN_BYTES && ilen > 0 )
{
if( SaSi_AesBlock( &cmac_ctx->CC_Context, cmac_ctx->unprocessed_block,
SASI_AES_BLOCK_SIZE_IN_BYTES, NULL ) != 0 )
{
ret = MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED;
goto exit;
}
cmac_ctx->unprocessed_len = 0;
}
}

if( ilen > 0 )

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the assumption for the code in this if statement is that there are is no data in unprocessed_block if ilen == 0. Could we write that down in a comment? I just think that the precondition is not obvious.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this is not correct.
It is possible to have ilen == 0 and data in unprocessed_block. This is in case the function is given an empty in buffer. However, in this case, if unprocessed_block is not full, we don't do anything with it, until mbedtls_cipher_cmac_finish() is called.

In addition, it is possible that a non empty buffer is given as input, but it is very short ( e.g. 1 byte ), and the `unprocessed_block has not reached 16 bytes after storing the byte.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, my comment was incorrect, I meant to say that the code in the if statement is executed only when unprocessed_block doesnt have any data.

{
const size_t size_to_store = ( ilen % SASI_AES_BLOCK_SIZE_IN_BYTES == 0 ) ?
SASI_AES_BLOCK_SIZE_IN_BYTES : ilen % SASI_AES_BLOCK_SIZE_IN_BYTES;
memcpy( cmac_ctx->unprocessed_block,
input + ilen - size_to_store,
size_to_store );
cmac_ctx->unprocessed_len = size_to_store;
ilen -= size_to_store;
if( ilen > 0 )
{
if( SaSi_AesBlock( &cmac_ctx->CC_Context, (uint8_t *)input,
ilen, NULL ) != 0 )
{
ret = MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED;
goto exit;
}
}
}

exit:
if( ret != 0 )
{
deinit_cc( cmac_ctx );
mbedtls_platform_zeroize( cmac_ctx, sizeof( *cmac_ctx ) );
mbedtls_free( cmac_ctx );
}
return( ret );
}

int mbedtls_cipher_cmac_finish( mbedtls_cipher_context_t *ctx,
unsigned char *output )
{
mbedtls_cmac_context_t *cmac_ctx;
int ret = 0;
size_t olen = SASI_AES_BLOCK_SIZE_IN_BYTES;

if( ctx == NULL || ctx->cipher_info == NULL ||
ctx->cmac_ctx == NULL || output == NULL )
return( MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA );

cmac_ctx = ctx->cmac_ctx;

if( ( ret = SaSi_AesFinish( &cmac_ctx->CC_Context, cmac_ctx->unprocessed_len,
cmac_ctx->unprocessed_block,
cmac_ctx->unprocessed_len, output, &olen ) ) != 0 )
{
ret = MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED;
goto exit;
}

exit:
if( deinit_cc( cmac_ctx ) && ret == 0 )
{
ret = MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED;
}

return( ret );
}

int mbedtls_cipher_cmac_reset( mbedtls_cipher_context_t *ctx )

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is the sequence of events supposed to work if the user wants to reset the cmac? Is it something like:

starts
update
update 
...
finish
reset
starts
update
update
...

Or is it something like:

starts
update
update 
...
finish
reset
update
update
...

Does the second one even make sense? How would the user change the key (for example)? Also there would probably be a memory leak in that case because the cmac context will be reallocated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to documentation for mbedtls_cmac_reset():


 *                      It is called after mbedtls_cipher_cmac_finish()
 *                      and before mbedtls_cipher_cmac_update().

mbedtls_cipher_cmac_finish()`:

 *                      It is called after mbedtls_cipher_cmac_update().
 *                      It can be followed by mbedtls_cipher_cmac_reset() and
 *                      mbedtls_cipher_cmac_update(), or mbedtls_cipher_free().

mbedtls_cipher_cmac_update():

*                      It is called between mbedtls_cipher_cmac_starts() or
 *                      mbedtls_cipher_cmac_reset(), and mbedtls_cipher_cmac_finish().
 *                      Can be called repeatedly.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other words, we do not need to call starts after reset.

{
mbedtls_cmac_context_t *cmac_ctx;

if( ctx == NULL || ctx->cipher_info == NULL || ctx->cmac_ctx == NULL )
return( MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA );

cmac_ctx = ctx->cmac_ctx;

/* Reset the internal state */
cmac_ctx->unprocessed_len = 0;
mbedtls_platform_zeroize( cmac_ctx->unprocessed_block,
sizeof( cmac_ctx->unprocessed_block ) );

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why doesnt this function need to reset the accelerator and clear the initialized flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because:

 * \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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

if( deinit_cc( cmac_ctx ) != 0 )
return( MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED );

return( init_cc( cmac_ctx ) );
}

int mbedtls_cipher_cmac( const mbedtls_cipher_info_t *cipher_info,
const unsigned char *key, size_t keylen,
const unsigned char *input, size_t ilen,
unsigned char *output )
{
int ret = 0;
mbedtls_cipher_context_t ctx;
size_t olen = SASI_AES_BLOCK_SIZE_IN_BYTES;

if( cipher_info == NULL || key == NULL ||
input == NULL || output == NULL )
return( MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA );

mbedtls_cipher_init( &ctx );

if( ( ret = mbedtls_cipher_setup( &ctx, cipher_info ) ) != 0 )
goto exit;

ret = mbedtls_cipher_cmac_starts( &ctx, key, keylen );
if( ret != 0 )
goto exit;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove extra newline.

if( SaSi_AesFinish( &ctx.cmac_ctx->CC_Context, ilen, ( uint8_t * ) input,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

ilen, output, &olen ) != 0 )
{
ret = MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED;
goto clear_cc;
}

clear_cc:
if( deinit_cc( ctx.cmac_ctx ) != 0 && ret == 0 )
{
ret = MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED;
}

exit:
mbedtls_cipher_free( &ctx );

return( ret );

}

#if defined(MBEDTLS_AES_C)

int mbedtls_aes_cmac_prf_128( const unsigned char *key, size_t key_len,
const unsigned char *input, size_t in_len,
unsigned char output[16] )
{
return( MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED );
}
#endif /* MBEDTLS_AES_C */


#endif /* MBEDTLS_CMAC_ALT */
50 changes: 50 additions & 0 deletions features/cryptocell/FEATURE_CRYPTOCELL310/cmac_alt.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/*
* cmac_alt.h
*
* Copyright (C) 2019, ARM Limited, All Rights Reserved
* SPDX-License-Identifier: Apache-2.0
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
*/

#ifndef __CMAC_ALT__
#define __CMAC_ALT__

#if defined(MBEDTLS_CMAC_ALT)
#include "ssi_aes.h"
#ifdef __cplusplus
extern "C" {
#endif

typedef struct mbedtls_cmac_context_t
{
SaSiAesUserContext_t CC_Context;
uint8_t CC_Key[SASI_AES_KEY_MAX_SIZE_IN_BYTES];
size_t CC_keySizeInBytes;
/** Unprocessed data - either data that was not block aligned and is still
* pending processing, or the final block */
unsigned char unprocessed_block[SASI_AES_BLOCK_SIZE_IN_BYTES];

/** The length of data pending processing. */
size_t unprocessed_len;

int is_cc_initiated;
} mbedtls_cmac_context_t;

#ifdef __cplusplus
}
#endif

#endif /* MBEDTLS_CMAC_ALT */
#endif /* __CMAC_ALT__ */
3 changes: 2 additions & 1 deletion features/cryptocell/FEATURE_CRYPTOCELL310/mbedtls_device.h
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/*
* mbedtls_device.h
*
* Copyright (C) 2018, Arm Limited, All Rights Reserved
* Copyright (C) 2018-2019, Arm Limited, All Rights Reserved
* SPDX-License-Identifier: Apache-2.0
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may
Expand All @@ -25,6 +25,7 @@
#define MBEDTLS_SHA1_ALT
#define MBEDTLS_SHA256_ALT
#define MBEDTLS_CCM_ALT
//#define MBEDTLS_CMAC_ALT
#define MBEDTLS_ECDSA_VERIFY_ALT
#define MBEDTLS_ECDSA_SIGN_ALT
#define MBEDTLS_ECDSA_GENKEY_ALT
Expand Down