Skip to content

Port aes cc310 driver #10907

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

Merged
merged 4 commits into from
Aug 23, 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
308 changes: 308 additions & 0 deletions features/cryptocell/FEATURE_CRYPTOCELL310/aes_alt.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,308 @@
/*
* aes_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/aes.h"
#if defined(MBEDTLS_AES_ALT)
#include <string.h>
#include "ssi_aes_defs.h"
#include "mbedtls/platform.h"

#if defined(MBEDTLS_CIPHER_MODE_CFB)
/*
* AES-CFB128 buffer encryption/decryption
*/
int mbedtls_aes_crypt_cfb128( mbedtls_aes_context *ctx,
int mode,
size_t length,
size_t *iv_off,
unsigned char iv[16],
const unsigned char *input,
unsigned char *output )
{
return( MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED );
}

/*
* AES-CFB8 buffer encryption/decryption
*/
int mbedtls_aes_crypt_cfb8( mbedtls_aes_context *ctx,
int mode,
size_t length,
unsigned char iv[16],
const unsigned char *input,
unsigned char *output )
{
return( MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED );
}
#endif /*MBEDTLS_CIPHER_MODE_CFB */

#if defined(MBEDTLS_CIPHER_MODE_XTS)

int mbedtls_aes_xts_setkey_enc( mbedtls_aes_xts_context *ctx,
const unsigned char *key,
unsigned int keybits )
{
return( MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED );
}

int mbedtls_aes_xts_setkey_dec( mbedtls_aes_xts_context *ctx,
const unsigned char *key,
unsigned int keybits )
{
return( MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED );
}

int mbedtls_aes_crypt_xts( mbedtls_aes_xts_context *ctx,
int mode,
size_t length,
const unsigned char data_unit[16],
const unsigned char *input,
unsigned char *output )
{
return( MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED );
}
#endif /* MBEDTLS_CIPHER_MODE_XTS */

#if defined(MBEDTLS_CIPHER_MODE_OFB)
int mbedtls_aes_crypt_ofb( mbedtls_aes_context *ctx,
size_t length,
size_t *iv_off,
unsigned char iv[16],
const unsigned char *input,
unsigned char *output );
{
return( MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED );
}
#endif /* MBEDTLS_CIPHER_MODE_OFB */

void mbedtls_aes_init( mbedtls_aes_context *ctx )
{
memset( ctx, 0, sizeof( mbedtls_aes_context ) );
}

void mbedtls_aes_free( mbedtls_aes_context *ctx )
{
if( ctx == NULL )
return;

mbedtls_platform_zeroize( ctx, sizeof( mbedtls_aes_context ) );
}
#if defined(MBEDTLS_CIPHER_MODE_XTS)

void mbedtls_aes_xts_init( mbedtls_aes_xts_context *ctx ){}
Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't define these, if someone attempts to use them, they'll get build failures. If we implement them doing nothing, that's a small waste of code space, but also could hide the unsupported nature until run-time. Maybe worth adding #error AES XTS is not supported with CC310 AES within this #if defined(MBEDTLS_CIPHER_MODE_XTS), instead of making empty functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally, I also prefer catching issues as soon as possible, e.g. at build time. However. this is according to our design, and in this specific case it may cause CI issues, when trying to set different configurations on different platforms

Copy link
Contributor

Choose a reason for hiding this comment

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

OK for now in this PR.

That's a problem in our CI, IMO. We shouldn't count it as a failure, nor attempt such an an invalid configuration, when a certain platform's ALT implementation can't do certain block modes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is why I added the FEATURE UNSUPPORTED PR

Copy link
Contributor

Choose a reason for hiding this comment

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

OK


void mbedtls_aes_xts_free( mbedtls_aes_xts_context *ctx ){}
#endif /* MBEDTLS_CIPHER_MODE_XTS */

static int CC_aes_setkey( mbedtls_aes_context *ctx, const unsigned char *key,
unsigned int keybits, SaSiAesEncryptMode_t cipher_flag )
{
int ret = 0;
if( ctx == NULL )
return( MBEDTLS_ERR_AES_BAD_INPUT_DATA );

switch( keybits )
{
case 128:
{
ctx->CC_cipherFlag = cipher_flag;
ctx->CC_keySizeInBytes = ( keybits / 8 );
memcpy( ctx->CC_Key, key, ctx->CC_keySizeInBytes );
}
break;
case 192:
case 256:
return( MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED );
default:
return( MBEDTLS_ERR_AES_INVALID_KEY_LENGTH );
}

return( 0 );
}
int mbedtls_aes_setkey_enc( mbedtls_aes_context *ctx, const unsigned char *key,
unsigned int keybits )
{
return( CC_aes_setkey( ctx, key, keybits, SASI_AES_ENCRYPT ) );
}

int mbedtls_aes_setkey_dec( mbedtls_aes_context *ctx, const unsigned char *key,
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks identical to mbedtls_aes_setkey_enc() other than one flag. Could we implement these functions by returning the result of a new function return mbedtls_aes_setkey(SASI_AES_DECRYPT)? It should save a bit of flash space, and be more readable.

unsigned int keybits )
{
return( CC_aes_setkey( ctx, key, keybits, SASI_AES_DECRYPT ) );
}

static int CC_aes_cipher( mbedtls_aes_context *ctx,
int mode,
SaSiAesOperationMode_t aes_mode,
size_t length,
unsigned char* iv,
size_t iv_len,
const unsigned char *input,
unsigned char *output )
{
int ret = 0;
SaSiAesUserKeyData_t CC_KeyData = { ctx->CC_Key,
ctx->CC_keySizeInBytes };

ret = SaSi_AesInit( &ctx->CC_Context,
ctx->CC_cipherFlag,
aes_mode, SASI_AES_PADDING_NONE );
if( ret != 0 )
return( MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED );

ret = SaSi_AesSetKey( &ctx->CC_Context, SASI_AES_USER_KEY,
&CC_KeyData, sizeof( CC_KeyData ) );
if( ret != 0 )
return( MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED );

if( iv )
{
if( iv_len != SASI_AES_IV_SIZE_IN_BYTES )
return( MBEDTLS_ERR_AES_INVALID_INPUT_LENGTH );

ret = SaSi_AesSetIv( &ctx->CC_Context, iv );
if( ret != 0 )
return( MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED );
}

ret = SaSi_AesFinish( &ctx->CC_Context, length,
( unsigned char* )input, length, output, &length );
if( ret != 0 )
return( MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED );

/* update the IV for next block
* For CTR mode, update the nonce only if the current length is a full AES block length
*/

if( ( ( aes_mode == SASI_AES_MODE_CBC ) ||
( (aes_mode == SASI_AES_MODE_CTR) && ( ( length % SASI_AES_BLOCK_SIZE_IN_BYTES) == 0) ) )
&& iv )
{
ret = SaSi_AesGetIv( &ctx->CC_Context, iv );
if( ret != 0 )
return( MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED );
}

ret = SaSi_AesFree( &ctx->CC_Context );
if( ret != 0 )
return( MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED );

return( 0 );
}

int mbedtls_aes_crypt_ecb( mbedtls_aes_context *ctx,
int mode,
const unsigned char input[16],
unsigned char output[16] )
{
if( ctx == NULL )
return( MBEDTLS_ERR_AES_BAD_INPUT_DATA );

if( ( mode == MBEDTLS_AES_ENCRYPT && ctx->CC_cipherFlag != SASI_AES_ENCRYPT ) ||
( mode == MBEDTLS_AES_DECRYPT && ctx->CC_cipherFlag != SASI_AES_DECRYPT ) )
return( MBEDTLS_ERR_AES_BAD_INPUT_DATA );

return( CC_aes_cipher( ctx, mode, SASI_AES_MODE_ECB, 16, NULL, 0, input, output ) );
}

#if defined(MBEDTLS_CIPHER_MODE_CBC)
int mbedtls_aes_crypt_cbc( mbedtls_aes_context *ctx,
int mode,
size_t length,
unsigned char iv[16],
const unsigned char *input,
unsigned char *output )
{
if( ctx == NULL )
return( MBEDTLS_ERR_AES_BAD_INPUT_DATA );

if( length % SASI_AES_BLOCK_SIZE_IN_BYTES )
return( MBEDTLS_ERR_AES_INVALID_INPUT_LENGTH );

if( ( mode != MBEDTLS_AES_ENCRYPT || ctx->CC_cipherFlag != SASI_AES_ENCRYPT ) &&
( mode != MBEDTLS_AES_DECRYPT || ctx->CC_cipherFlag != SASI_AES_DECRYPT ) )
return( MBEDTLS_ERR_AES_BAD_INPUT_DATA );

return( CC_aes_cipher( ctx, mode, SASI_AES_MODE_CBC, length, iv, 16, input, output ) );
}

#endif /* MBEDTLS_CIPHER_MODE_CBC */

#if defined(MBEDTLS_CIPHER_MODE_CTR)
int mbedtls_aes_crypt_ctr( mbedtls_aes_context *ctx,
size_t length,
size_t *nc_off,
unsigned char nonce_counter[16],
unsigned char stream_block[16],
const unsigned char *input,
unsigned char *output )
{
int ret = 0;
int n = *nc_off, c, i;
size_t j;
if( ctx == NULL )
return( MBEDTLS_ERR_AES_BAD_INPUT_DATA );

if( *nc_off )
{
/* handle corner case where we are resuming a previous encryption,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does CC_aes_cipher not handle this corner case itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no. CC CTR implementation only supports where *nc_off is zero

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if it isn't? Incorrect computation? How do users of the CC API cope with this limitation ordinarily?

Copy link
Contributor Author

@RonEld RonEld Jul 1, 2019

Choose a reason for hiding this comment

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

yes, incorrect computation. Users of CC, that are interested in *nc_off != 0 will need to do the same thing

* and we are resuming within current cipher stream(stream_block) */
while( n != 0 )
{
c = *input++;
*output++ = (unsigned char)( c ^ stream_block[n] );
n = ( n + 1) & 0x0F;
if( length > 0)
--length;
}
/*
* Increase the nonce_counter by 1 since we now passed one block
*/
for( i = 16; i > 0; i-- )
if( ++nonce_counter[i - 1] != 0 )
break;
}
if( CC_aes_cipher( ctx, MBEDTLS_AES_ENCRYPT, SASI_AES_MODE_CTR,
length, nonce_counter, SASI_AES_IV_SIZE_IN_BYTES, input, output ) != 0 )
{
ret = -1;
}
if( ( ( length % SASI_AES_BLOCK_SIZE_IN_BYTES ) != 0 ) && ret == 0 )
{
/* in case the length is not aligned, generate stream block for resuming
* increase nonce_block to the correct value*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we fail with hardware unsupported or invalid parameter when alignment is not correct?

If not, maybe it's smaller in code size to copy the block to an aligned location, perform the hardware operation, and then copy back.

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's bad wording. It's not a matter of memory alignment. It's a matter of length which is a multiple of AES block size (16). This functionality generates the stream block for resuming on next call( if there is), for generating the first block on next block ( since *nc_off is not zero in this case)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that returning unsupported feature in this case will result in a major functionality decrease, on the expense of minor flash set back, IMHO

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the use case exactly? Why this is a major functionality decrease? Don't users usually use AES with a length that's a multiple of the block length?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not for CTR.
CTR is streaming mode, which you can send any length of data, and pause the streaming in the middle, resume the streaming from same location, or any other arbitrary location

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems odd that CC only lets you do CTR in multiples of the block length. Is that really the case? Doesn't that sort of defeat the purpose of streaming?

Choose a reason for hiding this comment

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

@Patater CTR mode is a stream cipher based on top of a block operation. From the user's perspective, partial inputs can have any byte length (even any bit length in theory). But under the hood, the operation involving the key works on 128 bits at a time (for a cipher with 128-bit blocks). If a call completes with a total input size that isn't a multiple of the block size, something in the stack has to remember the partial state. The hardware can't do this by itself (it would need to store the state of all ongoing calls). Either the hardware stores the partial state in an opaque form that the driver just keeps in memory, or the hardware works on 128 bits at a time and it's up to the driver to compensate. Usually, it's the latter: if the “hardware” is doing it, it means that some software layer deeper down is doing 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.

@Patater In addition to what was mentioned by @gilles-peskine-arm CC supports any byte length as input, but it doesn't support byte offset.
As you can see from previous call to CC driver(line 284), the full length ( after reducing nc_off ), no matter what the length is. However, if the length is not a multiple of 16, then we need to calculate stream_block for next API call.

for( j = 0; j < ( length/SASI_AES_BLOCK_SIZE_IN_BYTES ); j++)
for( i = 16; i > 0; i-- )
if( ++nonce_counter[i - 1] != 0 )
break;
if( ( ret = CC_aes_cipher( ctx, MBEDTLS_AES_ENCRYPT, SASI_AES_MODE_ECB,
SASI_AES_BLOCK_SIZE_IN_BYTES, NULL, 0,
nonce_counter, stream_block ) ) != 0 )
{
goto exit;
}
}
*nc_off = ( length % SASI_AES_BLOCK_SIZE_IN_BYTES );

exit:
return( ret );
}
#endif /* MBEDTLS_CIPHER_MODE_CTR */
#endif/* MBEDTLS_AES_ALT */
56 changes: 56 additions & 0 deletions features/cryptocell/FEATURE_CRYPTOCELL310/aes_alt.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/*
* aes_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 __AES_ALT__
#define __AES_ALT__

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

typedef struct
{
SaSiAesUserContext_t CC_Context;
SaSiAesEncryptMode_t CC_cipherFlag;
uint8_t CC_Key[SASI_AES_KEY_MAX_SIZE_IN_BYTES];
size_t CC_keySizeInBytes;
}
mbedtls_aes_context;

#if defined(MBEDTLS_CIPHER_MODE_XTS)
/**
* \brief The AES XTS context-type definition.
*/
typedef struct mbedtls_aes_xts_context
{
int unsupported;
}
mbedtls_aes_xts_context;
#endif /* MBEDTLS_CIPHER_MODE_XTS */

#ifdef __cplusplus
}
#endif

#endif /* MBEDTLS_AES_ALT */
#endif /* __AES_ALT__ */

4 changes: 3 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 @@ -22,6 +22,8 @@
#define __MBEDTLS_DEVICE__

#define MBEDTLS_PLATFORM_SETUP_TEARDOWN_ALT
//#define MBEDTLS_AES_ALT
#define MBEDTLS_CTR_DRBG_USE_128_BIT_KEY
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we want to degrade the security of devices by default. Consider wrapping this setting in a ifdef, with a comment explaining why it's needed.

#if defined(MBEDTLS_AES_ALT)
#define MBEDTLS_CTR_DRBG_USE_128_BIT_KEY
#endif

Choose a reason for hiding this comment

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

AES-128 is acceptable as default security as long as there are less than about one quintillion devices (128-bit security, divide by two for the birthday bound) under ideal conditions (if there are side channels, it can be another matter, but more bits often doesn't help anyway). AES-256 gives a better security margin, but there's no reason to require 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.

I agree, we don't want to degrade security, and perhaps as long as AES_ALT is not defined we shouldn't , however I decide to add it in order to have as less configurability as possible, for readability reasons. In addition, as mentioned by Gilles, the degrade doesn't have much effect.

#define MBEDTLS_SHA1_ALT
#define MBEDTLS_SHA256_ALT
#define MBEDTLS_CCM_ALT
Expand Down