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

NUC472/M487: Refine code with mbed TLS crypto alternatives #4925

Merged
merged 60 commits into from Jan 8, 2018

Conversation

@ccli8
Contributor

ccli8 commented Aug 17, 2017

Description

This PR refines code with mbed TLS crypto alternatives on code review in previous PR. It includes the following modifications:

  1. Remove debug code in AES alternative.
  2. Remove unnecessary macro definitions and clarify header file include.
  3. Fix DES alternative function not thread-safe.

Related PRs

#4832
#4608

@0xc0170 @RonEld

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 17, 2017

features/mbedtls/targets/TARGET_NUVOTON/TARGET_NUC472/aes/aes_alt.c Outdated
static void __nvt_aes_crypt( mbedtls_aes_context *ctx,
const unsigned char input[16],
unsigned char output[16], int dataSize)
const unsigned char input[16],

This comment has been minimized.

@RonEld

RonEld Aug 17, 2017

Contributor

I missed this in the previous Review.
This function also seems not thread safe. au8OutputData and au8InputData are defined out of this function, as static variables, which could be overwritten every cryptographic call

This comment has been minimized.

@ccli8

ccli8 Aug 18, 2017

Contributor

@RonEld I fixed it. Please check c405d00.

@RonEld

This comment has been minimized.

Contributor

RonEld commented Aug 17, 2017

Hi @ccli8 Thank you for this PR, the code is now a lot more readable.
I have noticed that AES could also not be thread safe, and commented on that

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 21, 2017

@yanesca Thoughts?

@yanesca

This comment has been minimized.

Contributor

yanesca commented Aug 21, 2017

@theotherjimmy I didn't loose track of this PR, I am just tied down with other tasks. I will review this and the underlying two other PRs as soon as I can!

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 21, 2017

@yanesca The pings are just to make sure you don't lose track of it. Feel free to get to it when it's best for you.

@yanesca

This comment has been minimized.

Contributor

yanesca commented Aug 21, 2017

@theotherjimmy Ah ok, thank you!

@yanesca

This comment has been minimized.

Contributor

yanesca commented Aug 22, 2017

My current estimate is that I can start looking at this by the end of next week.

@adbridge adbridge requested a review from yanesca Aug 24, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 1, 2017

My current estimate is that I can start looking at this by the end of next week.

@yanesca Bump for review

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 1, 2017

@tommikas The jenkins CI did not report the status for this PR

@tommikas

This comment has been minimized.

Contributor

tommikas commented Sep 1, 2017

So it seems. I restarted the build. It should update once it's done.

@yanesca

This comment has been minimized.

Contributor

yanesca commented Sep 1, 2017

I am terribly sorry, but a lot has changed since my last comment and I haven't had the time to look at this yet. I don't have any estimate when that will be, but I am going to review this PR as soon as I can.

@ciarmcom

This comment has been minimized.

Member

ciarmcom commented Sep 7, 2017

ARM Internal Ref: IOTSSL-1718

@andresag01

This comment has been minimized.

Contributor

andresag01 commented Sep 12, 2017

I tried the simple stress test that I added in issue #5079 and I got the following error:

mbed assertation failed: No available AES channel, file: ./mbed-os/features/mbedtls/targets/TARGET_NUVOTON/TARGET_M480/aes/aes_����k96

Could you please take a look at this failure?

@ccli8

This comment has been minimized.

Contributor

ccli8 commented Sep 13, 2017

mbed assertation failed: No available AES channel, file: ./mbed-os/features/mbedtls/targets/TARGET_NUVOTON/TARGET_M480/aes...

@andresag01 NUC472/M480 AES H/W just supports 4 channels, but #5079 requires 12. That's the cause. Is the fix necessary? If yes, we need to merge in AES S/W (mbed-os/features/mbedtls/src/aes.c) as fallback. Because AES S/W (mbed-os/features/mbedtls/src/aes.c) and AES H/W (aes_alt.c) are mutually exclusive in current mbed TLS crypto framework, the merge-in of AES S/W would duplicate the AES S/W code.

Bump @cyliangtw

@andresag01

This comment has been minimized.

Contributor

andresag01 commented Sep 13, 2017

@ccli8: Thanks for replying. Given that there are only four channels in the device, is it possible to somehow save and restore the accelerator context somehow so that more than 4 mbedtls AES contexts are supported?

@RonEld, @yanesca, @sbutcher-arm: What do you think about this limitation?

@ccli8

This comment has been minimized.

Contributor

ccli8 commented Sep 14, 2017

Given that there are only four channels in the device, is it possible to somehow save and restore the accelerator context somehow so that more than 4 mbedtls AES contexts are supported?

@andresag01 I would try your suggestion.

@ccli8

This comment has been minimized.

Contributor

ccli8 commented Sep 14, 2017

@andresag01 To pass test code in #5079, I fix AES alter. bugs. Please check commits b9a3298 and a63c9e2.

@andresag01

This comment has been minimized.

Contributor

andresag01 commented Sep 14, 2017

@ccli8: Many thanks for considering my comments. I will take a look at the changes as soon as I can.

@andresag01

@ccli8: Thank you for your contribution. I have given a first pass to the changes related to hardware acceleration and left several comments and questions.

features/mbedtls/targets/TARGET_NUVOTON/TARGET_M480/aes/aes_alt.c Outdated
{
if( i >=0 && i < (int)sizeof(channel_flag) )
channel_flag[i] = 0x00;
unsigned int* piv;

This comment has been minimized.

@andresag01

andresag01 Sep 18, 2017

Contributor

I would prefer using uint32_t

features/mbedtls/targets/TARGET_NUVOTON/TARGET_M480/aes/aes_alt.h Outdated
#endif
uint32_t iv[4];
unsigned char prv_iv[16];
uint32_t buf[8];

This comment has been minimized.

@andresag01

andresag01 Sep 18, 2017

Contributor

Could you add a comment describing what this buffer is used for?

This comment has been minimized.

@ccli8

ccli8 Sep 21, 2017

Contributor

@andresag01 I add comment in 2a8875b.

features/mbedtls/targets/TARGET_NUVOTON/TARGET_M480/aes/aes_alt.c Outdated
/* We support multiple contexts with context save & restore and so needs just one
* H/W channel. Always use H/W channel #0. */
ctx->channel = 0;
memset(ctx->iv, 0, sizeof (ctx->iv));

This comment has been minimized.

@andresag01

andresag01 Sep 18, 2017

Contributor

This seems slightly redundant given that you have already set the whole ctx to 0.

This comment has been minimized.

@ccli8

ccli8 Sep 21, 2017

Contributor

@andresag01 This code is refined in 2d7a33e.

features/mbedtls/targets/TARGET_NUVOTON/TARGET_M480/aes/aes_alt.c Outdated
* H/W channel. Always use H/W channel #0. */
ctx->channel = 0;
memset(ctx->iv, 0, sizeof (ctx->iv));
/* Unlock protected registers */
SYS_UnlockReg();
CLK_EnableModuleClock(CRPT_MODULE);

This comment has been minimized.

@andresag01

andresag01 Sep 18, 2017

Contributor

Is it possible for a user to actually switch the crypto module off?

int i =-1;
mbedtls_trace("=== %s \r\n", __FUNCTION__);
memset( ctx, 0, sizeof( mbedtls_aes_context ) );

This comment has been minimized.

@andresag01

andresag01 Sep 18, 2017

Contributor

What happens if the input pointer is NULL?

This comment has been minimized.

@ccli8

ccli8 Sep 21, 2017

Contributor

@andresag01 This follows mbed-os/features/mbedtls/src/aes.c by assuming upper layer won't pass NULL context pointer.

This comment has been minimized.

@andresag01

andresag01 Sep 21, 2017

Contributor

ok

#include "config.h"
#else
#include MBEDTLS_CONFIG_FILE
#endif

This comment has been minimized.

@andresag01

andresag01 Sep 18, 2017

Contributor

Same comment as above regarding config.h

#if defined(MBEDTLS_SHA512_C)
#if defined(MBEDTLS_SHA512_ALT)
#include "mbedtls/sha512.h"
#if defined(_MSC_VER) || defined(__WATCOMC__)

This comment has been minimized.

@andresag01

andresag01 Sep 18, 2017

Contributor

I think this is just for windows, so not needed in mbed OS

This comment has been minimized.

@ccli8

ccli8 Sep 21, 2017

Contributor

@andresag01 Same as above.

@@ -56,8 +50,10 @@
#endif /* MBEDTLS_SELF_TEST */

This comment has been minimized.

@andresag01

andresag01 Sep 18, 2017

Contributor

I do not think there is a self test in this file

This comment has been minimized.

@ccli8

ccli8 Sep 21, 2017

Contributor

@andresag01 Same as above.

#else
#include MBEDTLS_CONFIG_FILE
#endif

This comment has been minimized.

@andresag01

andresag01 Sep 18, 2017

Contributor

Same comment as above regarding config.h

features/mbedtls/targets/TARGET_NUVOTON/TARGET_NUC472/sha/sha_alt_hw.h Outdated
#else
#include MBEDTLS_CONFIG_FILE
#endif
#if defined(MBEDTLS_SHA1_C) || defined(MBEDTLS_SHA256_C) || defined(MBEDTLS_SHA512_C)
#if defined(MBEDTLS_SHA1_ALT) || defined(MBEDTLS_SHA256_ALT) || defined(MBEDTLS_SHA512_ALT)

This comment has been minimized.

@andresag01

andresag01 Sep 18, 2017

Contributor

I woudl suggest splitting this file into 1 file per module as with the software ones...

This comment has been minimized.

@ccli8

ccli8 Sep 21, 2017

Contributor

@andresag01 SHA1/SHA256/SHA512 share the same SHA H/W, so these code are placed together.

@ccli8

This comment has been minimized.

Contributor

ccli8 commented Sep 21, 2017

I have given a first pass to the changes related to hardware acceleration and left several comments and questions.

@andresag01 I update AES first. I would update DES and SHA later.

@ccli8

This comment has been minimized.

Contributor

ccli8 commented Sep 21, 2017

I have given a first pass to the changes related to hardware acceleration and left several comments and questions.

@andresag01 Besides AES just now, I add updates for DES and SHA. Please check them.

features/mbedtls/targets/TARGET_NUVOTON/TARGET_M480/aes/aes_alt.c Outdated
channel_flag[i] = 0x00;
uint32_t buff_ = (uint32_t) buff;
return (((buff_ & 0x03) == 0) && ((buff_ & 0x20000000) == 0x20000000));

This comment has been minimized.

@andresag01

andresag01 Sep 21, 2017

Contributor

I am not sure this check corresponds to the conditions that you have stated in the comment above. For example consider the value 0x30000000 that is word aligned and will also satisfy the check (buff_ & 0x20000000) == 0x20000000 even though is not in the 0x2xxxxxxx range. Perhaps I misunderstood the constraints, but I believe something like this will satisfy:

ptr % 4 == 0 && (ptr & 0xF0000000) == 0x20000000

This comment has been minimized.

@ccli8

ccli8 Sep 22, 2017

Contributor

@andresag01 Yes, you are right. I fixed it in 3119935.

int i =-1;
mbedtls_trace("=== %s \r\n", __FUNCTION__);
memset( ctx, 0, sizeof( mbedtls_aes_context ) );

This comment has been minimized.

@andresag01

andresag01 Sep 21, 2017

Contributor

ok

@andresag01

andresag01 suggested changes Sep 22, 2017 edited

I have reviewed the changes once again and left more comments and questions.

As a general note, I still do not understand why are there two copies of the following files:

  • aes_alt.c
  • aes_alt.h
  • des_alt.h
  • des_alt.c
  • etc

I compared some of these and except for aes_alt.c they seem to have exactly the same content. Also, the differences between the two aes_alt.c files is only these lines:

< #include "M480.h"
---
> #include "NUC472_442.h"
193,196c193,196
<     ctx->iv[0] = CRPT->AES_FDBCK[0];
<     ctx->iv[1] = CRPT->AES_FDBCK[1];
<     ctx->iv[2] = CRPT->AES_FDBCK[2];
<     ctx->iv[3] = CRPT->AES_FDBCK[3];
---
>     ctx->iv[0] = CRPT->AES_FDBCK0;
>     ctx->iv[1] = CRPT->AES_FDBCK1;
>     ctx->iv[2] = CRPT->AES_FDBCK2;
>     ctx->iv[3] = CRPT->AES_FDBCK3;

I do not think that differences justify the code duplication. Please take a look at the acceleration code for other targets to get an idea of what I mean.

features/mbedtls/targets/TARGET_NUVOTON/TARGET_M480/aes/aes_alt.c Outdated
memset( ctx, 0, sizeof( mbedtls_aes_context ) );
ctx->swapType = AES_IN_OUT_SWAP;

This comment has been minimized.

@andresag01

andresag01 Sep 22, 2017

Contributor

Question, what does AES_IN_OUT_SWAP represent?

This comment has been minimized.

@ccli8

ccli8 Sep 27, 2017

Contributor

AES_IN_OUT_SWAP represents input/output data in little-endian. The code was refined in
ed39056.

features/mbedtls/targets/TARGET_NUVOTON/TARGET_M480/aes/aes_alt.c Outdated
ctx->channel = i;
ctx->iv = au32MyAESIV;
/* Unlock protected registers */
SYS_UnlockReg();
CLK_EnableModuleClock(CRPT_MODULE);

This comment has been minimized.

@andresag01

andresag01 Sep 22, 2017

Contributor

Question: Is it possible for a user to actually switch the crypto module off when not in use? Is it necessary at all to do that in order to save power, etc?

This comment has been minimized.

@ccli8

ccli8 Sep 27, 2017

Contributor

In this commit 3aff492, crypto module clock is disabled when it is not in use.

features/mbedtls/targets/TARGET_NUVOTON/TARGET_M480/aes/aes_alt.c Outdated
ctx->buf[i] = (*(key+i*4) << 24) |
(*(key+1+i*4) << 16) |
(*(key+2+i*4) << 8) |
(*(key+3+i*4) );

This comment has been minimized.

@andresag01

andresag01 Sep 22, 2017

Contributor

@ccli8: Perhaps I am missing something, but I do not think that commit bd0960a modifies these lines of code.

features/mbedtls/targets/TARGET_NUVOTON/TARGET_M480/aes/aes_alt.c Outdated
* 2) Located in 0x2xxxxxxx region
*/
MBED_ALIGN(4) uint8_t au8OutputData[MAX_DMA_CHAIN_SIZE];
MBED_ALIGN(4) uint8_t au8InputData[MAX_DMA_CHAIN_SIZE];

This comment has been minimized.

@andresag01

andresag01 Sep 22, 2017

Contributor

Could you please take a look at this code again? I am not entirely sure it will work reliably given the constraints in the memory range for the DMA.

At the moment, these two buffers are being allocated in the stack. However, the thread stack is actually allocated in heap memory when a new thread is spawned. I looked at the linker script for one of the target NUC472 (XRAM_SUPPORTED) and found that:

RAM_EXTERN (rwx)      : ORIGIN = 0x60000000, LENGTH = 0x00100000
...
    .heap (NOLOAD):
    {
        __end__ = .;
        end = __end__;
        *(.heap*);
        . += (ORIGIN(RAM_EXTERN) + LENGTH(RAM_EXTERN) - .);
        __HeapLimit = .;
    } > RAM_EXTERN

Given the origin for RAM_EXTERN, it looks like these buffers will not be in 0x20000000 and the check below would be triggered for this target. However, it would work on NUC (XRAM_UNSUPPORTED). Given the constraints, I think you probably need to allocate these buffers in .bss instead, but I am not as familiar with the nuvoton linker scripts. Please feel free to consider alternatives...

This comment has been minimized.

@ccli8

ccli8 Sep 27, 2017

Contributor

@andresag01 Yes, it was a potential bug. In 20b696d, DMA buffer is now allocated at bss to guarantee it is located at 0x20000.000 region.

features/mbedtls/targets/TARGET_NUVOTON/TARGET_M480/aes/aes_alt.c Outdated
ctx->encDec = 0;
__nvt_aes_crypt(ctx, input, output, blockChainLen);
memcpy( iv, temp, 16 );
}

This comment has been minimized.

@andresag01

andresag01 Sep 22, 2017

Contributor

As far as I can see, this code is using three copies of the same value, just in different format:

  • iv: the input parameter to the function in LE
  • ctx->iv: same as iv but BE
  • temp: same as iv
    Because of this, I think temp is unnecessary, but please feel free to point out any flaws in my reasoning.

This comment has been minimized.

@ccli8

ccli8 Sep 28, 2017

Contributor

@andresag01 Same as above.

features/mbedtls/targets/TARGET_NUVOTON/TARGET_M480/aes/aes_alt.c Outdated
}
/* Last incomplete block */
size_t last_block_len = length;

This comment has been minimized.

@andresag01

andresag01 Sep 22, 2017

Contributor

Is last_block_len really necessary?

This comment has been minimized.

@ccli8

ccli8 Sep 28, 2017

Contributor

@andresag01 I refined it in 2fd1226.

features/mbedtls/targets/TARGET_NUVOTON/TARGET_M480/aes/aes_alt.h Outdated
uint32_t opMode; /* AES_MODE_ECB/CBC/CFB */
uint32_t swapType; /* Input/Output data endianness */
uint32_t iv[4]; /* IV for next block cipher */
uint32_t buf[8]; /* Cipher key */

This comment has been minimized.

@andresag01

andresag01 Sep 22, 2017

Contributor

I think this buffer holds the BE representation of the AES keys. Perhaps we should just call the buffer key instead of buf...

This comment has been minimized.

@ccli8

ccli8 Sep 28, 2017

Contributor

@andresag01 I renamed buf to keys in 2fd1226.

features/mbedtls/targets/TARGET_NUVOTON/TARGET_M480/aes/aes_alt.h Outdated
uint32_t keySize; /* Key size: AES_KEY_SIZE_128/192/256 */
uint32_t encDec; /* 0: decrypt, 1: encrypt */
uint32_t opMode; /* AES_MODE_ECB/CBC/CFB */
uint32_t swapType; /* Input/Output data endianness */

This comment has been minimized.

@andresag01

andresag01 Sep 22, 2017

Contributor

swapType seems to only ever hold one value, namely:

ctx->swapType = AES_IN_OUT_SWAP;

Is it necessary to keep this variable around?

This comment has been minimized.

@ccli8

ccli8 Sep 28, 2017

Contributor

@andresag01 I removed it in ed39056.

features/mbedtls/targets/TARGET_NUVOTON/TARGET_M480/des/des_alt.c Outdated
while (rmn) {
// Must be a multiple of 64-bit block size
#define MAXSIZE_DMABUF (8 * 5)

This comment has been minimized.

@andresag01

andresag01 Sep 22, 2017

Contributor

Where does this limit come from?

This comment has been minimized.

@ccli8

ccli8 Sep 28, 2017

Contributor

@andresag01 It was roughly estimated to make a trade-off between memory footprint and performance. I commented it in 2fd1226.

#if defined(MBEDTLS_SHA1_C)
#if defined(MBEDTLS_SHA1_ALT)
#include "mbedtls/sha1.h"
#include <string.h>
#if defined(MBEDTLS_SELF_TEST)

This comment has been minimized.

@andresag01

andresag01 Sep 22, 2017

Contributor

@ccli8: Could you please help me understand why is there a software fallback for SHA1 at all? I mean, it seems that there is an accelerator available. Is it because for some reason it is not possible to switch the accelerator context so that operations can be interleaved?

P.S. The same comment does for SHA256 and SHA512

@ccli8

This comment has been minimized.

Contributor

ccli8 commented Sep 28, 2017

As a general note, I still do not understand why are there two copies of the following files ...

@andresag01 At present, our port, like serial, i2c, spi, etc. separate code by chip family. NUC472 and M487 belong to different chip families, so there are two copies of them. In the future, we may consider merging them together when there are too many branches and it is difficult to maintain them.

@ccli8 ccli8 force-pushed the OpenNuvoton:nuvoton_crypto branch from 08c4e82 to 67386b9 Jan 5, 2018

@ccli8

This comment has been minimized.

Contributor

ccli8 commented Jan 5, 2018

@0xc0170 I've done with rebase. Please check it.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 5, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jan 5, 2018

Build : SUCCESS

Build number : 809
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/4925/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 5, 2018

Node went offline, restarting

/morph export-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Jan 8, 2018

@0xc0170

0xc0170 approved these changes Jan 8, 2018

@theotherjimmy theotherjimmy merged commit 3c53908 into ARMmbed:master Jan 8, 2018

18 checks passed

AWS-CI uVisor Build & Test Verification build successful.
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Local events testing has passed
Details
travis-ci/littlefs Local littlefs testing has passed
Details
travis-ci/mbed2-ATMEL Local mbed2-ATMEL testing has passed
Details
travis-ci/mbed2-MAXIM Local mbed2-MAXIM testing has passed
Details
travis-ci/mbed2-NORDIC Local mbed2-NORDIC testing has passed
Details
travis-ci/mbed2-NUVOTON Local mbed2-NUVOTON testing has passed
Details
travis-ci/mbed2-NXP Local mbed2-NXP testing has passed
Details
travis-ci/mbed2-RENESAS Local mbed2-RENESAS testing has passed
Details
travis-ci/mbed2-SILICON_LABS Local mbed2-SILICON_LABS testing has passed
Details
travis-ci/mbed2-STM Local mbed2-STM testing has passed
Details
travis-ci/tools Local tools testing has passed
Details

@ccli8 ccli8 deleted the OpenNuvoton:nuvoton_crypto branch Jan 9, 2018

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