Fix issues in Cryptocell 310 shax_alt discovered by On Target Testing #8728

RonEld · 2018-11-13T12:35:34Z

Description

Initialize the Cryptocell context in the mbedtls_shax_process()
function, in case it wasn't initialized. The Process function can be
called without calling to the starts function. Discovered by
On Target Testing on the NRF52840_DK platform.

Note: This might have conflicts with #8643 , but not dependent on it.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

0xc0170 · 2018-11-15T07:49:39Z

@RonEld is @ARMmbed/mbed-os-tls the team that should review this fix? If yes, please review

RonEld · 2018-11-15T09:58:36Z

@0xc0170 I would say the @ARMmbed/mbed-os-crypto team should review this

cmonr · 2018-11-22T02:23:16Z

Restarting CI

0xc0170 · 2018-11-22T11:01:29Z

Aborting CI, will start once it is approved

mbed-ci · 2018-11-22T12:18:47Z

Test run: SUCCESS

Summary: 4 of 4 test jobs passed
Build number : 3
Build artifacts
Build logs

0xc0170 · 2018-11-22T12:24:01Z

It was completed, waiting for reviewers

yanesca

I don't think that this is the right way to resolve the issue. I left a comment about my thoughts in the code.

yanesca · 2018-11-22T15:39:10Z

features/cryptocell/FEATURE_CRYPTOCELL310/sha1_alt.c

+    int ret = 0;
+    if( ctx->is_cc_initiated == 0 )
+    {
+        ret = init_cc( ctx );


I don't think that calling mbedtls_internal_sha1_process() should be supported for several reasons:

This is an internal function, users shouldn't call it anyway.

If anybody, Mbed TLS, the tests or our users call it without calling mbedtls_sha1_starts() first, then they are violating the API

As you can see::
sha1_process_wrap calls mbedtls_internal_sha1_process(). I guess it's ok, since it's internal. The test for mbedtlos_md_process() only calls setup and process, so perhaps it's a bug in the test...

I think it is a bug in the test, it should call mbedtls_md_starts( ) too.

created Mbed-TLS/mbedtls#2227 to track this

yanesca · 2018-11-22T15:40:34Z

features/cryptocell/FEATURE_CRYPTOCELL310/sha1_alt.h

@@ -33,6 +33,7 @@ extern "C" {
 typedef struct
 {
    CRYS_HASHUserContext_t crys_hash_ctx;
+    int                    is_cc_initiated;


style: I think the word we are looking for here is "initialised" and not "initiated".

RonEld · 2018-11-22T15:51:50Z

@yanesca Thanks for the review!
If we agreee the issue is in the test code, and not in the driver, then I guess we can close this PR

yanesca · 2018-11-22T16:03:25Z

I think we agree, but I think that the style changes in this PR are still valuable. Maybe we could still merge those or just cherry-pick them to the top of one of the other PRs.

Also, before doing anything we should raise an issue for the bug in the tests. Which repository are they in?

RonEld · 2018-11-22T16:07:12Z

The tests are from the Mbed TLS repository
I can change do the style changes early next week

yanesca · 2018-11-22T16:33:57Z

Can you point me to the failing test and/or raise an issue about this in the Mbed TLS repository?

RonEld · 2018-11-22T18:47:18Z

@yanesca mbedtls_md_process() is the failing test.
However, after thinking some more, I think we still need to have this initiated mechanism.
The TLS code, for example here clones an already started SHA context. I think that once cloning, we should consider the context as not initiated, as the HW may have switched context, so we need to initiate again.

RonEld · 2018-11-28T09:18:54Z

@yanesca As discussed offline, I have changed the implementation of the internal process functions, to return the unsupported error.
Once reviewed and approved, I will rebase to resolve the conflicts

RonEld · 2018-11-28T12:30:20Z

I have taken Andres' stress test from #5079 and modified to test cloning:

int sha256()
{
    int ret, next;
    size_t i, j;
    mbedtls_sha256_context *ctx;
    mbedtls_sha256_context *cloned_ctx;
    unsigned char output[32];
    size_t proc_len[SHA256_CTX_COUNT];
    size_t next_len;

    if( ( ctx = (mbedtls_sha256_context *)mbedtls_calloc( SHA256_CTX_COUNT,
                                    sizeof( mbedtls_sha256_context ) ) ) == NULL )
    {
        mbedtls_printf( "  !  mbedtls_calloc() return NULL\r\n" );
        return( -1 );
    }
    
    if( ( cloned_ctx = (mbedtls_sha256_context *)mbedtls_calloc( SHA256_CTX_COUNT,
                                    sizeof( mbedtls_sha256_context ) ) ) == NULL )
    {
        mbedtls_printf( "  !  mbedtls_calloc() return NULL\r\n" );
        return( -1 );
    }

    /* Initialise all context structures */
    for( i = 0; i < SHA256_CTX_COUNT; i++ )
    {
        mbedtls_sha256_init( &ctx[i] );
        mbedtls_sha256_init( &cloned_ctx[i] );
    }

    for( j = 1; j < REPETITIONS; j++ )
    {
        int cloned[SHA256_CTX_COUNT] = {0};
        /* Start the contexts */
        for( i = 0; i < SHA256_CTX_COUNT; i++ )
            mbedtls_sha256_starts( &ctx[i], 0 );

        memset( proc_len, 0, sizeof( proc_len ) );

        do
        {
            next = 0;

            for( i = 0; i < SHA256_CTX_COUNT; i++ )
            {
                /* Nothing left to do */
                if( proc_len[i] == sha256_ilens[i] )
                    continue;

                next = 1;

                next_len = sha256_ilens[i] / SPLIT + 1;
                next_len = ( next_len + proc_len[i] <= sha256_ilens[i] ) ? next_len :
                            sha256_ilens[i] - proc_len[i];
                if( proc_len[i] >= sha256_ilens[i] / 2 && !cloned[i] )
                {
                    mbedtls_sha256_clone( &cloned_ctx[i], &ctx[i] );
                    cloned[i] = 1;
                }
                mbedtls_sha256_update( ( cloned[i])? &cloned_ctx[i]:&ctx[i], sha256_inputs[i] + proc_len[i],
                                    next_len );
                    
                proc_len[i] += next_len;
            }
        }
        while( next == 1 );

        /* Verify the outputs */
        for( i = 0; i < SHA256_CTX_COUNT; i++ )
        {
            mbedtls_sha256_finish( cloned[i]? &cloned_ctx[i]:&ctx[i], output );

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

    ret = 0;

exit:
    for( i = 0; i < SHA256_CTX_COUNT; i++ )
        mbedtls_sha256_free( &ctx[i] );

    mbedtls_free( ctx );

    return( ret );
}

This test passes with the original implementation. So, I will remove the original commit, keeping the style fix, and latest commit for not supporting the internal process function

1. Change camelcase variables to Mbed OS style. 2. Remove functions declarations from the `_alt` header, since they are now added from the module header regardless whether an alternative implementation exists. 3. Remove the `extern "c"` declaration from the `_alt` headers. 4. Remove redundant extra lines.

Return the `MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED` for the internal process function, as it is not public.

RonEld · 2018-11-28T13:41:30Z

@yanesca As mentioned in previous comment, I have rebased to resolved conflicts, and removed the first commit that is not needed.
A reference branch from before the rebase is in https://github.com/RonEld/mbed-os/tree/cryptocell_sha_alt_fixes_before_rebase

cmonr · 2018-11-29T16:33:04Z

@yanesca Please re-review.
@ARMmbed/mbed-os-tls Please review.

yanesca · 2018-11-30T14:41:01Z

Looks good to me! I will approve formally as soon as @ARMmbed/mbed-os-tls has approved it too.

RonEld · 2018-12-09T06:41:28Z

@yanesca I don't think this should be reviewed by @ARMmbed/mbed-os-tls but by @ARMmbed/mbed-os-crypto as this is a crypto bug fix

dgreen-arm

Looks good to me.

RonEld · 2018-12-11T14:50:41Z

@cmonr @0xc0170 Could you start CI please?

cmonr · 2018-12-11T17:15:02Z

Will start CI when able.

We're wrapping up RC3 PRs right now.

cmonr · 2018-12-12T03:19:42Z

CI started

mbed-ci · 2018-12-12T07:45:17Z

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 4
Build artifacts

cmonr requested a review from a team November 13, 2018 15:11

cmonr added the needs: review label Nov 13, 2018

0xc0170 approved these changes Nov 15, 2018

View reviewed changes

cmonr requested a review from a team November 22, 2018 02:22

RonEld mentioned this pull request Nov 22, 2018

Fix issues in Cryptocell 310 ccm_alt discovered by On Target Testing #8704

Merged

yanesca suggested changes Nov 22, 2018

View reviewed changes

RonEld mentioned this pull request Nov 25, 2018

test_suite_md in mbedtls_md_process() violates the md API Mbed-TLS/mbedtls#2227

Closed

RonEld added needs: work and removed needs: review labels Nov 26, 2018

cmonr added the release-version: 5.11.1 label Nov 26, 2018

Ron Eldor added 2 commits November 28, 2018 15:31

Return not supported for internal process function

bcbda3e

Return the `MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED` for the internal process function, as it is not public.

RonEld force-pushed the cryptocell_sha_alt_fixes branch from b04800b to bcbda3e Compare November 28, 2018 13:38

RonEld added needs: review and removed needs: work labels Nov 28, 2018

dgreen-arm approved these changes Dec 11, 2018

View reviewed changes

yanesca approved these changes Dec 11, 2018

View reviewed changes

cmonr added needs: CI and removed needs: review labels Dec 11, 2018

0xc0170 added ready for merge and removed needs: CI labels Dec 12, 2018

0xc0170 merged commit 985afb8 into ARMmbed:master Dec 12, 2018

0xc0170 removed the ready for merge label Dec 12, 2018

RonEld mentioned this pull request Jul 21, 2019

Port CC 310 sha 512 driver #10913

Merged

Fix issues in Cryptocell 310 shax_alt discovered by On Target Testing #8728

Fix issues in Cryptocell 310 shax_alt discovered by On Target Testing #8728

Uh oh!

Conversation

RonEld commented Nov 13, 2018

Description

Pull request type

Uh oh!

0xc0170 commented Nov 15, 2018

Uh oh!

RonEld commented Nov 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmonr commented Nov 22, 2018

Uh oh!

0xc0170 commented Nov 22, 2018

Uh oh!

mbed-ci commented Nov 22, 2018

Uh oh!

0xc0170 commented Nov 22, 2018

Uh oh!

yanesca left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RonEld commented Nov 22, 2018

Uh oh!

yanesca commented Nov 22, 2018

Uh oh!

RonEld commented Nov 22, 2018

Uh oh!

yanesca commented Nov 22, 2018

Uh oh!

RonEld commented Nov 22, 2018

Uh oh!

RonEld commented Nov 28, 2018

Uh oh!

RonEld commented Nov 28, 2018

Uh oh!

RonEld commented Nov 28, 2018

Uh oh!

cmonr commented Nov 29, 2018

Uh oh!

yanesca commented Nov 30, 2018

Uh oh!

RonEld commented Dec 9, 2018

Uh oh!

dgreen-arm left a comment

Choose a reason for hiding this comment

Uh oh!

RonEld commented Dec 11, 2018

Uh oh!

cmonr commented Dec 11, 2018

Uh oh!

cmonr commented Dec 12, 2018

Uh oh!

mbed-ci commented Dec 12, 2018

Uh oh!

Uh oh!

RonEld commented Nov 15, 2018 •

edited

Loading