ring icon indicating copy to clipboard operation
ring copied to clipboard

Published 20 hours ago • verified publisher icon briansmith

Support Xtensa, RISC-V and ESP-IDF

Open MabezDev opened this issue 3 years ago • 2 comments
trafficstars

This is a continuation of the upstreaming effort from https://github.com/briansmith/ring/pull/1459.

We would really like some feedback on this PR, we wish to use ring to secure millions of Espressif devices. If could let us know what reservations you have about this PR, I would be more than happy to address them.

Quoting from the original PR for comments about the PR itself:

A few points:

  • The code for bn_mult_mont is from https://github.com/briansmith/ring/pull/1297
  • I know you expressed reluctance to accepting variable-length arrays in the past, so let me know if you want that changed to something else.
  • Embedded targets are usually compiled with size-optimization so the compiler is very, very reluctant to inline things like the value barrier code in crypto/internal.h. Since we treat warnings as errors I needed to add a way to disable the -Winline flag.
  • I'm adding architecture support here, and also adding support for the ESP-IDF RTOS. Architecture support seems fairly uncontroversial if the code meets standards, but I understand if you're reluctant to add support for a new OS target for maintenance burden reasons. Just let me know and I can yank that part and maintain it myself as a patch. I'd obviously prefer it if we could upstream it though.
  • This has been lightly tested through rustls on both xtensa based ESP32 micros and RISC-V based ESP32's, but we haven't been able to run a full test suite on them on account of not having a full general purpose OS, so no cargo test. Let me know if this is a blocker and I'll figure out how to at least make the tests work on RISC-V. I think linux on RISC-V would work in qemu or something.

MabezDev avatar Jul 27 '22 11:07 MabezDev

This patch would also be extremely enabling for Xous on Precursor! We're trying to add secure messaging to our RISC-V mobile platform and getting rustls to compile past the ring dependency is a major stumbling block. Something like this would allow us to move ahead on application development for our end users.

Thanks for opening the PR @MabezDev, I really appreciate it!

bunnie avatar Jul 27 '22 13:07 bunnie

We are also currently using a patched version of ring for Espressif based platforms - would really love to see this getting upstreamed!

My thoughts regarding the dynamically sized arrays: I don't really know where the size comes from or how large it can get - but I think if the maximum is reasonably small it would make sense to keep this as a static stack array in combination with a check. The comment above the function in the header seems to suggest that the function has no way of signalling an error, so I'm not sure how a size check would look in this context.

Janrupf avatar Aug 08 '22 23:08 Janrupf

@briansmith would you mind taking a look at this PR when you get a few moments? It would be really great to get some feedback :).

MabezDev avatar Oct 18 '22 13:10 MabezDev

Just have seen https://twitter.com/feistyduck/status/1577644279740387328: This PR is linked in this article: https://www.crowdsupply.com/sutajio-kosagi/precursor/updates/bringing-up-tls-on-precursor They have made a temporary pure-Rust fork of ring with c2rust: https://github.com/betrusted-io/ring-xous/tree/0.16.20-cleanup/src/c2rust

Darkspirit avatar Oct 26 '22 03:10 Darkspirit

I can confirm that this patch fixed building ring for RISC-V for me. I also had to apply this patch:

--- a/build.rs
+++ b/build.rs
@@ -236,6 +236,13 @@ const ASM_TARGETS: &[AsmTarget] = &[
         asm_extension: "S",
         preassemble: true,
     },
+    AsmTarget {
+        oss: LINUX_ABI,
+        arch: "riscv64",
+        perlasm_format: "linux64",
+        asm_extension: "S",
+        preassemble: false,
+    },
 ];
 
 struct AsmTarget {

I was also building for a non-standard RISC-V arch and had to do:

export CRATE_CC_NO_DEFAULTS=1

before calling cargo build.

juztamau5 avatar Nov 09 '22 05:11 juztamau5

@MabezDev Thanks again for the PR. Because my comments are tantamount to "redo the whole thing and split it up into multiple PRs," and because you aren't the original author of the bn_mult_mont code, I don't think this PR makes sense any more. I'm going to close it so we can concentrate on resolving #1455 and then rebasing the remaining work on top of that solution.

briansmith avatar Nov 09 '22 21:11 briansmith

@MabezDev Been wanting to try use rustls in my ESP32S3 project. First had to port these changes to version 0.16.20 (https://github.com/reinismu/ring/commit/d82e61ddb27c0485c5a405e0d97144864f1f417b). It compiled fine, but at linker stage it fails with:

xtensa-esp32s3-elf/bin/ld: /home/reinis/projects/hub/target/xtensa-esp32s3-espidf/release/deps/libring-01be472a3cba49f7.rlib: error adding symbols: file format not recognized

Investigating

objdump 
lib.rmeta:     file format elf32-little
architecture: UNKNOWN!, flags 0x00000010:
HAS_SYMS
start address 0x00000000


ring-01be472a3cba49f7.ring.54f8b82c-cgu.0.rcgu.o:     file format elf32-little
architecture: UNKNOWN!, flags 0x00000011:
HAS_RELOC, HAS_SYMS
start address 0x00000000


ring-01be472a3cba49f7.ring.54f8b82c-cgu.1.rcgu.o:     file format elf32-little
architecture: UNKNOWN!, flags 0x00000011:
HAS_RELOC, HAS_SYMS
start address 0x00000000


ring-01be472a3cba49f7.ring.54f8b82c-cgu.10.rcgu.o:     file format elf32-little
architecture: UNKNOWN!, flags 0x00000011:
HAS_RELOC, HAS_SYMS
start address 0x00000000


ring-01be472a3cba49f7.ring.54f8b82c-cgu.11.rcgu.o:     file format elf32-little
architecture: UNKNOWN!, flags 0x00000011:
HAS_RELOC, HAS_SYMS
start address 0x00000000


ring-01be472a3cba49f7.ring.54f8b82c-cgu.12.rcgu.o:     file format elf32-little
architecture: UNKNOWN!, flags 0x00000011:
HAS_RELOC, HAS_SYMS
start address 0x00000000


ring-01be472a3cba49f7.ring.54f8b82c-cgu.13.rcgu.o:     file format elf32-little
architecture: UNKNOWN!, flags 0x00000011:
HAS_RELOC, HAS_SYMS
start address 0x00000000


ring-01be472a3cba49f7.ring.54f8b82c-cgu.14.rcgu.o:     file format elf32-little
architecture: UNKNOWN!, flags 0x00000011:
HAS_RELOC, HAS_SYMS
start address 0x00000000


ring-01be472a3cba49f7.ring.54f8b82c-cgu.15.rcgu.o:     file format elf32-little
architecture: UNKNOWN!, flags 0x00000011:
HAS_RELOC, HAS_SYMS
start address 0x00000000


ring-01be472a3cba49f7.ring.54f8b82c-cgu.2.rcgu.o:     file format elf32-little
architecture: UNKNOWN!, flags 0x00000011:
HAS_RELOC, HAS_SYMS
start address 0x00000000


ring-01be472a3cba49f7.ring.54f8b82c-cgu.3.rcgu.o:     file format elf32-little
architecture: UNKNOWN!, flags 0x00000011:
HAS_RELOC, HAS_SYMS
start address 0x00000000


ring-01be472a3cba49f7.ring.54f8b82c-cgu.4.rcgu.o:     file format elf32-little
architecture: UNKNOWN!, flags 0x00000011:
HAS_RELOC, HAS_SYMS
start address 0x00000000


ring-01be472a3cba49f7.ring.54f8b82c-cgu.5.rcgu.o:     file format elf32-little
architecture: UNKNOWN!, flags 0x00000011:
HAS_RELOC, HAS_SYMS
start address 0x00000000


ring-01be472a3cba49f7.ring.54f8b82c-cgu.6.rcgu.o:     file format elf32-little
architecture: UNKNOWN!, flags 0x00000011:
HAS_RELOC, HAS_SYMS
start address 0x00000000


ring-01be472a3cba49f7.ring.54f8b82c-cgu.7.rcgu.o:     file format elf32-little
architecture: UNKNOWN!, flags 0x00000011:
HAS_RELOC, HAS_SYMS
start address 0x00000000


ring-01be472a3cba49f7.ring.54f8b82c-cgu.8.rcgu.o:     file format elf32-little
architecture: UNKNOWN!, flags 0x00000011:
HAS_RELOC, HAS_SYMS
start address 0x00000000


ring-01be472a3cba49f7.ring.54f8b82c-cgu.9.rcgu.o:     file format elf32-little
architecture: UNKNOWN!, flags 0x00000011:
HAS_RELOC, HAS_SYMS
start address 0x00000000


aes_nohw.o:     file format elf64-x86-64
architecture: i386:x86-64, flags 0x00000011:
HAS_RELOC, HAS_SYMS
start address 0x0000000000000000


montgomery.o:     file format elf64-x86-64
architecture: i386:x86-64, flags 0x00000011:
HAS_RELOC, HAS_SYMS
start address 0x0000000000000000


montgomery_inv.o:     file format elf64-x86-64
architecture: i386:x86-64, flags 0x00000011:
HAS_RELOC, HAS_SYMS
start address 0x0000000000000000


limbs.o:     file format elf64-x86-64
architecture: i386:x86-64, flags 0x00000011:
HAS_RELOC, HAS_SYMS
start address 0x0000000000000000


mem.o:     file format elf64-x86-64
architecture: i386:x86-64, flags 0x00000011:
HAS_RELOC, HAS_SYMS
start address 0x0000000000000000


poly1305.o:     file format elf64-x86-64
architecture: i386:x86-64, flags 0x00000011:
HAS_RELOC, HAS_SYMS
start address 0x0000000000000000


crypto.o:     file format elf64-x86-64
architecture: i386:x86-64, flags 0x00000011:
HAS_RELOC, HAS_SYMS
start address 0x0000000000000000


curve25519.o:     file format elf64-x86-64
architecture: i386:x86-64, flags 0x00000011:
HAS_RELOC, HAS_SYMS
start address 0x0000000000000000


ecp_nistz.o:     file format elf64-x86-64
architecture: i386:x86-64, flags 0x00000011:
HAS_RELOC, HAS_SYMS
start address 0x0000000000000000


ecp_nistz256.o:     file format elf64-x86-64
architecture: i386:x86-64, flags 0x00000011:
HAS_RELOC, HAS_SYMS
start address 0x0000000000000000


gfp_p256.o:     file format elf64-x86-64
architecture: i386:x86-64, flags 0x00000011:
HAS_RELOC, HAS_SYMS
start address 0x0000000000000000


gfp_p384.o:     file format elf64-x86-64
architecture: i386:x86-64, flags 0x00000011:
HAS_RELOC, HAS_SYMS
start address 0x0000000000000000


constant_time_test.o:     file format elf64-x86-64
architecture: i386:x86-64, flags 0x00000011:
HAS_RELOC, HAS_SYMS
start address 0x0000000000000000

Here I notice architecture: i386:x86-64 could that be the cause? If that is the case how do I fix it? :thinking:

Seems like ring calls system cc to compile source code. Tried to replace it with cc1 inside esp32s3, but it doesn't like the args

running "/home/reinis/projects/hub/.embuild/platformio/packages/toolchain-xtensa-esp32s3/libexec/gcc/xtensa-esp32s3-elf/8.4.0/cc1" "-Os" "-ffunction-sections" "-fdata-sections" "-fPIC" "-I" "include" "-Wall" "-Wextra" "-std=c1x" "-Wbad-function-cast" "-Wnested-externs" "-Wstrict-prototypes" "-pedantic" "-pedantic-errors" "-Wall" "-Wextra" "-Wcast-align" "-Wcast-qual" "-Wconversion" "-Wenum-compare" "-Wfloat-equal" "-Wformat=2" "-Winvalid-pch" "-Wmissing-field-initializers" "-Wmissing-include-dirs" "-Wredundant-decls" "-Wshadow" "-Wsign-compare" "-Wsign-conversion" "-Wundef" "-Wuninitialized" "-Wwrite-strings" "-fno-strict-aliasing" "-fvisibility=hidden" "-fstack-protector" "-g3" "-Werror" "-DNDEBUG" "-c" "-o/home/reinis/projects/hub/target/xtensa-esp32s3-espidf/release/build/ring-5a9be61bdfb3c181/out/aes_nohw.o" "crypto/fipsmodule/aes/aes_nohw.c"

  --- stderr
  cc1: error: command line option '-c' is valid for the driver but not for C

Removed -c argument and now I get

--- stderr
  In file included from include/GFp/aes.h:52,
                   from crypto/fipsmodule/aes/aes_nohw.c:15:
  include/GFp/base.h:65:10: fatal error: stddef.h: No such file or directory
   #include <stddef.h>
            ^~~~~~~~~~

Put back -c argument and switched to /home/reinis/projects/hub/.embuild/platformio/packages/toolchain-xtensa-esp32s3/bin/xtensa-esp32s3-elf-gcc. Got a bit further

[ring 0.16.20] running "/home/reinis/projects/hub/.embuild/platformio/packages/toolchain-xtensa-esp32s3/bin/xtensa-esp32s3-elf-gcc" "-Os" "-ffunction-sections" "-fdata-sections" "-fPIC" "-I" "include" "-Wall" "-Wextra" "-std=c1x" "-Wbad-function-cast" "-Wnested-externs" "-Wstrict-prototypes" "-pedantic" "-pedantic-errors" "-Wall" "-Wextra" "-Wcast-align" "-Wcast-qual" "-Wconversion" "-Wenum-compare" "-Wfloat-equal" "-Wformat=2" "-Winvalid-pch" "-Wmissing-field-initializers" "-Wmissing-include-dirs" "-Wredundant-decls" "-Wshadow" "-Wsign-compare" "-Wsign-conversion" "-Wundef" "-Wuninitialized" "-Wwrite-strings" "-fno-strict-aliasing" "-fvisibility=hidden" "-fstack-protector" "-g3" "-Werror" "-DNDEBUG" "-c" "-o/home/reinis/projects/hub/target/xtensa-esp32s3-espidf/release/build/ring-5a9be61bdfb3c181/out/montgomery.o" "crypto/fipsmodule/bn/montgomery.c"
[ring 0.16.20] crypto/fipsmodule/bn/montgomery.c: In function 'bn_mul_mont':
[ring 0.16.20] crypto/fipsmodule/bn/montgomery.c:168:20: error: implicit declaration of function 'limbs_mul_add_limb'; did you mean 'GFp_limbs_mul_add_limb'? [-Wimplicit-function-declaration]
[ring 0.16.20]      tmp[num + i] = limbs_mul_add_limb(tmp + i, ap, bp[i], num);
[ring 0.16.20]                     ^~~~~~~~~~~~~~~~~~
[ring 0.16.20]                     GFp_limbs_mul_add_limb
[ring 0.16.20] crypto/fipsmodule/bn/montgomery.c:168:20: error: nested extern declaration of 'limbs_mul_add_limb' [-Werror=nested-externs]
[ring 0.16.20] crypto/fipsmodule/bn/montgomery.c:168:20: error: conversion to 'Limb' {aka 'unsigned int'} from 'int' may change the sign of the result [-Werror=sign-conversion]
[ring 0.16.20] crypto/fipsmodule/bn/montgomery.c:170:3: error: implicit declaration of function 'bn_from_montgomery_in_place'; did you mean 'GFp_bn_from_montgomery_in_place'? [-Wimplicit-function-declaration]
[ring 0.16.20]    bn_from_montgomery_in_place(rp, num, tmp, 2 * num, np, num, n0);
[ring 0.16.20]    ^~~~~~~~~~~~~~~~~~~~~~~~~~~
[ring 0.16.20]    GFp_bn_from_montgomery_in_place

Had wrong function name. I guess it was renamed in 0.17. Changed limbs_mul_add_limb to GFp_limbs_mul_add_limb and bn_from_montgomery_in_place to GFp_bn_from_montgomery_in_place.

It compiles now, but at link time I get a bit more linking errors for undefined references. If I don't try to use my TcpAcceptor with SSL it compiles fine. I guess it requires nistz256 methods and as we don't have them created for xtensa it fails at link time.

linker errors 
          /home/reinis/projects/hub/.embuild/espressif/tools/xtensa-esp32s3-elf/esp-2021r2-patch5-8.4.0/xtensa-esp32s3-elf/bin/../lib/gcc/xtensa-esp32s3-elf/8.4.0/../../../../xtensa-esp32s3-elf/bin/ld: /home/reinis/projects/hub/target/xtensa-esp32s3-espidf/release/deps/libring-01be472a3cba49f7.rlib(ring-01be472a3cba49f7.ring.54f8b82c-cgu.14.rcgu.o):(.literal._ZN4ring2ec7suite_b3ops4p25621p256_elem_inv_squared17hc29dd16a24b1de3bE.llvm.13593639650277209433+0x0): undefined reference to `GFp_nistz256_sqr_mont'
          /home/reinis/projects/hub/.embuild/espressif/tools/xtensa-esp32s3-elf/esp-2021r2-patch5-8.4.0/xtensa-esp32s3-elf/bin/../lib/gcc/xtensa-esp32s3-elf/8.4.0/../../../../xtensa-esp32s3-elf/bin/ld: /home/reinis/projects/hub/target/xtensa-esp32s3-espidf/release/deps/libring-01be472a3cba49f7.rlib(ring-01be472a3cba49f7.ring.54f8b82c-cgu.14.rcgu.o):(.literal._ZN4ring2ec7suite_b3ops4p25621p256_elem_inv_squared17hc29dd16a24b1de3bE.llvm.13593639650277209433+0x4): undefined reference to `GFp_nistz256_mul_mont'
          /home/reinis/projects/hub/.embuild/espressif/tools/xtensa-esp32s3-elf/esp-2021r2-patch5-8.4.0/xtensa-esp32s3-elf/bin/../lib/gcc/xtensa-esp32s3-elf/8.4.0/../../../../xtensa-esp32s3-elf/bin/ld: /home/reinis/projects/hub/target/xtensa-esp32s3-espidf/release/deps/libring-01be472a3cba49f7.rlib(ring-01be472a3cba49f7.ring.54f8b82c-cgu.14.rcgu.o):(.rodata._ZN4ring2ec7suite_b3ops4p25610COMMON_OPS17h28a8eeed38090905E.llvm.13593639650277209433+0xf4): undefined reference to `GFp_nistz256_add'
          /home/reinis/projects/hub/.embuild/espressif/tools/xtensa-esp32s3-elf/esp-2021r2-patch5-8.4.0/xtensa-esp32s3-elf/bin/../lib/gcc/xtensa-esp32s3-elf/8.4.0/../../../../xtensa-esp32s3-elf/bin/ld: /home/reinis/projects/hub/target/xtensa-esp32s3-espidf/release/deps/libring-01be472a3cba49f7.rlib(ring-01be472a3cba49f7.ring.54f8b82c-cgu.14.rcgu.o):(.rodata._ZN4ring2ec7suite_b3ops4p25610COMMON_OPS17h28a8eeed38090905E.llvm.13593639650277209433+0xf8): undefined reference to `GFp_nistz256_mul_mont'
          /home/reinis/projects/hub/.embuild/espressif/tools/xtensa-esp32s3-elf/esp-2021r2-patch5-8.4.0/xtensa-esp32s3-elf/bin/../lib/gcc/xtensa-esp32s3-elf/8.4.0/../../../../xtensa-esp32s3-elf/bin/ld: /home/reinis/projects/hub/target/xtensa-esp32s3-espidf/release/deps/libring-01be472a3cba49f7.rlib(ring-01be472a3cba49f7.ring.54f8b82c-cgu.14.rcgu.o):(.rodata._ZN4ring2ec7suite_b3ops4p25610COMMON_OPS17h28a8eeed38090905E.llvm.13593639650277209433+0xfc): undefined reference to `GFp_nistz256_sqr_mont'
          /home/reinis/projects/hub/.embuild/espressif/tools/xtensa-esp32s3-elf/esp-2021r2-patch5-8.4.0/xtensa-esp32s3-elf/bin/../lib/gcc/xtensa-esp32s3-elf/8.4.0/../../../../xtensa-esp32s3-elf/bin/ld: /home/reinis/projects/hub/target/xtensa-esp32s3-espidf/release/deps/libring-01be472a3cba49f7.rlib(ring-01be472a3cba49f7.ring.54f8b82c-cgu.5.rcgu.o):(.literal._ZN4ring4aead17chacha20_poly130522chacha20_poly1305_seal17hc9cab6da13a9e4c3E+0x8): undefined reference to `GFp_ChaCha20_ctr32'
          /home/reinis/projects/hub/.embuild/espressif/tools/xtensa-esp32s3-elf/esp-2021r2-patch5-8.4.0/xtensa-esp32s3-elf/bin/../lib/gcc/xtensa-esp32s3-elf/8.4.0/../../../../xtensa-esp32s3-elf/bin/ld: /home/reinis/projects/hub/target/xtensa-esp32s3-espidf/release/deps/libring-01be472a3cba49f7.rlib(ecp_nistz256.o):(.literal.GFp_nistz256_point_add+0x4): undefined reference to `GFp_nistz256_sqr_mont'
          /home/reinis/projects/hub/.embuild/espressif/tools/xtensa-esp32s3-elf/esp-2021r2-patch5-8.4.0/xtensa-esp32s3-elf/bin/../lib/gcc/xtensa-esp32s3-elf/8.4.0/../../../../xtensa-esp32s3-elf/bin/ld: /home/reinis/projects/hub/target/xtensa-esp32s3-espidf/release/deps/libring-01be472a3cba49f7.rlib(ecp_nistz256.o):(.literal.GFp_nistz256_point_add+0x8): undefined reference to `GFp_nistz256_mul_mont'
          /home/reinis/projects/hub/.embuild/espressif/tools/xtensa-esp32s3-elf/esp-2021r2-patch5-8.4.0/xtensa-esp32s3-elf/bin/../lib/gcc/xtensa-esp32s3-elf/8.4.0/../../../../xtensa-esp32s3-elf/bin/ld: /home/reinis/projects/hub/target/xtensa-esp32s3-espidf/release/deps/libring-01be472a3cba49f7.rlib(ecp_nistz256.o):(.literal.GFp_nistz256_point_add+0x10): undefined reference to `GFp_nistz256_point_double'
          /home/reinis/projects/hub/.embuild/espressif/tools/xtensa-esp32s3-elf/esp-2021r2-patch5-8.4.0/xtensa-esp32s3-elf/bin/../lib/gcc/xtensa-esp32s3-elf/8.4.0/../../../../xtensa-esp32s3-elf/bin/ld: /home/reinis/projects/hub/target/xtensa-esp32s3-espidf/release/deps/libring-01be472a3cba49f7.rlib(ecp_nistz256.o):(.literal.GFp_nistz256_point_mul+0x14): undefined reference to `GFp_nistz256_neg'

From what I see we don't have it implemented in 0.17 version as well. Seems like ring won't function properly until those methods for xtensa architecture are created

Seems like EC algs are impacted. While I manage to stick with RSA and not link and code related to EC it should work

reinismu avatar Feb 03 '23 12:02 reinismu