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

MBEDTLS_HAVE_ASM checks in check_config.h fire up on the wrong platforms #7236

Open
gilles-peskine-arm opened this issue Mar 9, 2023 · 0 comments
Labels
bug component-platform Portability layer and build scripts size-s Estimated task size: small (~2d)

Comments

@gilles-peskine-arm
Copy link
Contributor

In check_config.h, we have the following checks to error out if a requested library feature requires assembly code but the user has declared that the compiler doesn't support inline assembly:

#if defined(MBEDTLS_AESNI_C) && !defined(MBEDTLS_HAVE_ASM)
#error "MBEDTLS_AESNI_C defined, but not all prerequisites"
#endif

#if defined(MBEDTLS_AESCE_C) && !defined(MBEDTLS_HAVE_ASM)
#error "MBEDTLS_AESCE_C defined, but not all prerequisites"
#endif

#if defined(MBEDTLS_PADLOCK_C) && !defined(MBEDTLS_HAVE_ASM)
#error "MBEDTLS_PADLOCK_C defined, but not all prerequisites"
#endif

These checks are overly strict: MBEDTLS_AESNI_C, MBEDTLS_AESCE_C and MBEDTLS_PADLOCK_C have no effect when building for a target CPU architecture that doesn't support those features. More precisely, the corresponding module is empty on other architectures. For example, if you build for arm, you get the exact same binary (apart from mbedtls_strerror) whether MBEDTLS_AESNI_C is enabled or not; but if you disable MBEDTLS_HAVE_ASM, the build fails for no good reason.

This was raised in #7234 for AESCE when building on x86. It's in fact a preexisting issue with AESNI and Padlock when building on non-x86.

As a fix, we should only perform this check if the feature actually gets built. This means either:

  • Gate the check in check_config.h on whether the feature actually gets built. This requires using the symbols MBEDTLS_HAVE_X86, MBEDTLS_HAVE_X86_64, MBEDTLS_HAVE_ARM64 (3.4+ only), which are currently defined in private headers in 3.x and in the respective public headers (aesni.h, padlock.h) in 2.28.
  • Or move those checks into the library source file (aesce.c, aesni.c, padlock.c). This would avoid messing with publicly exposed symbols and would ensure that there's no inconsistency. I don't see any reason not to do that even in 2.28.

On a very related note, in aesni.c, there is a check that the configuration is consistent with build settings:

#if defined(__has_feature)
#if __has_feature(memory_sanitizer)
#warning \
    "MBEDTLS_AESNI_C is known to cause spurious error reports with some memory >
#endif
#endif

This check is in the wrong place: it should be gated by MBEDTLS_HAVE_X86_64.

Definition of done:

  • A build with MBEDTLS_AESNI_C for a non-x86 target does not error out even if MBEDTLS_HAVE_ASM is disabled.
  • A build with MBEDTLS_AESNI_C for a non-x86 target does not error out even when using Msan with GCC or Clang.
  • A build with MBEDTLS_PADLOCK_C for a non-x86 target does not error out even if MBEDTLS_HAVE_ASM is disabled.
@gilles-peskine-arm gilles-peskine-arm added bug component-platform Portability layer and build scripts size-s Estimated task size: small (~2d) labels Mar 9, 2023
paul-elliott-arm pushed a commit that referenced this issue Mar 28, 2023
The warning is only correct if the assembly code for AESNI is built, not if
MBEDTLS_AESNI_C is activated but MBEDTLS_HAVE_ASM is disabled or the target
architecture isn't x86_64.

This is a partial fix for #7236.

Signed-off-by: Gilles Peskine <[email protected]>
paul-elliott-arm pushed a commit that referenced this issue Mar 28, 2023
The warning is only correct if the assembly code for AESNI is built, not if
MBEDTLS_AESNI_C is activated but MBEDTLS_HAVE_ASM is disabled or the target
architecture isn't x86_64.

This is a partial fix for #7236.

Signed-off-by: Gilles Peskine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug component-platform Portability layer and build scripts size-s Estimated task size: small (~2d)
Projects
None yet
Development

No branches or pull requests

1 participant