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

Fix use of AES_ALT on STM32F439 for example-tls-client #5018

Merged
merged 4 commits into from Sep 20, 2017

Conversation

Projects
None yet
@adustm
Member

adustm commented Sep 5, 2017

Description

tls-client example is calling aes_encrypt with 4 separated instances (ctx), in ECB mode.
There is a moment where we have :

call aes_set_key(ctx3)   <-- here the key of ctx3 is copied in ctx3->key
call aes_encrypt(ctx3)   <-- here ctx3->key is copied in the hardware registers
call aes_set_key(ctx4)   <-- here the key of ctx4 is copied in ctx4->key
call aes_encrypt(ctx4)   <-- here ctx4->key is copied in the hardware registers
call aes_encrypt(ctx3)   <-- PROBLEM: hardware registers still contain the key of ctx4
call aes_encrypt(ctx3)...

I have added
ctx->hcryp_aes.Phase = HAL_CRYP_PHASE_READY;
in aesecb_encrypt function so that the firmware manages all the time the copy of ctx->key in hardware registers

I have removed the workaround implemented by @Patater in #4934

Status

READY
Resolves #4928

Steps to test or reproduce

mbed import http://mbed.org/teams/mbed-os-examples/code/mbed-os-example-tls-tls-client
cd mbed-os-example-tls-tls-client
mbed compile -c -m NUCLEO_F439ZI -t GCC_ARM

```Connect the NUCLEO_F439ZI to the computer and plug ethernet wire to a network
Open an hyperterminal
copy ./BUILD/NUCLEO_F439ZI/GCC_ARM/mbed-os-example-tls-tls-client.bin on the platform

Without the fix, you will have :

ssl_tls.c:3961: |2| got an alert message, type: [2:20]
ssl_tls.c:3969: |1| is a fatal alert message (msg 20)
ssl_tls.c:3744: |1| mbedtls_ssl_handle_message_type() returned -30592 (-0x7780)
ssl_cli.c:3184: |1| mbedtls_ssl_read_record() returned -30592 (-0x7780)
ssl_tls.c:6354: |2| <= handshake
mbedtls_ssl_handshake() failed: -0x7780 (-30592): SSL - A fatal alert message was received from our peer

with the fix you will have :

Starting the TLS handshake...
TLS connection to developer.mbed.org established
... 
Hello world!

cc @andresag01 @RonEld @Patater @JanneKiiskila @sg- @RobMeades

@adustm

This comment has been minimized.

Member

adustm commented Sep 5, 2017

FYI, I've launched every existing mbed-os-mbedtls tests :
mbed-os/TESTS/mbedtls
mbed-os-example-tls-tls-client
mbed-os-example-tls-authcrypt
mbed-os-example-tls-hashing
mbed-os-example-tls-benchmark

All successful

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 5, 2017

Can you please remove that merge commit and rebase instead ?

@JanneKiiskila

This comment has been minimized.

Contributor

JanneKiiskila commented Sep 5, 2017

Super!

@RonEld

I will approve this after I get explanation on why HAL_CRYP_PHASE_READY is added and what does it mean? If it's important, why wasn't it added to mbedtls_aes_crypt_ecb

@adustm adustm force-pushed the adustm:f439_aes_tlsclient_fix branch Sep 5, 2017

@adustm

This comment has been minimized.

Member

adustm commented Sep 5, 2017

@0xc0170 I have fixed the rebase

@RonEld

I will approve this after I get explanation on why HAL_CRYP_PHASE_READY is added and what does it mean? If it's important, why wasn't it added to mbedtls_aes_crypt_ecb

mbedtls_aes_crypt_ecb calls mbedtls_aes_encrypt where the fix is located

Let me detail my former explanation. This is the explanation before the fix.

call aes_set_key(ctx3)   <-- here the key of ctx3 is copied in ctx3->key
call aes_encrypt(ctx3)
    -> call HAL_CRYP_AESECB_Encrypt
        -> as ctx->hcryp_aes.Phase = HAL_CRYP_PHASE_READY, copy the ctx3->key in HW registers
        -> set ctx->hcryp_aes.Phase = HAL_CRYP_PHASE_PROCESS
        -> encrypt processing
call aes_set_key(ctx4)   <-- here the key of ctx4 is copied in ctx4->key
call aes_encrypt(ctx4)   <-- here ctx4->key is copied in the hardware registers
    -> call HAL_CRYP_AESECB_Encrypt
        -> as ctx->hcryp_aes.Phase = HAL_CRYP_PHASE_READY, copy the ctx4->key in HW registers
        -> set ctx->hcryp_aes.Phase = HAL_CRYP_PHASE_PROCESS
        -> encrypt processing
call aes_encrypt(ctx3)
    -> call HAL_CRYP_AESECB_Encrypt
        -> as ctx->hcryp_aes.Phase = HAL_CRYP_PHASE_PROCESS, move directly to the processing phase (so the HW still contains ctx4 key, PROBLEM !)
        -> encrypt processing
call aes_encrypt(ctx3)...

The fix consists of resetting the phase variable to READY, so that it keeps copying the keys of the current context in the HW registers

(this was not visible with my former tests, because they were based on aes_selftest, that uses the same key for every tests)
Kind regards
Armelle

@RonEld

RonEld approved these changes Sep 5, 2017

Thanks @adustm for the explanation.
I approve this change

@RonEld

This comment has been minimized.

Contributor

RonEld commented Sep 5, 2017

One comment though, shouldn't we have the same change in mbedtls_aes_decrypt? Couldn't this scenario happen on decryption as well?

@JanneKiiskila

This comment has been minimized.

Contributor

JanneKiiskila commented Sep 5, 2017

Can you please verify also using UBLOX_ODIN_EVK_W2? Seems this fix doesn't resolve the issue we're having with this board.

@andreaslarssonublox

@adustm

This comment has been minimized.

Member

adustm commented Sep 5, 2017

Hello, I don't have the Ublox board. Are you talking about an issue with mbedtls-client test ?

@JanneKiiskila

This comment has been minimized.

Contributor

JanneKiiskila commented Sep 5, 2017

Nope, this is testing with the mbed-cloud-client-example.

@adustm

This comment has been minimized.

Member

adustm commented Sep 5, 2017

Can you reproduce the issue with NUCLEO_F439ZI and mbed-cloud-client-example ?

@teetak01

This comment has been minimized.

teetak01 commented Sep 5, 2017

I am unable to get the UBLOX working with this. Only if I remove the MBEDTLS_CONFIG_HW_SUPPORT macro from targets.json, does UBLOX work again.

Mistake in testing. This works with Ublox :-)

@RobMeades

This comment has been minimized.

Contributor

RobMeades commented Sep 5, 2017

Maybe @andreaslarssonublox can help?

@adustm

This comment has been minimized.

Member

adustm commented Sep 5, 2017

@teetak01 @JanneKiiskila
Your both comments are confusing me : are you both mentionning the same test or not ? Is UBLOX eventually working or not in your use case ?

Kind regards

@andresag01

This comment has been minimized.

Contributor

andresag01 commented Sep 5, 2017

@adustm: Thank you for the PR. I have been playing around with the hardware acceleration code and I reached another failure. I crafted a simple mbed program to reproduce the issue when hardware acceleration is enabled. It does the following:

  • Allocate an array of mbedtls_aes_context structs
  • Set different keys for each struct
  • for each context, run an encryption and check the result matches an expected value
  • Run the previous step X number of times.

It seems that the encryption output is incorrect after the second round in the fourth step, but the program works as expected when hardware acceleration is disabled.

I was able to reproduce this failure with UBLOX_ODIN_EVK_W2, but you can probably observe it with NUCLEO_F439ZI.

#include "mbed.h"

#include "mbedtls/aes.h"
#include "mbedtls/platform.h"
#include "mbedtls/error.h"

#define AES_CTX_COUNT 12
#define REPETITIONS 5

#if !defined(MBEDTLS_AES_ALT)
#warning "AES hardware acceleration is not enabled!"
#endif

const unsigned char keys[AES_CTX_COUNT][16] = {
    { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xf8, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, },
    { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xfc, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, },
    { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xfe, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, },
    { 0xe3, 0x7b, 0x1c, 0x6a, 0xa2, 0x84, 0x6f, 0x6f, 0xdb, 0x41, 0x3f, 0x23, 0x8b, 0x08, 0x9f, 0x23, },
    { 0x6c, 0x00, 0x2b, 0x68, 0x24, 0x83, 0xe0, 0xca, 0xbc, 0xc7, 0x31, 0xc2, 0x53, 0xbe, 0x56, 0x74, },
    { 0x14, 0x3a, 0xe8, 0xed, 0x65, 0x55, 0xab, 0xa9, 0x61, 0x10, 0xab, 0x58, 0x89, 0x3a, 0x8a, 0xe1, },
    { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, },
    { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, },
    { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, },
    { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, },
    { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, },
    { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, },
};

const unsigned char inputs[AES_CTX_COUNT][16] = {
    { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, },
    { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, },
    { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, },
    { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, },
    { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, },
    { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, },
    { 0x6a, 0x11, 0x8a, 0x87, 0x45, 0x19, 0xe6, 0x4e, 0x99, 0x63, 0x79, 0x8a, 0x50, 0x3f, 0x1d, 0x35, },
    { 0xcb, 0x9f, 0xce, 0xec, 0x81, 0x28, 0x6c, 0xa3, 0xe9, 0x89, 0xbd, 0x97, 0x9b, 0x0c, 0xb2, 0x84, },
    { 0xb2, 0x6a, 0xeb, 0x18, 0x74, 0xe4, 0x7c, 0xa8, 0x35, 0x8f, 0xf2, 0x23, 0x78, 0xf0, 0x91, 0x44, },
    { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xc0, 0x00, 0x00, 0x00, 0x00, },
    { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xe0, 0x00, 0x00, 0x00, 0x00, },
    { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xf0, 0x00, 0x00, 0x00, 0x00, },
};

const unsigned char results[AES_CTX_COUNT][16] = {
    { 0x8b, 0x52, 0x7a, 0x6a, 0xeb, 0xda, 0xec, 0x9e, 0xae, 0xf8, 0xed, 0xa2, 0xcb, 0x77, 0x83, 0xe5, },
    { 0x43, 0xfd, 0xaf, 0x53, 0xeb, 0xbc, 0x98, 0x80, 0xc2, 0x28, 0x61, 0x7d, 0x6a, 0x9b, 0x54, 0x8b, },
    { 0x53, 0x78, 0x61, 0x04, 0xb9, 0x74, 0x4b, 0x98, 0xf0, 0x52, 0xc4, 0x6f, 0x1c, 0x85, 0x0d, 0x0b, },
    { 0x43, 0xc9, 0xf7, 0xe6, 0x2f, 0x5d, 0x28, 0x8b, 0xb2, 0x7a, 0xa4, 0x0e, 0xf8, 0xfe, 0x1e, 0xa8, },
    { 0x35, 0x80, 0xd1, 0x9c, 0xff, 0x44, 0xf1, 0x01, 0x4a, 0x7c, 0x96, 0x6a, 0x69, 0x05, 0x9d, 0xe5, },
    { 0x80, 0x6d, 0xa8, 0x64, 0xdd, 0x29, 0xd4, 0x8d, 0xea, 0xfb, 0xe7, 0x64, 0xf8, 0x20, 0x2a, 0xef, },
    { 0xdc, 0x43, 0xbe, 0x40, 0xbe, 0x0e, 0x53, 0x71, 0x2f, 0x7e, 0x2b, 0xf5, 0xca, 0x70, 0x72, 0x09, },
    { 0x92, 0xbe, 0xed, 0xab, 0x18, 0x95, 0xa9, 0x4f, 0xaa, 0x69, 0xb6, 0x32, 0xe5, 0xcc, 0x47, 0xce, },
    { 0x45, 0x92, 0x64, 0xf4, 0x79, 0x8f, 0x6a, 0x78, 0xba, 0xcb, 0x89, 0xc1, 0x5e, 0xd3, 0xd6, 0x01, },
    { 0x90, 0x68, 0x4a, 0x2a, 0xc5, 0x5f, 0xe1, 0xec, 0x2b, 0x8e, 0xbd, 0x56, 0x22, 0x52, 0x0b, 0x73, },
    { 0x74, 0x72, 0xf9, 0xa7, 0x98, 0x86, 0x07, 0xca, 0x79, 0x70, 0x77, 0x95, 0x99, 0x10, 0x35, 0xe6, },
    { 0x56, 0xaf, 0xf0, 0x89, 0x87, 0x8b, 0xf3, 0x35, 0x2f, 0x8d, 0xf1, 0x72, 0xa3, 0xae, 0x47, 0xd8, },
};

int main() {
    int exit_code = MBEDTLS_EXIT_FAILURE, ret;
    mbedtls_aes_context *ctx;
    size_t i, j;
    unsigned char output[16];
    unsigned char iv[16];

    if( ( ctx = (mbedtls_aes_context *)mbedtls_calloc( AES_CTX_COUNT,
                                    sizeof( mbedtls_aes_context ) ) ) == NULL )
    {
        mbedtls_printf( "  !  mbedtls_calloc() returned NULL\r\n" );
        return( exit_code );
    }

    /* Initialise all context structures */
    for( i = 0; i < AES_CTX_COUNT; i++ )
        mbedtls_aes_init( &ctx[i] );

    /* Set keys */
    for( i = 0; i < AES_CTX_COUNT; i++ )
        if( ( ret = mbedtls_aes_setkey_enc( &ctx[i], keys[i], 128 ) ) != 0 )
        {
            mbedtls_printf( "  !  mbedtls_aes_setkey_enc() returned -0x%04X\r\n",
                            ret );
            goto exit;
        }

    /* Encrypt values... a few times */
    for( j = 0; j < REPETITIONS; j++ )
        for( i = 0; i < AES_CTX_COUNT; i++ )
        {
            memset( iv, 0x00, sizeof( iv ) );

            if( ( ret = mbedtls_aes_crypt_cbc( &ctx[i], MBEDTLS_AES_ENCRYPT,
                                               16, iv,
                                               inputs[i], output ) ) != 0 )
            {
                mbedtls_printf( "%d:  !  mbedtls_aes_crypt_cbc() returned "
                                "-0x%04X\r\n", __LINE__, ret );
                goto exit;
            }

            if( memcmp( output, results[i], 16 ) != 0 )
            {
                mbedtls_printf( "%d:  !  memcmp() result is not 0 at "
                                " repetition %u:%u\r\n",
                                __LINE__, j, i );
                goto exit;
            }
        }

    mbedtls_printf( "DONE\r\n" );

    exit_code = MBEDTLS_EXIT_SUCCESS;

exit:
    for( i = 0 ; i < AES_CTX_COUNT; i++ )
        mbedtls_aes_free( &ctx[i] );

    mbedtls_free( ctx );

    return( exit_code );
}
@JanneKiiskila

This comment has been minimized.

Contributor

JanneKiiskila commented Sep 5, 2017

We're talking of the same test here, or actually our application mbed-cloud-client-example.

@andresag01

This comment has been minimized.

Contributor

andresag01 commented Sep 5, 2017

Also, I noticed that the hardware acceleration files aes_alt.h and aes_alt.c are still using the deprecated APIs: mbedtls_aes_encrypt() and mbedtls_aes_decrypt(). Please note that based on feedback, we have updated the AES module with mbedtls_aes_internal_encrypt() and mbedtls_aes_internal_decrypt() that allow int values to be returned to check for errors in the underlying accelerator.

@adustm

This comment has been minimized.

Member

adustm commented Sep 5, 2017

@andresag01

we have updated the AES module with mbedtls_aes_internal_encrypt() and mbedtls_aes_internal_decrypt() that allow int values to be returned to check for errors in the underlying accelerator.

Well thanks, I'll look at that

And also:

It seems that the encryption output is incorrect after the second round in the fourth step, but the program works as expected when hardware acceleration is disabled.

Did you test with this PR fix or without it ? I'll try that, but you definitely should add this test in mbed-os/TESTS/mbed-tls

Cheers

@adustm

This comment has been minimized.

Member

adustm commented Sep 5, 2017

@JanneKiiskila @teetak01
then you confirm that this PR fixes your problem with cloud client example on UBLOX platform ?

@JanneKiiskila

This comment has been minimized.

Contributor

JanneKiiskila commented Sep 5, 2017

@adustm - please elaborate what you mean "then" you confirm? No, with the current fork/commits - the UBLOX_EVK_ODIN_W2 board it is not working correctly. I'm not sure if @teetak01 then tried yet the Nucleo (we have F429 ZI, not F439).

@teetak01

This comment has been minimized.

teetak01 commented Sep 6, 2017

@adustm @JanneKiiskila I tested with Cloud Client. The UBLOX_EVK_ODIN_W2 works fine with this PR. No issues there. Without this PR is always fails on cmac check on certificate data.

@andresag01

This comment has been minimized.

Contributor

andresag01 commented Sep 6, 2017

@adustm:

Did you test with this PR fix or without it ?

I tried the code I posted above with a UBLOX_EVK_ODIN_W2 in the following cases (with the following results):

  1. hardware acceleration with this PR (FAIL)
  2. hardware acceleration without this PR (FAIL)
  3. No hardware acceleration (PASS)

you definitely should add this test in mbed-os/TESTS/mbed-tls

Thank you for the feedback, I have created a new issue to add more stress tests for hardware accelerators in mbed OS: #5036

Please let me know if you would like more information.

@adustm

This comment has been minimized.

Member

adustm commented Sep 7, 2017

@teetak01
Good to know it fixes your issue.
@andresag01 , I'll investigate your test code with NUCLEO_F439ZI ( it should fail as well)
@JanneKiiskila ST has sent a few NUCLEO_F439ZI boards (Mihail or Ron should have them). Please email me if you want 1 more.

Kind regards

@RonEld

This comment has been minimized.

Contributor

RonEld commented Sep 7, 2017

Hi @andresag01
Since there is a new issue for the stress tests, I think it will require a new PR, as it seems not related.
Obviously, the new PR will require this PR to be merged first.
I recomend merging this PR, if it fixes the original issue, and creating a new PR, to fix #5036

@andresag01

This comment has been minimized.

Contributor

andresag01 commented Sep 7, 2017

@RonEld: My understanding from @adustm is that the original problem with the tls-client sample program was that mbed TLS maintains 3 or 4 mbedtls_aes_context structures at some point and interleaves encryption/decryption operations. At some point, this caused some misconfiguration in the accelerator because it was using the parameters of one of the contexts to encrypt data that was meant to be used with a different context.

The small stress test simulates exactly the same situation, but instead of using a couple of mbedtls_aes_context it maintains 12 and interleaves their operation several times. For this reason, I consider that the issue in the acceleration code should be investigated with this test case before this PR is merged. However, this is just my opinion...

@adustm adustm force-pushed the adustm:f439_aes_tlsclient_fix branch to cd1a18f Sep 14, 2017

@adustm

This comment has been minimized.

Member

adustm commented Sep 14, 2017

Dear all,
I have fixed multi context for AES_ECB and AES_CBC modes (ie the hardware accelerated modes) for NUCLEO_F439ZI / NUCLEO_L486RG and NUCLEO_F756ZG.
I have tested the 3 platforms with the test provided by @andresag01 (+ an adaptation for the AES 2nd mode) in GCC_ARM, IAR and ARM toolchains.
I have also tested that it does not break mbed-os-example-tls-tls-client mbed-os-example-tls-authcrypt mbed-os-example-tls-benchmark and mbed-os-example-tls-hashing examples.

At the same time, I am now using the new mbedtls interface for aes (that checks the return values) instead of the deprecated former interface.

I have alse removed the workaround by adding MBEDTLS_AES_ALT back.
I hope this will fit the need. Let me know.

Kind regards
Armelle

* \param input Plaintext block
* \param output Output (ciphertext) block
*/
MBEDTLS_DEPRECATED void mbedtls_aes_encrypt( mbedtls_aes_context *ctx,

This comment has been minimized.

@RonEld

RonEld Sep 14, 2017

Contributor

since it's deprecated and you don't call this API, you don't need to declare it

This comment has been minimized.

@adustm

adustm Sep 14, 2017

Member

What if it is called by users ?

* \param input Ciphertext block
* \param output Output (plaintext) block
*/
MBEDTLS_DEPRECATED void mbedtls_aes_decrypt( mbedtls_aes_context *ctx,

This comment has been minimized.

@RonEld

RonEld Sep 14, 2017

Contributor

since it's deprecated and you don't call this API, you don't need to declare it

This comment has been minimized.

@adustm

adustm Sep 14, 2017

Member

What if it is called by users ?

This comment has been minimized.

@andresag01

andresag01 Sep 14, 2017

Contributor

I think that both @adustm and @RonEld have a point:

@RonEld: mbedtls_aes_decrypt() and mbedtls_aes_encrypt() are both internal functions that were only exposed for the purpose of hardware acceleration. Therefore, they should not really be called by user applications, and we could remove them.

@adustm: In the unlikely case that mbedtls_aes_encrypt() and mbedtls_aes_decrypt() are called by user applications, they need to be defined here otherwise there will be a compilation failure.

I am not sure what the right course of action is in these cases. @yanesca what do you think?

This comment has been minimized.

@RonEld

RonEld Sep 14, 2017

Contributor

I haven't noticed you implemented these functions. But at least you should surround the implementation of these functions with #if !defined(MBEDTLS_DEPRECATED_REMOVED)
As @andresag01 mentioned, these functions are not intended to be called by users

This comment has been minimized.

@yanesca

yanesca Sep 14, 2017

Contributor

The options of giving deprecated warnings for deprecated functions and for removing them is an mbed TLS feature, and as far as I know mbed OS doesn't have it and even if it does, it is unlikely to be integrated. (@0xc0170 Do you know something about a feature like this in mbed OS?)

Therefore I think that we should implement the deprecated functions and enable the applications to call them (even if they were never supposed to do so).

Although I think that the current solution is acceptable, @RonEld is right that it would be prettier if the implementations would be surrounded by #if !defined(MBEDTLS_DEPRECATED_REMOVED). Alternatively we could just remove the MBEDTLS_DEPRECATED macro entirely.

This comment has been minimized.

@adustm

adustm Sep 14, 2017

Member

Thank you all for these answers. I'll add the #if !defined(MBEDTLS_DEPRECATED_REMOVED)

@andresag01

This comment has been minimized.

Contributor

andresag01 commented Sep 14, 2017

@adustm: Many thanks for providing a fix. We will take a look as soon as we can.

@yanesca

@yanesca

I have found a single minor issue, other than that it looks good to me.

if( length % 16 )
return( MBEDTLS_ERR_AES_INVALID_INPUT_LENGTH );
ctx->hcryp_aes.Init.pInitVect = &iv[0];
status |= st_cbc_restore_context(ctx);

This comment has been minimized.

@yanesca

yanesca Sep 14, 2017

Contributor

I think we should check the return value here and return on error, because otherwise it is theoretically possible for a compromised application to use another applications key/context if it gets the timing right.

@@ -1,5 +1,5 @@
/*
* Hardware aes collector for the STM32F4 family
* Hardware aes collector for the STM32F4 F7 and L4 family

This comment has been minimized.

@yanesca

yanesca Sep 14, 2017

Contributor

I am terribly sorry, but I am not very familiar with this terminology, could you please give me some pointers on what an "AES collector" is?

This comment has been minimized.

@adustm

adustm Sep 14, 2017

Member

I honestly don't remember where I copied this expression from. I'll rephrase to Hardware AES implementation for STM32F4 / STM32L4 and STM32F7 families

* \param input Ciphertext block
* \param output Output (plaintext) block
*/
MBEDTLS_DEPRECATED void mbedtls_aes_decrypt( mbedtls_aes_context *ctx,

This comment has been minimized.

@yanesca

yanesca Sep 14, 2017

Contributor

The options of giving deprecated warnings for deprecated functions and for removing them is an mbed TLS feature, and as far as I know mbed OS doesn't have it and even if it does, it is unlikely to be integrated. (@0xc0170 Do you know something about a feature like this in mbed OS?)

Therefore I think that we should implement the deprecated functions and enable the applications to call them (even if they were never supposed to do so).

Although I think that the current solution is acceptable, @RonEld is right that it would be prettier if the implementations would be surrounded by #if !defined(MBEDTLS_DEPRECATED_REMOVED). Alternatively we could just remove the MBEDTLS_DEPRECATED macro entirely.

@@ -182,21 +247,41 @@ int mbedtls_aes_crypt_cbc( mbedtls_aes_context *ctx,
int status = 0;
if( length % 16 )
return( MBEDTLS_ERR_AES_INVALID_INPUT_LENGTH );
ctx->hcryp_aes.Init.pInitVect = &iv[0];
status |= st_cbc_restore_context(ctx);

This comment has been minimized.

@yanesca

yanesca Sep 14, 2017

Contributor

One of my review comments is shown as outdated, reposting it for visibility:

I think we should check the return value here and return on error, because otherwise it is theoretically possible for a compromised application to use another applications key/context if it gets the timing right.

This comment has been minimized.

@adustm

adustm Sep 14, 2017

Member

Correct.
I have 6 times status |=...
Do you think I should replace each of them by
if (... ) return 1;
?

This comment has been minimized.

@yanesca

yanesca Sep 14, 2017

Contributor

Yes, I think that would be good practice and a more defensive coding!

@andresag01

@adustm: Many thanks for the PR. I have reviewed the changes and left some comments, questions, suggestions, etc.

@@ -30,6 +30,8 @@
#ifdef __cplusplus
extern "C" {
#endif

#define ST_AES_TIMEOUT ((uint32_t) 3)

This comment has been minimized.

@andresag01

andresag01 Sep 14, 2017

Contributor

Could you please add a comment explaining the rationale behind this #define? Also, it would be good to document what units are used and why is 3 a good choice.

This comment has been minimized.

@adustm

adustm Sep 15, 2017

Member

I must check that the crypto processor is not busy before reading / writing some registers.
I cannot use a while loop without any timeout.
The minimum timeout is 1 ms, which is more than sufficient for the crypto processor to process any command.
I change this value to 1 and add a comment /* 1 ms timeout is sufficient for the crypto processor */

This comment has been minimized.

@yanesca

yanesca Sep 15, 2017

Contributor

I think that instead of lowering the timeout to one it would be better to raise it to a very high value, preferably high enough to make the behaviour practically synchronous, because:

  • it has no performance toll when the accelerator is working properly
  • the interface of mbed TLS crypto modules is inherently synchronous and applications (with the TLS stack in mbed TLS among them) are not prepared for asynchronous use and low timeout here would result in highly unreliable behaviour.

This comment has been minimized.

@adustm

adustm Sep 15, 2017

Member

How many milliseconds would you consider as high enough ? The default value used in ST examples codes is 0xFF

This comment has been minimized.

@yanesca

yanesca Sep 15, 2017

Contributor

Considering that as you say 1 ms should be fine in most cases, 0xFF seems good enough to me.

if (HAL_CRYP_AESECB_Encrypt(&ctx->hcryp_aes, (uint8_t *)input, 16, (uint8_t *)output, 10) !=0) {
// error found to be returned
// error found
return 1;

This comment has been minimized.

@andresag01

andresag01 Sep 14, 2017

Contributor

Normally mbed TLS errors are negative integers no larger than 2 bytes. What should the hardware acceleration error codes look like? @RonEld @yanesca

if(HAL_CRYP_AESECB_Decrypt(&ctx->hcryp_aes, (uint8_t *)input, 16, (uint8_t *)output, 10)) {
// error found to be returned
// error found
return 1;

This comment has been minimized.

@andresag01

andresag01 Sep 14, 2017

Contributor

Same as above

This comment has been minimized.

@adustm

adustm Sep 15, 2017

Member

Same as above

tickstart = HAL_GetTick();
while((ctx->hcryp_aes.Instance->SR & AES_SR_BUSY) != 0){
if ((HAL_GetTick() - tickstart) > ST_AES_TIMEOUT) {
return 1; // timeout: CRYP processor is busy

This comment has been minimized.

@andresag01

andresag01 Sep 14, 2017

Contributor

I think this is quite interesting. What should happen if the crypto accelerator is busy? Normally you would have a lock or something to protect concurrent use of the accelerator. However, if you do not lock accesses and find out that the accelerator is busy, should you return a generic error? IMHO we should be returning something like MBEDTLS_ERR_AES_BUSY, or at least something that allows the user to distinguish when its a real failure and when its a concurrency problem. @yanesca @RonEld what do you think?

This comment has been minimized.

@adustm

adustm Sep 15, 2017

Member

I'll return ST_ERR_AES_BUSY, as MBEDTLS_ERR_AES_BUSY is not defined
Can it be -0x23 ? (#define ST_ERR_AES_BUSY (-0x0023) )

This comment has been minimized.

@yanesca

yanesca Sep 15, 2017

Contributor

Yes, it is a very good choice, that error code is not used in mbed TLS at the moment.

Even if later the code gets assigned to some mbed TLS error, this shouldn't be a problem as long as the AES module has strictly synchronous interface in mbed TLS (this seems highly unlikely to change in the future).

tickstart = HAL_GetTick();
while((ctx->hcryp_aes.Instance->SR & AES_SR_BUSY) != 0){
if ((HAL_GetTick() - tickstart) > ST_AES_TIMEOUT) {
return 1; // timeout: CRYP processor is busy

This comment has been minimized.

@andresag01

andresag01 Sep 14, 2017

Contributor

Same as above

This comment has been minimized.

@adustm

adustm Sep 15, 2017

Member

Same as above

tickstart = HAL_GetTick();
while((ctx->hcryp_aes.Instance->SR & (CRYP_SR_IFEM | CRYP_SR_OFNE | CRYP_SR_BUSY)) != CRYP_SR_IFEM){
if ((HAL_GetTick() - tickstart) > ST_AES_TIMEOUT) {
return 1; // timeout: CRYP processor is busy

This comment has been minimized.

@andresag01

andresag01 Sep 14, 2017

Contributor

Same as above

tickstart = HAL_GetTick();
while((ctx->hcryp_aes.Instance->SR & (CRYP_SR_IFEM | CRYP_SR_OFNE | CRYP_SR_BUSY)) != CRYP_SR_IFEM){
if ((HAL_GetTick() - tickstart) > ST_AES_TIMEOUT) {
return 1; // timeout: CRYP processor is busy

This comment has been minimized.

@andresag01

andresag01 Sep 14, 2017

Contributor

Same as above

This comment has been minimized.

@adustm

adustm Sep 15, 2017

Member

Same as above

if( mode == MBEDTLS_AES_DECRYPT ) {
status = st_hal_cryp_cbc(ctx, CRYP_ALGOMODE_KEYDERIVATION_DECRYPT, length, iv, (uint8_t *)input, (uint8_t *)output);
status |= st_hal_cryp_cbc(ctx, CRYP_ALGOMODE_KEYDERIVATION_DECRYPT, length, iv, (uint8_t *)input, (uint8_t *)output);

This comment has been minimized.

@andresag01

andresag01 Sep 14, 2017

Contributor

I see that the input parameter is marked as const. Is there a risk that this function will actually modify the input buffer contents? If this is the case, this will probably change the behaviour of the mbed TLS function

tickstart = HAL_GetTick();
while((ctx->hcryp_aes.Instance->SR & AES_SR_BUSY) != 0){
if ((HAL_GetTick() - tickstart) > ST_AES_TIMEOUT) {
return 1; // timeout: CRYP processor is busy

This comment has been minimized.

@andresag01

andresag01 Sep 14, 2017

Contributor

Same as above

This comment has been minimized.

@adustm

adustm Sep 15, 2017

Member

Same as above

#endif /* TARGET_STM32L486xG */

int mbedtls_aes_crypt_cbc( mbedtls_aes_context *ctx,

This comment has been minimized.

@andresag01

andresag01 Sep 14, 2017

Contributor

It seems that procedure to save/restore contexts is quite involved and has different requirements for different functions. Would it be possible to include a comment somewhere in the code that documents in plain English what is the thinking behind these operations to enable interleaved uses of the accelerator with multiple contexts?

Change after code review : standardize calls among ST families
Check return values in alignment with MBEDTLS error codes
@yanesca

Looks good to me! Thank you very much for this PR and your patience!

Although I don't have any change request, I would like to share a minor remark, feel free to ignore it if you don't think it is an issue:

I think it is great that now we return on errors, but masking all return value with ST_ERR_AES_BUSY might be misleading and it might make debugging and error handling difficult for the application developer.

@adustm

This comment has been minimized.

Member

adustm commented Sep 18, 2017

Thank you @yanesca

I think it is great that now we return on errors, but masking all return value with ST_ERR_AES_BUSY might be misleading and it might make debugging and error handling difficult for the application developer.

You're right, but the only available values are (-0x21) and (-0x23). I took only 1 of those values.
At least the user knows on which module to look at

@andresag01

@adustm: Many thanks for the fix.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 19, 2017

@adustm I restarted jenkins CI, will review the results once done and we should trigger nightly for this one

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 20, 2017

/morph test-nightly

@theotherjimmy theotherjimmy changed the title from Fix #4928 use of AES_ALT on STM32F439 for example-tls-client to Fix use of AES_ALT on STM32F439 for example-tls-client Sep 20, 2017

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 20, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 1345

All builds and test passed!

@theotherjimmy theotherjimmy merged commit 43aa6a2 into ARMmbed:master Sep 20, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test-nightly Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@adustm adustm deleted the adustm:f439_aes_tlsclient_fix branch Jan 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment