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

Add wolfcrypt test: R/O filesystem const memory pointer #6523

Merged
merged 1 commit into from
Jul 3, 2023

Conversation

gojimmypi
Copy link
Contributor

@gojimmypi gojimmypi commented Jun 19, 2023

Description

As noted in #6522, the wolfSSL wolfcrypt test currently fails duing a memory access operation on
a read-only embedded filesystem such as Xtensa Linux kernel on the ESP32-S3 when accessing
const var pointers.

This failure does not currently occur during the memory test. This PR fixes that with a new test.

With WOLFSSL_DEBUG turned on, the testwolfcrypt will now show the failure during the memory test.

Without this change, the memory test would pass, but the next test (base64_test()) would fail:

/opt # ./testwolfcrypt
Math:   Multi-Precision: Wolf(SP) word-size=32 bits=3072 sp_int.c
------------------------------------------------------------------------------
 ESP32-S3 Xtensa 1.0a wolfSSL version 5.6.3
----------------------[   16.551666] Caught unhandled exception in 'testwolfcrypt' (pid = 63, pc = 0x42e3c7e2) - should not happen
[   16.551666]  EXCCAUSE is 3
[   16.564861]  3a 37 32 03 00 e0 a0 34 3c f5 90 80 54 e0 60 54 90 95 c0 8a 87 6a 67 40 aa 20 d2 08 00 32 41 30
--------------------------------------------------------
error    test passed!
MEMORY   test passed!
Illegal instruction

Here's an example of the new failure message during the memory test:

~ # cd /opt
/opt # ./testwolfcrypt
Math:   Multi-Precision: Wolf(SP) word-size=32 bits=3072 sp_int.c
------------------------------------------------------------------------------
 wolfSSL version 5.6.3
------------------------[   16.738576] Caught unhandled exception in 'testwolfcrypt' (pid = 63, pc = 0x42e2a61a) - should not happen
[   16.738576]  EXCCAUSE is 3
[   16.751910]  20 00 a8 01 da 99 98 09 aa 99 92 09 00 c0 20 00 92 41 08 86 05 00 a1 7e df da 9c 98 09 da aa b8
------------------------------------------------------
error    test passed!
Testing const byte ptr reference...
Illegal instruction

The fix for this is to add -mforce-l32 to the wolfSSL ./configure command. Thanks @jcmvbkbc for the suggestion.

Here's an example of the memory test with the proper compiler flags and WOLFSSL_DEBUG turned on:

Running sysctl: OK
seedrng: can't create directory '/var/lib/seedrng': Read-only file system
Starting network: OK

Welcome to Buildroot
buildroot login: root
~ # cd /opt
/opt # ./testwolfcrypt
Math:   Multi-Precision: Wolf(SP) word-size=32 bits=3072 sp_int.c
------------------------------------------------------------------------------
 wolfSSL version 5.6.3
------------------------------------------------------------------------------
error    test passed!
Testing const byte ptr reference...
const byte ptr test success
MEMORY   test passed!

For reference, here's the entire wolfSSL ./configure command for Xtensa Linux Kernel on the ESP32-S3:

./configure \
  --host=xtensa-esp32s3-linux-uclibcfdpic \
  CC=xtensa-esp32s3-linux-uclibcfdpic-gcc \
  AR=xtensa-esp32s3-linux-uclibcfdpic-ar \
  STRIP=xtensa-esp32s3-linux-uclibcfdpic-strip \
  RANLIB=xtensa-esp32s3-linux-uclibcfdpic-ranlib \
  --prefix=/mnt/s3linux/build-xtensa-2023.02-fdpic-esp32s3/target/opt \
  CFLAGS="-mfdpic -mforce-l32  -DNO_WRITEV -DWOLFSSL_USER_SETTINGS -DDEBUG_WOLFSSL -DTFM_NO_ASM -DALT_ECC_SIZE" \
  --disable-filesystem --disable-all \
  --disable-shared --enable-psk --enable-sha --disable-fpecc --disable-ecc --disable-rsa --disable-sp-asm --disable-sha384 --disable-sha512 --disable-sha224 --disable-md5

  • edit: See also this informative presentation: ARM FDPIC Toolchain and ABI. Much (or all?) of the FDPIC information also applies to the Xtensa environment. The section about "Pointers to global data are indirectly referenced by the Global Offset Table (GOT)" is particularly relevant to the changes in the PR.

Fixes zd# n/a

Testing

How did you test?

Ran wolfCrypt test in:

  • ESP-IDF Embedded app
  • Xtensa ESP32-S3 Linux app
  • Regular compiled linux app.

Checklist

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

@gojimmypi gojimmypi self-assigned this Jun 19, 2023
@gojimmypi
Copy link
Contributor Author

The only PRB failure appears to be unrelated to the change:

*** Failed to download the package list from https://downloads.openwrt.org/releases/21.02-SNAPSHOT/targets/x86/64/ ...

@bandi13
Copy link
Contributor

bandi13 commented Jun 20, 2023

The only PRB failure appears to be unrelated to the change:

*** Failed to download the package list from https://downloads.openwrt.org/releases/21.02-SNAPSHOT/targets/x86/64/ ...

Looks like this error is happening on other PRs as well. Probably safe to ignore until we get a fix in.

Copy link
Contributor

@dgarske dgarske left a comment

Choose a reason for hiding this comment

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

Jim, are you looking to ensure that .const globals are actually loaded? This test seems odd to require and need on all platforms. The PR is like 20 lines of actual code and 60 lines of comments. Are you really wanting this PR to be merged into master still?

@gojimmypi
Copy link
Contributor Author

Hi David, thanks for taking a look at this.

are you looking to ensure that .const globals are actually loaded?

Not really. The .const globals, to my understanding, are not actually "loaded". As everything is on the same flash: read-only kernel, compiled app, and all... when the binary app runs, the .const values are not loaded into memory, rather they stay on the file system as an extreme method of resource conservation.

The trick is to tell the compiler this. It was not at all intuitive to me. I learned this from @jcmvbkbc (see tweet):

  • "The function peek(const byte *) loads a byte from the address passed to it and the function main calls it with the address of a constant. The constant goes into .rodata section, .rodata stays in the FLASH with the rest of .text when the program runs."

  • "You can't do 1-byte loads from FLASH mapped through the instruction bus, but you can do an aligned 4-byte load and extract the byte from it. That's what this compiler option does."

  • "And it does so by turning one l8ui into l32i + 5 other instructions which only takes ~5 cycles more, as opposed to taking an exception and fixing it up in the handler, which takes ~120."

This test seems odd to require and need on all platforms.

I agree, however it is not an intrusive test that would cause any harm. I also do not know how to detect at runtime the fact that a compile-time directive was missing.

It appears this was all caused by the cross-ng build (see tweet):

  • "This option is also used to build all software in the esp32 buildroot."

The PR is like 20 lines of actual code and 60 lines of comments.

Agreed this is a bit odd. I typically add a lot of detail to code that is not very intuitive. This is some of the most unintuitive code I've ever written. Given the unintuitive nature of what it is doing, future edits might cause the test to not actually detect the problem.

Are you really wanting this PR to be merged into master still?

I'm open to suggestion for improvement, but yes.

Any future developers of a read-only kernel app will greatly appreciate the error occurring in the memory test and not some arbitrary code that accesses const values that required the -mforce-l32 option.

@gojimmypi gojimmypi force-pushed the wc-memory-test branch 3 times, most recently from 3b445a9 to 86fec6f Compare June 26, 2023 00:07
@gojimmypi
Copy link
Contributor Author

After reconsidering the feedback, I've updated the changes to the new memory test. There are fewer comments and a couple of additional WOLFSSL_MSG debug messages that will be helpful to the embedded developer.

I've resynced with upstream master, retested with both -mforce-l32 to confirm success and a missing -mforce-l32 to confirm the error is properly manifested. All commits squashed & ready for review.

The test is not very intrusive. I propose letting it run on all platforms and configurations.

@gojimmypi gojimmypi requested a review from dgarske June 26, 2023 01:29
wolfcrypt/test/test.c Show resolved Hide resolved
wolfcrypt/test/test.c Show resolved Hide resolved
wolfcrypt/test/test.c Show resolved Hide resolved
wolfcrypt/test/test.c Show resolved Hide resolved
@gojimmypi
Copy link
Contributor Author

gojimmypi commented Jun 27, 2023

I'll be delayed on resolving this issue, as pulling from upstream master I now see these new cross-compile-time errors in api.c code changed recently by @SparkiDev in 578f56e6:

  CCLD     examples/asn1/asn1
  CCLD     testsuite/testsuite.test
  CCLD     tests/unit.test
tests/unit_test-api.o: in function `test_wolfSSL_client_server_nofail_memio':
api.c:(.text+0x3fc3c): dangerous relocation: l32r: literal target out of range (try using text-section-literals): (.literal+0x5464)
api.c:(.text+0x3fc59): dangerous relocation: l32r: literal target out of range (try using text-section-literals): (.literal+0x5458)
api.c:(.text+0x3fc7c): dangerous relocation: l32r: literal target out of range (try using text-section-literals): (.literal+0x543c)
[ ... snip,: many, many more  dangerous relocation: l32r ... ]

My current ./configure looks like this:

./configure \
  --disable-shared \
  --host=xtensa-esp32s3-linux-uclibcfdpic \
  CC=xtensa-esp32s3-linux-uclibcfdpic-gcc \
  AR=xtensa-esp32s3-linux-uclibcfdpic-ar \
  STRIP=xtensa-esp32s3-linux-uclibcfdpic-strip \
  RANLIB=xtensa-esp32s3-linux-uclibcfdpic-ranlib \
  --prefix=/mnt/s3linux/wolfssl-prefix \
  --libdir=/mnt/s3linux/wolfssl-prefix/lib \
  --includedir=/mnt/s3linux/wolfssl-prefix/include \
  --oldincludedir=/mnt/s3linux/wolfssl-prefix/include \
  CPPFLAGS="-I./" \
  CFLAGS=" -mfdpic  \
           -DTIME_T_NOT_64BIT -DNO_WRITEV -DHAVE_PK_CALLBACKS  \
           -DWOLFSSL_USER_IO -DWOLFSSL_USER_SETTINGS -DWOLFSSL_DEBUG  \
           -DTFM_NO_ASM -DALT_ECC_SIZE"

@gojimmypi gojimmypi force-pushed the wc-memory-test branch 3 times, most recently from d156243 to 6fa20da Compare June 28, 2023 17:09
@gojimmypi
Copy link
Contributor Author

Stackoverflow to the rescue regarding dangerous relocation: l32r:

image

Compiling with -mtext-section-literals should eliminate the error

Confirmed. Here's my latest wolfSSL ./configure for cross-compiling to Xtensa Linux on the ESP32:

./configure \
  --disable-shared \
  --host=xtensa-esp32s3-linux-uclibcfdpic \
  CC=xtensa-esp32s3-linux-uclibcfdpic-gcc \
  AR=xtensa-esp32s3-linux-uclibcfdpic-ar \
  STRIP=xtensa-esp32s3-linux-uclibcfdpic-strip \
  RANLIB=xtensa-esp32s3-linux-uclibcfdpic-ranlib \
  --prefix=/mnt/s3linux/wolfssl-prefix \
  --libdir=/mnt/s3linux/wolfssl-prefix/lib \
  --includedir=/mnt/s3linux/wolfssl-prefix/include \
  --oldincludedir=/mnt/s3linux/wolfssl-prefix/include \
  CPPFLAGS="-I./" \
  CFLAGS=" -mfdpic -mforce-l32 -mtext-section-literals \
           -DTIME_T_NOT_64BIT -DNO_WRITEV -DHAVE_PK_CALLBACKS  \
           -DWOLFSSL_USER_IO -DWOLFSSL_USER_SETTINGS -DWOLFSSL_DEBUG  \
           -DTFM_NO_ASM -DALT_ECC_SIZE"

@gojimmypi
Copy link
Contributor Author

The 2 PRB errors are likely unrelated to the changes here.

@gojimmypi gojimmypi requested a review from dgarske June 30, 2023 04:10
@dgarske
Copy link
Contributor

dgarske commented Jun 30, 2023

Looks good. I want to run this on some embedded targets also. We may need to wrap the test with a way to disable it.
Retest this please

@dgarske dgarske self-assigned this Jun 30, 2023
dgarske
dgarske previously approved these changes Jun 30, 2023
Copy link
Contributor

@dgarske dgarske left a comment

Choose a reason for hiding this comment

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

Tests out fine on STM32U5 CortexM. Thanks

@dgarske dgarske removed their assignment Jul 3, 2023
@gojimmypi
Copy link
Contributor Author

No changes, but there were recent upstream test.c changes in conflict. Merged, resolved & squashed.

@gojimmypi gojimmypi requested a review from dgarske July 3, 2023 18:54
@dgarske dgarske merged commit 6028dfd into wolfSSL:master Jul 3, 2023
64 checks passed
@gojimmypi gojimmypi deleted the wc-memory-test branch July 13, 2023 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants