Skip to content
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

Warn if using a memory sanitizer on AESNI #1560

Merged
merged 1 commit into from
Apr 12, 2018

Conversation

gilles-peskine-arm
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm commented Apr 5, 2018

Clang-Msan is known to report spurious errors when MBEDTLS_AESNI_C is
enabled (which is the case in the default configuration), due to the use of assembly code. The error reports don't
mention AES, so they can be difficult to trace back to the use of
AES-NI. Warn about this potential problem at compile time.

This would make sense in backports too, but it has a risk of breaking somebody's build, so I lean towards not backporting.

Clang-Msan is known to report spurious errors when MBEDTLS_AESNI_C is
enabled, due to the use of assembly code. The error reports don't
mention AES, so they can be difficult to trace back to the use of
AES-NI. Warn about this potential problem at compile time.
@gilles-peskine-arm gilles-peskine-arm added enhancement mbed TLS team needs-review Every commit must be reviewed by at least two team members, labels Apr 5, 2018
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

Looks good to me.

We could probably add a similar test in padlock.c and perhaps other places as well, but aesni.c is the only place that is both enabled by default and known to cause issues, so I support starting with this.

@hanno-becker hanno-becker removed the needs-review Every commit must be reviewed by at least two team members, label Apr 9, 2018
@mpg
Copy link
Contributor

mpg commented Apr 10, 2018

Note: Jenkins only failed on known-flaky timing test (#1517), so not a blocker for merging.

@mpg mpg added the approved Design and code approved - may be waiting for CI or backports label Apr 10, 2018
@mpg mpg merged commit 5053efd into Mbed-TLS:development Apr 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants