Skip to content

AES ARM32 and Thumb2 ASM: fixup ARM32 and add Thumb2#6731

Merged
JacobBarthelmeh merged 3 commits intowolfSSL:masterfrom
SparkiDev:aes_arm32_thumb2
Sep 1, 2023
Merged

AES ARM32 and Thumb2 ASM: fixup ARM32 and add Thumb2#6731
JacobBarthelmeh merged 3 commits intowolfSSL:masterfrom
SparkiDev:aes_arm32_thumb2

Conversation

@SparkiDev
Copy link
Copy Markdown
Contributor

Description

Fix which functions and data are compiled in depending on defines.
Better handing of constants.

Testing

Regression tested ARM32 and Thumb2 assembly including inline.

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

@SparkiDev SparkiDev self-assigned this Aug 25, 2023
@dgarske
Copy link
Copy Markdown
Member

dgarske commented Aug 25, 2023

@SparkiDev

/tmp/workspace/PRB-multi-test-script/wolfssl/wolfcrypt/src/port/arm/armv8-aes.c:6312:39: warning: ISO C requires a translation unit to contain at least one declaration [clang-diagnostic-empty-translation-unit]
#endif /* !NO_AES && WOLFSSL_ARMASM */
^

@douzzer
Copy link
Copy Markdown
Contributor

douzzer commented Aug 25, 2023

Detected by the nightly on master, and still present in this PR:

failed: cross-armv7a-all-armasm-testsuite cross-armv7a-armasm-fips-140-3-ready-sp-all-testsuite-sanitizer cross-armv7a-all-armasm-testwolfcrypt-sanitizer clang-tidy-all-intelasm

Must be from 36b92a4.

In this PR, rebased on current master, the errors are:

AES      test failed!
 error L=10775
 [fiducial line numbers: 7836 23987 35081 47260]

================================================================================
testsuite.test for scenario cross-armv7a-all-armasm-testsuite exited with status 255.
AES      test failed!
 error L=10751 code=-204 (AES Known Answer Test check FIPS error)
 [fiducial line numbers: 7836 23987 35081 47260]

    cross-armv7a-armasm-fips-140-3-ready-sp-all-testsuite-sanitizer fail_analytic_check
AES      test failed!
 error L=10775
 [fiducial line numbers: 7836 23987 35081 47260]
Exiting main with return code: -1

    cross-armv7a-all-armasm-testwolfcrypt-sanitizer fail_analytic_check
[clang-tidy-all-intelasm] [4 of 4] [d00a0a2b2a]
    configure...   real 0m19.914s  user 0m10.420s  sys 0m10.886s
    build...1 warning generated.
6fd623248e (<david@wolfssl.com> 2019-07-18 06:49:36 -0700 6312) #endif /* !NO_AES && WOLFSSL_ARMASM */
/tmp/tmp.4346_10542/wolfssl_test_workdir.31004/wolfssl/wolfcrypt/src/port/arm/armv8-aes.c:6312:39: warning: ISO C requires a translation unit to contain at least one declaration [clang-diagnostic-empty-translation-unit]
6312 | #endif /* !NO_AES && WOLFSSL_ARMASM */
|                                       ^
make[2]: *** [Makefile:6210: wolfcrypt/src/port/arm/src_libwolfssl_la-armv8-aes.lo] Error 1
make[2]: *** Waiting for unfinished jobs....
36b92a4cef (<sean@wolfssl.com> 2023-07-13 17:24:36 +1000 118) #define ge_p3_dbl(r, p)     ge_p2_dbl((ge_p1p1 *)r, (ge_p2 *)p)
./wolfssl/wolfcrypt/ge_operations.h:118:50: warning: macro argument should be enclosed in parentheses [bugprone-macro-parentheses]
118 | #define ge_p3_dbl(r, p)     ge_p2_dbl((ge_p1p1 *)r, (ge_p2 *)p)
|                                                  ^
      | 
[and 36 more like that]
    clang-tidy-all-intelasm fail_analytic_build

The config for cross-armv7a-all-armasm-testsuite:

'--enable-all' '--enable-testcert' 'CPPFLAGS=-DNO_WOLFSSL_CIPHER_SUITE_TEST -DWOLFSSL_OLD_PRIME_CHECK -pedantic' '--enable-asn=template' '--enable-armasm' '--disable-chacha' '--disable-poly1305' '--disable-xchacha' '--disable-curve25519' '--disable-ed25519' '--disable-ed25519-stream' '--disable-curve448' '--disable-ed448' '--disable-ed448-stream' '--disable-openssh' '--host=armv7a-unknown-linux-gnueabihf' 'FILECMD=file' 'MANIFEST_TOOL=/bin/false' 'DLLTOOL=/bin/false'

@SparkiDev
Copy link
Copy Markdown
Contributor Author

AES works for me cross-compiling for ARMv7a.
Which version of the compiler are you using?

@SparkiDev SparkiDev force-pushed the aes_arm32_thumb2 branch 2 times, most recently from b1f8a22 to d786674 Compare August 27, 2023 22:42
@douzzer
Copy link
Copy Markdown
Contributor

douzzer commented Aug 28, 2023

for all ARM variants, gcc-12.3.1_p20230623.

@douzzer
Copy link
Copy Markdown
Contributor

douzzer commented Aug 28, 2023

retried with gcc-13.2.0. same result:

[cross-armv7a-all-armasm-testsuite] [1 of 1] [d00a0a2b2a]
    using armv7a-unknown-linux-gnueabihf-gcc 13.2.0
    autogen.sh d00a0a2b2a...   real 0m15.909s  user 0m14.008s  sys 0m1.037s
    configure...   real 0m14.481s  user 0m7.559s  sys 0m8.136s
    build.../usr/libexec/gcc/armv7a-unknown-linux-gnueabihf/ld: warning: creating DT_TEXTREL in a shared object
   real 0m54.116s  user 2m39.239s  sys 0m5.974s
    testsuite.test...   real 0m0.705s  user 0m0.689s  sys 0m0.016s
testsuite.test for scenario cross-armv7a-all-armasm-testsuite exited with status 255.
================================================================================
[...]
DES3     test passed!
AES      test failed!
 error L=10775
 [fiducial line numbers: 7836 23987 35081 47260]

Maybe qemu is the variable:

qemu-arm version 8.0.4
Copyright (c) 2003-2022 Fabrice Bellard and the QEMU Project developers

@douzzer
Copy link
Copy Markdown
Contributor

douzzer commented Aug 28, 2023

Upgraded to qemu 8.1.0 here this morning, and reran under that. Same result.

Bear in mind these tests were all succeeding until 36b92a4.

@SparkiDev SparkiDev force-pushed the aes_arm32_thumb2 branch 9 times, most recently from 8361ee8 to 1690e1f Compare August 30, 2023 01:20
Fix which functions and data are compiled in depending on defines.
Better handing of constants.
Also fix Aarch64 ed25519 inline assembly.
@SparkiDev SparkiDev force-pushed the aes_arm32_thumb2 branch 2 times, most recently from 62e51f6 to a0ba5d8 Compare September 1, 2023 01:41
@douzzer
Copy link
Copy Markdown
Contributor

douzzer commented Sep 1, 2023

Jenkins, retest this please.

1 similar comment
@douzzer
Copy link
Copy Markdown
Contributor

douzzer commented Sep 1, 2023

Jenkins, retest this please.

@JacobBarthelmeh JacobBarthelmeh merged commit 0352b38 into wolfSSL:master Sep 1, 2023
@markhermeling
Copy link
Copy Markdown

I have a static analysis finding in CodeSonar that popped up due to this PR. Screenshot attached.

I have to admit, I do not understand the logic. line 1015 in wolfcrypt/src/ed25519.c passes in in+1+ED25519_PUB_KEY_SIZE. Problem is that in points to a buffer of ED25519_PUB_KEY_SIZE, so by definition that pointer is outside of the buffer and then used by XMEMCPY.

SARIF also attached, this you can load into VS Code to see the flow.

2283404.sarif.zip
Screenshot 2023-09-05 at 8 21 48 PM

@SparkiDev
Copy link
Copy Markdown
Contributor Author

Hi @markhermeling

Thank you for letting us know about this so quickly!
I have a new PR that contains a fix for this: #6750

Let us know if it is not fixed in the PR.

Thanks,
Sean :-)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants