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

mbedtls: STM32F439xI: Don't enable AES acceleration by default #4934

Merged
merged 1 commit into from Sep 4, 2017

Conversation

Projects
None yet
9 participants
@Patater
Contributor

Patater commented Aug 18, 2017

Critical workaround to issue #4928. This issue blocks releases until either the issue is fixed or this workaround is merged.

STM32F439xI-family AES hardware acceleration occasionally produces
incorrect output (#4928).

Don't enable AES HW acceleration on STM32F439xI-family targets by
default until issue #4928 is fixed.

Status

READY for REVIEW as WORKAROUND

  • @adustm to perform initial investigation of #4928
  • If a fix to #4928 is ready before the next patch release, this workaround PR will be discarded.

This is a workaround for issue #4928

mbedtls: STM32F439xI: Don't enable AES acceleration by default
STM32F439xI-family AES hardware acceleration occasionally produces
incorrect output (#4928).

Don't enable AES HW acceleration on STM32F439xI-family targets by
default until issue #4928 is fixed.

@Patater Patater requested review from andresag01 and adustm Aug 18, 2017

@adustm

This comment has been minimized.

Member

adustm commented Aug 18, 2017

Hello @Patater
Such a workaround has already been implemented yesterday by @andresag01 in ARMmbed/mbed-os-example-tls#112 directly.
I am working on the issue #4928 today.
Would you accept to wait before this PR is merged ? We may not even need it...

@Patater

This comment has been minimized.

Contributor

Patater commented Aug 18, 2017

Hi @adustm

The workaround implemented by @andresag01 yesterday was only for the Mbed TLS example. The issue is currently affecting all applications using AES on STM32F439xI-family targets.

I would much prefer issue #4928 fixed instead of this PR's workaround merged. Please keep us posted. We'll watch the issue.

Thanks

@adustm

This comment has been minimized.

Member

adustm commented Aug 18, 2017

Are you aware of any other use case that would fail ?
I'd be interested in a use case that isn't as deep as the mbedtls hanshake. It looks like the issue is timing dependant (adding breakpoint does not show the error log always at the same time).

@Patater

This comment has been minimized.

Contributor

Patater commented Aug 18, 2017

@adustm
We don't have any other test cases that reproduce this issue.

@andresag01

This comment has been minimized.

Contributor

andresag01 commented Aug 18, 2017

@adustm: As @Patater mentioned, we do not have any other test cases that reproduce this issue. However, @JanneKiiskila mentioned:

using the HW acceleration for crypto also breaks the SD-cards init

If this is indeed some timing or concurrency issue, you could try to reproduce the problem by exercising the accelerator in a therad and while simultaneously spinning other threads that use IO such as ethernet or the sd card. Again, I am not sure this will reproduce the issue, but if it does its probably simpler to investigate than running the full TLS stack.

@andresag01

The changes look good to me. I think we should merge this for the next release only if the actual problem is not fixed by then.

@sg-

This comment has been minimized.

Member

sg- commented Aug 23, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 24, 2017

Result: ABORTED

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

/morph test

Output

mbed Build Number: 1098

Example Prep failed!

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Aug 24, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 24, 2017

Result: FAILURE

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

/morph test

Output

mbed Build Number: 1103

Build failed!

@adustm

adustm approved these changes Aug 25, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 29, 2017

@yanesca Could you review when you get the chance?

@yanesca

Looks good to me.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 31, 2017

As this was approved by both sides, this run CI and be ready for integration (if the fix does not come by then).

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Sep 1, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 1, 2017

Result: FAILURE

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

/morph test

Output

mbed Build Number: 1151

Build failed!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 1, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 1, 2017

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 1155

All builds and test passed!

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Sep 4, 2017

@0xc0170 0xc0170 merged commit fc4afab into ARMmbed:master Sep 4, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test 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

@0xc0170 0xc0170 removed the ready for merge label Sep 4, 2017

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