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

Tests failure with cryptopp 6.0.0 on ppc64le #588

Closed
kwizart opened this issue Feb 19, 2018 · 12 comments
Closed

Tests failure with cryptopp 6.0.0 on ppc64le #588

kwizart opened this issue Feb 19, 2018 · 12 comments
Labels

Comments

@kwizart
Copy link
Contributor

kwizart commented Feb 19, 2018

Crypto++ Issue Report

The cryptopp compilation succeeded once a little fix is made:

sed -i -e 's/-mcpu=power4//' GNUmakefile

Prevent an error at aria-simd.cpp compile:

type_traits:347:39: error: '__float128' was not declared in this scope

Here is the full log:

State the operating system and version (Ubutnu 17 x86_64, Windows 7 Professional x64, etc)

  • Fedora 28 (Rawhide: development branch) ppc64le
  • Others arches have succeeded.

State the version of the Crypto++ library (Crypto++ 5.6.5, Master, etc)

  • 6.0.0

State how you built the library (Makefile, Cmake, distro, etc)

  • GNUMakefile

Show a typical command line (the output of the compiler for cryptlib.cpp)

  • g++ -DNDEBUG -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mcpu=power8 -mtune=power8 -fstack-clash-protection -fPIC -DPIC -c cryptlib.cpp

Show the link command (the output of the linker for libcryptopp.so or cryptest.exe)

  • g++ -shared -Wl,-soname,libcryptopp.so.6 -o libcryptopp.so.6.0.0 -DNDEBUG -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mcpu=power8 -mtune=power8 -fstack-clash-protection -fPIC -DPIC -Wl,-z,relro -specs=/usr/lib/rpm/redhat/redhat-hardened-ld cryptlib.o cpu.o integer.o 3way.o adler32.o algebra.o algparam.o arc4.o aria-simd.o aria.o ariatab.o asn.o authenc.o base32.o base64.o basecode.o bfinit.o blake2-simd.o blake2.o blowfish.o blumshub.o camellia.o cast.o casts.o cbcmac.o ccm.o chacha.o channels.o cmac.o crc-simd.o crc.o default.o des.o dessp.o dh.o dh2.o dll.o dsa.o eax.o ec2n.o eccrypto.o ecp.o elgamal.o emsa2.o eprecomp.o esign.o files.o filters.o fips140.o fipstest.o gcm-simd.o gcm.o gf256.o gf2_32.o gf2n.o gfpcrypt.o gost.o gzip.o hex.o hmac.o hrtimer.o ida.o idea.o iterhash.o kalyna.o kalynatab.o keccak.o luc.o mars.o marss.o md2.o md4.o md5.o misc.o modes.o mqueue.o mqv.o nbtheory.o neon-simd.o network.o oaep.o osrng.o padlkrng.o panama.o pkcspad.o poly1305.o polynomi.o ppc-simd.o pssr.o pubkey.o queue.o rabin.o randpool.o rc2.o rc5.o rc6.o rdrand.o rdtables.o rijndael-simd.o rijndael.o ripemd.o rng.o rsa.o rw.o safer.o salsa.o seal.o seed.o serpent.o sha-simd.o sha.o sha3.o shacal2-simd.o shacal2.o shark.o sharkbox.o simon-simd.o simon.o skipjack.o sm3.o sm4.o socketft.o sosemanuk.o speck-simd.o speck.o square.o squaretb.o sse-simd.o strciphr.o tea.o tftables.o threefish.o tiger.o tigertab.o trdlocal.o ttmac.o tweetnacl.o twofish.o vmac.o wait.o wake.o whrlpool.o xtr.o xtrcrypt.o zdeflate.o zinflate.o zlib.o

Show the exact error message you are receiving (copy and paste it); or

Clearly state the undesired behavior (and state the expected behavior)

  • Tests have to succeed.
@noloader
Copy link
Collaborator

noloader commented Feb 19, 2018

Thanks @kwizart.

The log is interesting in a morbid sort of way. It looks like 4 or 5 algorithms failed (search for FAIL in all capitol letters):

FAILED   Poly1305 test set 0
FAILED   Poly1305 test set 1
FAILED   Poly1305 test set 2
...

Rijndael (AES) validation suite running...
passed:  Algorithm key lengths
FAILED   000102030405060708090A0B0C0D0E0F ...
FAILED   00010203050607080A0B0C0D0F101112 ...
FAILED   14151617191A1B1C1E1F202123242526 ...
...

VMAC validation suite running...
Testing MAC algorithm VMAC(AES)-64.
Test failed.
Skipping to next test.
...

We test on a ppc64le machine. It is gcc112.fsffrance.org from the compile farm.

A few of questions...

  1. What machine are you working on?

  2. Can I get access to the machine?

  3. Is Fedora 28 stable?

  4. In the bigger picture, how can I duplicate this?


Regarding (1), it looks like Power8. Below is the head of ./cryptest.exe v. I thought we had that case covered but it looks like we have gaps.

+ ./cryptest.exe v
Using seed: 1519063107      
Testing Settings...
passed:  Your machine is little endian.
passed:  Aligned data access (no CRYPTOPP_ALLOW_UNALIGNED_DATA_ACCESS).
passed:  sizeof(byte) == 1
passed:  sizeof(word16) == 2
passed:  sizeof(word32) == 4
passed:  sizeof(word64) == 8
passed:  sizeof(word128) == 16
passed:  sizeof(hword) == 4, sizeof(word) == 8, sizeof(dword) == 16
passed:  cacheLineSize == 128
passed:  hasAltivec == 1, hasPower7 == 1, hasPower8 == 1, hasAES == 1, hasSHA256 == 1, hasSHA512 == 1
No operating system provided blocking random number generator, skipping test.
Testing operating system provided nonblocking random number generator...
...

Regarding (4), how can I duplicate... Using the following command line does not result in a failure on gcc112. It is the command line from the log file less the Red Hat hardening because it is missing.

CXXFLAGS="-DNDEBUG -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -m64 -mcpu=power8 -mtune=power8 -fPIC -DPIC" make -j 12

@noloader noloader added the Bug label Feb 19, 2018
@kwizart
Copy link
Contributor Author

kwizart commented Feb 19, 2018

  • What machine are you working on?
    Here is the top level link for the task:
    https://koji.fedoraproject.org/koji/taskinfo?taskID=25167092
    At the end, there is an output hw_info.log . It basically says (it's a KVM guest):
    "POWER8 (architected), altivec supported"

  • Can I get access to the machine?
    I think fedora contributor (like me) can have some access to dedicated machine to reproduce an issue, but not that exact builder.

  • Is Fedora 28 stable?
    It depends, Fedora 28 is the development version, but is made of stable components along with components which stabilization is meant to progress ;).
    Actually, this ppc64le failure prevent me to introduce cryptopp 6.0.0 in fedora 28, and rebuild dependencies. As this is where this kind of improvement should be done...

At least, I can only reproduce with fedora 28, as fedora 27 doesn't have the issue:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25168306
Majors changes are:
glibc 2.27-3.fc28
gcc 8.0.1-0.14.fc28
etc.

  • In the bigger picture, how can I duplicate this?
    I can probably be reproduced with a gcc 8 toolchain on any distro. But that would still need to be confirmed...

@noloader
Copy link
Collaborator

noloader commented Feb 19, 2018

gcc 8.0.1-0.14.fc28

Ah, thanks. This explains a lot.


Actually, this ppc64le failure prevent me to introduce cryptopp 6.0.0 in fedora 28, and rebuild dependencies. As this is where this kind of improvement should be done...

Ack, thanks. Yeah, we want to work through these issues.


Removing -mcpu=power4 seems like it should be OK:

sed -i -e 's/-mcpu=power4//' GNUmakefile

It is needed for older compilers, like early GCC 4.x on a PowerMac G5 to help engage Altivec. Since you are at Power8 the minimum Altivec/Power4 should already be engaged.

I don't recall seeing this one before:

type_traits:347:39: error: '__float128' was not declared in this scope

It seems like a SIMD float should go missing without -mcpu=powerN. That is is present without -mcpu=powerN seems kind of odd to me. But take it with a grain of salt... I don't work with PPC64 and Power architecture often.


Actually, this ppc64le failure prevent me to introduce cryptopp 6.0.0 in fedora 28, and rebuild dependencies. As this is where this kind of improvement should be done...

OK, thanks.

For PPC64 only... does Fedora governance allow you to build with an additional define? If so, here is what I suggest until we can get to the bottom of it...

Open config.h, and around line 57 add the following for PPC64-build only:

+// Define this to disable ASM, intrinsics and built-ins. The code will be
+// compiled using C++ only. The library code will not include SSE2 (and
+// above), NEON, Aarch32, Aarch64, Power4, Power7 or Power8.
+ #define CRYPTOPP_DISABLE_ASM 1

Maybe something like:

sed -i '57i #define CRYPTOPP_DISABLE_ASM 1' config.h

We actually added that block to config.h several days ago (but ours is commented out). Also see Commit a0e217799614ae2c. It was added for cases like this to ensure the library and user programs are built with the same settings.


A quick heads up... We released Crypto++ 6.0 on January 22, 2018. On February 22 we are releasing Crypto++ 6.1. It is a planned release.

We found it is good housekeeping to (1) perform major release, and then (2) perform minor release 30 days later. The testing we hoped for before 6.0 was released always seems to happen after it is released. So 6.1 will be the rollup with the more complete testing and minor bug fixes. Effectively Crypto++ 6.1 is the long term release.

Crypto++ 6.1 is also going to fix this issue with Simon and Speck: Issue 585, Speck, Android and Linux kernel interop. Simon and Speck are block ciphers added at 6.0. The issue is fixed so we want to get the modified cipher into circulation. (We would also like to see the Simon and Speck team publish updated test vectors, but that is probably not going to happen).

By the way, feel free to email me at noloader, gmail account. It may be easier on you if you want email rather than bug reports.

noloader added a commit that referenced this issue Feb 20, 2018
The code below was flagged by undefined behavior santizier under GCC 8. The offender was the doubling at "r4 = vec_add(r4, r4)". R4 is rcon and an unsigned type. It depends on integer wrap but GCC is generating code that is being flagged for signed overflow. GCC 7 and below is OK.

   for (unsigned int i=0; i<8; ++i)
   {
      r1 = Rijndael_Subkey_POWER8(r1, r4, r5);
      r4 = vec_add(r4, r4);
      skptr = IncrementPointerAndStore(r1, skptr);
   }

   // Final two rounds using table lookup
   ...
noloader added a commit that referenced this issue Feb 20, 2018
This is due to the removal of a path in Rijndael_UncheckedSetKey_POWER8
@noloader
Copy link
Collaborator

noloader commented Feb 20, 2018

@kwizart,

Good news... I am able to duplicate the issue with GCC 8 now. Thanks to Segher Boessenkool for building GCC 8 and installing on GCC112 for us.

Here is the baseline I used to recreate the issue on GCC112 with GCC 8. These are Fedora's options less the Red Hat spec files because GCC112 lacks them.

$ CXX=/opt/cfarm/gcc8-r257824/bin/g++ CXXFLAGS="-DNDEBUG -O2 -g -pipe -Wall -Werror=format-security \
-Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong \
-grecord-gcc-switches -m64 -mcpu=power8 -mtune=power8 -fstack-clash-protection -fPIC -DPIC" make -j 20

It results in the failure:

$ LD_LIBRARY_PATH=/opt/cfarm/gcc8-r257824/lib64/ ./cryptest.exe tv aes
Using seed: 1519113557

Testing SymmetricCipher algorithm AES/ECB.

incorrectly encrypted: 76EAA4A921B8C217FCCDDFAEC8A554E08025DBC564E7FC99A279122EB
664B0C05D011F8488B8F2FA00AD197B4E8D6A50136A39C12CB6DBCB1DD2DFBFE300F36B
AlgorithmType: SymmetricCipher
Ciphertext: 3ad77bb40d7a3660a89ecaf32466ef97 f5d3d58503b9699de785895a96fdbaaf 43
b1cd7f598ece23881b00e3ed030688 7b0c785e27e8ad3f8223207104725dd4
Comment: F.1.1 ECB-AES128.Encrypt
Key: 2b7e151628aed2a6abf7158809cf4f3c
Name: AES/ECB
Plaintext: 6bc1bee22e409f96e93d7e117393172a ae2d8a571e03ac9c9eb76fac45af8e51 30c
81c46a35ce411e5fbc1191a0a52ef f69f2445df4f9b17ad2b417be66c3710
Source: NIST Special Publication 800-38A
Test: Encrypt

Test FAILED.
...

Next, remove -fstack-protector-strong:

$ CXX=/opt/cfarm/gcc8-r257824/bin/g++ CXXFLAGS="-DNDEBUG -O2 -g -pipe -Wall -Werror=format-security \
-Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -grecord-gcc-switches \
-m64 -mcpu=power8 -mtune=power8 -fstack-clash-protection -fPIC -DPIC" make -j 20

And:

$ LD_LIBRARY_PATH=/opt/cfarm/gcc8-r257824/lib64/ ./cryptest.exe tv aes
Using seed: 1519113751

Testing SymmetricCipher algorithm AES/ECB.
....
Testing SymmetricCipher algorithm AES/CBC.
........
Testing SymmetricCipher algorithm AES/CFB.
.......
Testing SymmetricCipher algorithm AES/OFB.
....
Testing SymmetricCipher algorithm AES/CTR.
.............

Now here is the rub... Compiling with just CXXFLAGS="-DNDEBUG -g2 -O3 -fstack-protector-strong" does not witness the problem. Compiling with just CXXFLAGS="-DNDEBUG -g2 -O2 -fstack-protector-strong" does witness the problem. So the problem seems to be -fstack-protector-strong modulo -O2.

At this point I don't know how to take things further. I'm usually reliable up to about objdump --disassemble, but -fstack-protector-strong interactions are a little above my knowledge. I'm not sure how to spot a good application of -fstack-protector-strong versus a bad one.

I pinged the GCC-dev list to see what they have to say. Also see GCC 8, ppc64-le and problems with -fstack-protector-strong.

@noloader
Copy link
Collaborator

noloader commented Feb 20, 2018

@kwizart,

I've got a feeling things are going to stall at this point. At this point in time I think your options for GCC 8 on ppc64-le are:

  1. Add #define CRYPTOPP_DISABLE_ASM 1 to config.h around line 57 or so.
  2. Compile rijndael-simd.cpp with -O3.
  3. Do nothing and wait for GCC to help with the issue.

I advise you to select option (2) because AES builtins/hardware are hardened against many side channel attacks that are present in software-only implementations. And AES builtins run around 1.2 cycles-per-byte (cpb), so it is about 15x faster than software only using C++.

@kwizart
Copy link
Contributor Author

kwizart commented Feb 20, 2018

Confirmed to build and pass tests as appropriate.
(Using AES_CFLAG="-O3 -maltivec only for ppc64le).

I will try to test back to the default from time to time and report here if fixed.
Thx for the help and advices.

@noloader
Copy link
Collaborator

noloader commented Feb 20, 2018

Ack, thanks for the help.

We test the library using -O0 through -O5, -Os and -Ofast. You should be OK selecting any of them if it is easier to package that way.

Let's ping László Böszörményi (@gcsideal) to let him know GCC 8 may cause some troubles for him. He is a Debian packager and maintainer.

I'm going to close this out for the time being. Please drop a note when you notice -O2 working as expected. I'll do the same if the GCC devs move things forward.

@gcsideal
Copy link

Debian is not affected by for the time being. While we have a recent GCC 8 in the archives, it's not yet the default to be used. But thanks for the heads-up @noloader.

@noloader
Copy link
Collaborator

noloader commented Feb 20, 2018

@gcsideal, @kwizart,

I think I tracked the failure down to a One Definition Rule (ODR) violation in our Power8 code. It was not a GCC problem. Several functions in adv-simd.h were missing inline on the declaration. Also see Commit 33c10bc027b9.

Jonathan Wakely commented he did not think the inline change fixed anything. I tested the changes incrementally, and the testing seemed to show the inline declaration resolved the issue. Maybe we just side stepped/worked around a compiler issue.

Sorry about the troubles the issue caused.

noloader added a commit that referenced this issue Feb 21, 2018
@noloader
Copy link
Collaborator

noloader commented Feb 22, 2018

@kwizart, @gcsideal,

Crypto++ was just released. Release notes at Crypto++ 6.1.

@gcsideal
Copy link

gcsideal commented Feb 23, 2018

Seen and packaged it. Had to add -lstdc++ -lpthread -lm to libcryptopp_la_LIBADD in your Makefile.am but that's all I remember now. Not yet uploaded as I need to choose an other SONAME for the package transition - didn't decide yet if simply 1 would be the best. On the other hand, @kwizart already uploaded it to Fedora 28 and 29 releases as I see.

@noloader
Copy link
Collaborator

Great, thanks.

Had to add -lstdc++ -lpthread -lm to libcryptopp_la_LIBADD in your Makefile.am

That should be OK (I think). LDFLAGS includes -Wl,--as-needed, so unneeded libraries will be removed by the linker if unneeded. The Binutil folks recommend it, but I rarely see it used. (I'm pretty sure I read it on a binutil mailing list post due to problems with weak symbols and possibly unneeded -lm).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants