8e464c4 breaks clang 12 -O2 (make test failed) #4786

Benau · 2021-07-18T05:47:41Z

Summary

seems that 8e464c4 (bisected) confused clang 12 when you compile with -O2....

System information

Mbed TLS version (number or commit id): 3.0.0 or 2.27 branch
Operating system and version: archlinux x86_64 with clang 12, happens with llvm-mingw (windows cross-compiler) too
Configuration (if not default, please attach mbedtls_config.h): default
Compiler and options (if you used a pre-built binary, please indicate how you obtained it):
Additional environment information:

Expected behavior

make test succeeded

Actual behavior

make test failed

Steps to reproduce

CC=clang cmake .. -DCMAKE_BUILD_TYPE=Release
make -j14
make -j14 test

Additional information

The text was updated successfully, but these errors were encountered:

gilles-peskine-arm · 2021-07-18T14:39:11Z

I can reproduce the failure with Ubuntu clang version 12.0.1-++20210630032618+fed41342a82f-1~~exp1~~20210630133332.127 from the LLVM apt repository, with Ubuntu clang version 12.0.0-1ubuntu1 from Ubuntu 21.04, and withUbuntu clang version 13.0.0-++20210717052618+5df48493f089-1~~exp1~~20210717153411.478 from the LLVM apt repository, all on x86_64 and compiling with -O2. I can't reproduce it with Clang 11, with -O1 or for x86_32.

Thanks for doing the bisection. I'm staring at this patch and I really can't see how it might cause undefined behavior. So this is looking like a compiler bug.

Based on the logs from test_suite_mpi, it seems that mpi_mul_hlp works correctly when called from mbedtls_mpi_mul_mpi or mbedtls_mpi_mul_int, but not when called by mpi_montmul, so exponentiation calculates incorrect result (and then of course so do things that depend on it).

Base test mbedtls_mpi_exp_mod #1 .................................. FAILED
  mbedtls_mpi_cmp_mpi( &Z, &X ) == 0
  at line 1009, ../source/tests/suites/test_suite_mpi.function
Base test mbedtls_mpi_exp_mod #2 (Even N) ......................... PASS
Base test mbedtls_mpi_exp_mod #2 (N = 0 (null)) ................... PASS
Base test mbedtls_mpi_exp_mod #3 (Negative N) ..................... PASS
Base test mbedtls_mpi_exp_mod #4 (Negative base) .................. FAILED
  mbedtls_mpi_cmp_mpi( &Z, &X ) == 0
  at line 1009, ../source/tests/suites/test_suite_mpi.function
Base test mbedtls_mpi_exp_mod #5 (Negative exponent) .............. PASS
Base test mbedtls_mpi_exp_mod #6 (Negative base + exponent) ....... PASS
Test mbedtls_mpi_exp_mod: 0 (null) ^ 0 (null) mod 9 ............... FAILED
  mbedtls_mpi_cmp_mpi( &Z, &X ) == 0
  at line 1009, ../source/tests/suites/test_suite_mpi.function
Test mbedtls_mpi_exp_mod: 0 (null) ^ 0 (1 limb) mod 9 ............. FAILED
  mbedtls_mpi_cmp_mpi( &Z, &X ) == 0
  at line 1009, ../source/tests/suites/test_suite_mpi.function
Test mbedtls_mpi_exp_mod: 0 (null) ^ 1 mod 9 ...................... PASS
Test mbedtls_mpi_exp_mod: 0 (null) ^ 2 mod 9 ...................... PASS
Test mbedtls_mpi_exp_mod: 0 (1 limb) ^ 0 (null) mod 9 ............. FAILED
  mbedtls_mpi_cmp_mpi( &Z, &X ) == 0
  at line 1009, ../source/tests/suites/test_suite_mpi.function
Test mbedtls_mpi_exp_mod: 0 (1 limb) ^ 0 (1 limb) mod 9 ........... FAILED
  mbedtls_mpi_cmp_mpi( &Z, &X ) == 0
  at line 1009, ../source/tests/suites/test_suite_mpi.function
Test mbedtls_mpi_exp_mod: 0 (1 limb) ^ 1 mod 9 .................... PASS
Test mbedtls_mpi_exp_mod: 0 (1 limb) ^ 2 mod 9 .................... PASS
Test mbedtls_mpi_exp_mod: 1 ^ 0 (null) mod 9 ...................... FAILED
  mbedtls_mpi_cmp_mpi( &Z, &X ) == 0
  at line 1009, ../source/tests/suites/test_suite_mpi.function
Test mbedtls_mpi_exp_mod: 4 ^ 0 (null) mod 9 ...................... FAILED
  mbedtls_mpi_cmp_mpi( &Z, &X ) == 0
  at line 1009, ../source/tests/suites/test_suite_mpi.function
Test mbedtls_mpi_exp_mod: 10 ^ 0 (null) mod 9 ..................... FAILED
  mbedtls_mpi_cmp_mpi( &Z, &X ) == 0
  at line 1009, ../source/tests/suites/test_suite_mpi.function
Test mbedtls_mpi_exp_mod: 1 ^ 0 (1 limb) mod 9 .................... FAILED
  mbedtls_mpi_cmp_mpi( &Z, &X ) == 0
  at line 1009, ../source/tests/suites/test_suite_mpi.function
Test mbedtls_mpi_exp_mod: 4 ^ 0 (1 limb) mod 9 .................... FAILED
  mbedtls_mpi_cmp_mpi( &Z, &X ) == 0
  at line 1009, ../source/tests/suites/test_suite_mpi.function
Test mbedtls_mpi_exp_mod: 10 ^ 0 (1 limb) mod 9 ................... FAILED
  mbedtls_mpi_cmp_mpi( &Z, &X ) == 0
  at line 1009, ../source/tests/suites/test_suite_mpi.function
Test mbedtls_mpi_exp_mod: MAX_SIZE exponent ....................... PASS
Test mbedtls_mpi_exp_mod: MAX_SIZE + 1 exponent ................... PASS
Test mbedtls_mpi_exp_mod: MAX_SIZE modulus ........................ PASS
Test mbedtls_mpi_exp_mod: MAX_SIZE + 1 modulus .................... PASS
Test mbedtls_mpi_exp_mod: MAX_SIZE exponent and modulus ........... PASS
Test mbedtls_mpi_exp_mod: MAX_SIZE + 1 exponent and modulus ....... PASS
Test mbedtls_mpi_exp_mod #1 ....................................... FAILED
  mbedtls_mpi_cmp_mpi( &Z, &X ) == 0
  at line 1009, ../source/tests/suites/test_suite_mpi.function
Test mbedtls_mpi_exp_mod (Negative base) [#1] ..................... FAILED
  mbedtls_mpi_cmp_mpi( &Z, &X ) == 0
  at line 1009, ../source/tests/suites/test_suite_mpi.function
Test mbedtls_mpi_exp_mod (Negative base) [#2] ..................... FAILED
  mbedtls_mpi_cmp_mpi( &Z, &X ) == 0
  at line 1009, ../source/tests/suites/test_suite_mpi.function
…
Base test mbedtls_mpi_is_prime #1 ................................. PASS
Base test mbedtls_mpi_is_prime #2 ................................. PASS
Base test mbedtls_mpi_is_prime #3 ................................. PASS
Base test mbedtls_mpi_is_prime #4 ................................. PASS
Base test mbedtls_mpi_is_prime #5 ................................. PASS
Base test mbedtls_mpi_is_prime #6 ................................. PASS
Base test mbedtls_mpi_is_prime #7 ................................. PASS
Base test mbedtls_mpi_is_prime #8 ................................. PASS
Test mbedtls_mpi_is_prime #1a ..................................... PASS
Test mbedtls_mpi_is_prime #1b ..................................... PASS
Test mbedtls_mpi_is_prime #2a ..................................... FAILED
  res == div_result
  at line 1104, ../source/tests/suites/test_suite_mpi.function
Test mbedtls_mpi_is_prime #2b ..................................... PASS
Test mbedtls_mpi_is_prime #3 ...................................... FAILED
  res == div_result
  at line 1104, ../source/tests/suites/test_suite_mpi.function
Test mbedtls_mpi_is_prime #4 ...................................... FAILED
  res == div_result
  at line 1104, ../source/tests/suites/test_suite_mpi.function
Test mbedtls_mpi_is_prime #5 [#1] ................................. FAILED
  res == div_result
  at line 1104, ../source/tests/suites/test_suite_mpi.function
Test mbedtls_mpi_is_prime #5 [#2] ................................. FAILED
  res == div_result
  at line 1104, ../source/tests/suites/test_suite_mpi.function
Test mbedtls_mpi_is_prime #6 ...................................... FAILED
  res == div_result
  at line 1104, ../source/tests/suites/test_suite_mpi.function
Test mbedtls_mpi_is_prime #7 ...................................... FAILED
  res == div_result
  at line 1104, ../source/tests/suites/test_suite_mpi.function
Test mbedtls_mpi_is_prime #8 ...................................... FAILED
  res == div_result
  at line 1104, ../source/tests/suites/test_suite_mpi.function
Test mbedtls_mpi_is_prime #9 ...................................... FAILED
  res == div_result
  at line 1104, ../source/tests/suites/test_suite_mpi.function
Test mbedtls_mpi_is_prime #10 ..................................... FAILED
  res == div_result
  at line 1104, ../source/tests/suites/test_suite_mpi.function
Test mbedtls_mpi_is_prime #11 ..................................... FAILED
  res == div_result
  at line 1104, ../source/tests/suites/test_suite_mpi.function
Test mbedtls_mpi_is_prime #12 ..................................... FAILED
  res == div_result
  at line 1104, ../source/tests/suites/test_suite_mpi.function
Test mbedtls_mpi_is_prime #13 ..................................... FAILED
  res == div_result
  at line 1104, ../source/tests/suites/test_suite_mpi.function
Test mbedtls_mpi_is_prime #14 ..................................... FAILED
  res == div_result
  at line 1104, ../source/tests/suites/test_suite_mpi.function
Test mbedtls_mpi_is_prime #15 ..................................... FAILED
  res == div_result
  at line 1104, ../source/tests/suites/test_suite_mpi.function
Test mbedtls_mpi_is_prime #16 ..................................... FAILED
  res == div_result
  at line 1104, ../source/tests/suites/test_suite_mpi.function
Test mbedtls_mpi_is_prime #17 ..................................... FAILED
  res == div_result
  at line 1104, ../source/tests/suites/test_suite_mpi.function
Test mbedtls_mpi_is_prime #18 ..................................... FAILED
  res == div_result
  at line 1104, ../source/tests/suites/test_suite_mpi.function
Test mbedtls_mpi_is_prime #19 ..................................... FAILED
  res == div_result
  at line 1104, ../source/tests/suites/test_suite_mpi.function
Test mbedtls_mpi_is_prime #20 ..................................... FAILED
  res == div_result
  at line 1104, ../source/tests/suites/test_suite_mpi.function
Test mbedtls_mpi_is_prime_det (4 non-witnesses) ................... FAILED
  res == 0
  at line 1128, ../source/tests/suites/test_suite_mpi.function
Test mbedtls_mpi_is_prime_det (39 non-witnesses) .................. FAILED
  res == 0
  at line 1128, ../source/tests/suites/test_suite_mpi.function
Test mbedtls_mpi_gen_prime (Too small) ............................ PASS
Test mbedtls_mpi_gen_prime (OK, minimum size) ..................... PASS
Test mbedtls_mpi_gen_prime (corner case limb size -1 bits) ........ ^Cmake: *** [Makefile:17090: tests/test_suite_mpi.run] Interrupt

gilles-peskine-arm · 2021-07-18T14:41:41Z

The easy fix is to revert the patch and we'll probably do that, but I want to 1. make sure this is indeed a compiler bug and not undefined behavior in our code, and 2. if so be a good citizen and make sure the Clang bug is reported.

davidhorstmann-arm · 2021-07-22T08:24:52Z

I couldn't reproduce this on clang 13.0.0, I'll try version 12 shortly.

gilles-peskine-arm · 2021-07-22T08:30:19Z

@davidhorstmann-arm Which build of Clang 13 was that? It's a development version, so maybe the bug has been fixed recently.

davidhorstmann-arm · 2021-07-22T08:35:28Z

I built it from source - the llvm-project main branch, so should have all of the latest fixes.

gilles-peskine-arm · 2021-07-22T09:21:26Z

Still failing with Ubuntu clang version 13.0.0-++20210721052618+968899ad9cf1-1~~exp1~~20210721153410.482, i.e. the latest official nightly for Ubuntu.

ronald-cron-arm · 2021-07-23T07:36:29Z

I think we should investigate completely what is going on here. I can see that the function mpi_mul_hlp() had already some weird issue with some compiler:

#if defined(__APPLE__) && defined(__arm__)                                      
/*                                                                              
 * Apple LLVM version 4.2 (clang-425.0.24) (based on LLVM 3.2svn)               
 * appears to need this to prevent bad ARM code generation at -O3.              
 */                                                                             
__attribute__ ((noinline))                                                      
#endif

Furthermore, this function uses assembly code which is probably very difficult to make right especially for multiple compiler versions, compilation options ...

davidhorstmann-arm · 2021-09-06T11:28:52Z

Apologies for the delay - I can confirm that I've reproduced the tests hanging on clang-12 with the following exact version:

$ clang --version
clang version 12.0.1 (https://github.com/llvm/llvm-project.git fed41342a82f5a3a9201819a82bf7a48313e296b)

The tests hang on 53/89:

...

50/89 Test #50: md-suite ...................................   Passed    0.00 sec
      Start 51: mdx-suite
51/89 Test #51: mdx-suite ..................................   Passed    0.00 sec
      Start 52: memory_buffer_alloc-suite
52/89 Test #52: memory_buffer_alloc-suite ..................   Passed    0.00 sec
      Start 53: mpi-suite
(No further output)

davidhorstmann-arm · 2021-09-06T11:51:00Z

I'm not sure exactly what happened previously (I may have forgotten to remove some state), but I can now reproduce the problem with clang-13. Specifically:

$ clang --version
clang version 13.0.0 (https://github.com/llvm/llvm-project.git 3396377743939f35856b2ee4ac47d64270e822f1)

One small note: to get this to compile I had to modify library/psa_crypto_cipher.c because of an unused parameter warning (it would seem that clang-12 doesn't mind about this, but clang-13 takes issue with it).

davidhorstmann-arm · 2021-09-06T12:10:50Z

As @ronald-cron-arm pointed out, there is already some code generation issue. When I commented out the #if and #endif to make the following:

/*#if defined(__APPLE__) && defined(__arm__)*/
/*
 * Apple LLVM version 4.2 (clang-425.0.24) (based on LLVM 3.2svn)
 * appears to need this to prevent bad ARM code generation at -O3.
 */
__attribute__ ((noinline))
/*#endif*/

(i.e. making the __attribute__ ((noinline)) unconditional) the tests no longer failed.

gilles-peskine-arm · 2021-09-13T21:57:33Z

Given that this bug only arises when mpi_mul_hlp calls assembly code, before concluding that this is a compiler bug, we need to carefully review whether we're declaring the behavior of the assembly code correctly.

I think we are declaring register usage correctly: the code uses 7 registers:

%%r8 (written): clobber "r8"
%%rax (written): clobber "rax"
%%rbx (read): input "b"
%%rcx (read and written): output "+c"
%%rdi (read and written): constraint "+D"
%%rdx (written): constraint "rdx"
%%rsi (read and written): constraint "+S"

However, there are no constraints on memory. Shouldn't there be? The code reads from *s, and reads and writes to *d. If I add the clobber "memory", the tests pass.

davidhorstmann-arm · 2021-09-14T11:58:26Z

I have been able to reproduce this result locally. Would it be worthwhile to see if this also removes the need for the previous __attribute__ ((noinline)) annotation from this commit?

gilles-peskine-arm · 2021-09-14T12:43:24Z

I only changed the x86_64 code, so My patch doesn't do anything about fc4f46f, but it may well be due to a similar bug. See #4943

Benau referenced this issue in supertuxkart/dependencies Jul 18, 2021

Revert back to MBed TLS to 2.26.0 because Windows doesn't work

b867fb5

gilles-peskine-arm added bug component-crypto Crypto primitives and low-level interfaces regression labels Jul 18, 2021

gilles-peskine-arm added the Product Backlog label Jul 18, 2021

gilles-peskine-arm mentioned this issue Sep 6, 2021

mbedTLS compiled with clang incorrectly rejects certificates (broken validation) #4917

Closed

daverodgman changed the title ~~8e464c407a1ce8b88412c6a8cc8aafa8d2cf1b0f breaks clang 12 -O2 (make test failed)~~ 8e464c4 breaks clang 12 -O2 (make test failed) Sep 13, 2021

gilles-peskine-arm mentioned this issue Sep 13, 2021

Backport 2.x: Fix x86_64 assembly for bignum multiplication #4941

Merged

gilles-peskine-arm added the fix available label Sep 13, 2021

gilles-peskine-arm mentioned this issue Sep 14, 2021

Review inline assembly for missing constraints and fix them #4943

Open

davidhorstmann-arm mentioned this issue Sep 14, 2021

Work around bad code generation with clang at -O2 #4945

Closed

4 tasks

davidhorstmann-arm linked a pull request Sep 14, 2021 that will close this issue

Backport 2.x: Fix x86_64 assembly for bignum multiplication #4941

Merged

gilles-peskine-arm mentioned this issue Sep 15, 2021

Fix x86_64 assembly for bignum multiplication #4947

Merged

This was linked to pull requests Sep 21, 2021

Fix x86_64 assembly for bignum multiplication #4947

Merged

Backport 2.16: Fix x86_64 assembly for bignum multiplication #4948

Merged

davidhorstmann-arm closed this as completed Sep 21, 2021

the-real-adammork mentioned this issue Nov 23, 2022

Patch for MbedTLS PR#4978 - fixes bignum multiplication for newer versions of clang (Xcode 13/14) mobilecoinfoundation/rust-mbedtls#66

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8e464c4 breaks clang 12 -O2 (make test failed) #4786

8e464c4 breaks clang 12 -O2 (make test failed) #4786

Benau commented Jul 18, 2021 •

edited

Loading

gilles-peskine-arm commented Jul 18, 2021 •

edited

Loading

gilles-peskine-arm commented Jul 18, 2021

davidhorstmann-arm commented Jul 22, 2021

gilles-peskine-arm commented Jul 22, 2021

davidhorstmann-arm commented Jul 22, 2021

gilles-peskine-arm commented Jul 22, 2021

ronald-cron-arm commented Jul 23, 2021 •

edited

Loading

davidhorstmann-arm commented Sep 6, 2021

davidhorstmann-arm commented Sep 6, 2021

davidhorstmann-arm commented Sep 6, 2021

gilles-peskine-arm commented Sep 13, 2021 •

edited

Loading

davidhorstmann-arm commented Sep 14, 2021

gilles-peskine-arm commented Sep 14, 2021

8e464c4 breaks clang 12 -O2 (make test failed) #4786

8e464c4 breaks clang 12 -O2 (make test failed) #4786

Comments

Benau commented Jul 18, 2021 • edited Loading

Summary

System information

Expected behavior

Actual behavior

Steps to reproduce

Additional information

gilles-peskine-arm commented Jul 18, 2021 • edited Loading

gilles-peskine-arm commented Jul 18, 2021

davidhorstmann-arm commented Jul 22, 2021

gilles-peskine-arm commented Jul 22, 2021

davidhorstmann-arm commented Jul 22, 2021

gilles-peskine-arm commented Jul 22, 2021

ronald-cron-arm commented Jul 23, 2021 • edited Loading

davidhorstmann-arm commented Sep 6, 2021

davidhorstmann-arm commented Sep 6, 2021

davidhorstmann-arm commented Sep 6, 2021

gilles-peskine-arm commented Sep 13, 2021 • edited Loading

davidhorstmann-arm commented Sep 14, 2021

gilles-peskine-arm commented Sep 14, 2021

Benau commented Jul 18, 2021 •

edited

Loading

gilles-peskine-arm commented Jul 18, 2021 •

edited

Loading

ronald-cron-arm commented Jul 23, 2021 •

edited

Loading

gilles-peskine-arm commented Sep 13, 2021 •

edited

Loading