C (or Rust) fallback for all functions that work for all little-endian targets

[ellenhp]

Related: #1419

Just want to start a conversation around portability to embedded architectures.

Despite having a lot of assembly, ring actually pretty close to being portable. All that's preventing it from building on architectures like RISC-V and xtensa is a Rust or pure-C implementation of what appears to be a montgomery multiplier. There's an open PR (#1436) to add one (written in C) but I can't figure out where the code actually came from, and there are no tests so I don't really trust it.

I'm pretty motivated to get crypto stuff working for an ESP32 project of mine, so let me know if I can help make this happen. I'd be up for trying to write a C or Rust fallback for this function, or writing tests for the existing one, trying to analyze it with KLEE to prove its memory safety and constant-time properties, etc, whatever's needed to meet code standards.

It would be amazing to have the entire rust ecosystem of crypto libraries working on the ESP32 especially if I don't have to maintain a patch like I am right now. :)

[ellenhp]

Also related to #1297 it seems!

[briansmith]

I am generalizing this issue to be about automatically supporting every little-endian architecture with a generic fallback. Big-endian architectures are tracked separately in #1555; the work in supporting them is a superset of the work in supporting the little-endian architectures.

Here's what we need to do, AFAICT:

• Have a C or Rust fallback for each of the few functions that are currently only implemented in Rust. In the case of bn_mul_mont we may just want to replace the callers with callers that call the Rust equivalent, or implement bn_mul_mont in terms of that Rust code.
• Replace the allowlist of target architectures in include/ring-core/base.h with a more general mechanism; see below.
• Run tests in CI (probably using qemu like we do for 32-bit ARM and Aarch64) for an architecture that doesn't have any assembly language implementations in use. I suggest wasm32-wasi.

Regarding the allowlist in include/ring-core/base.h: It looks like this:

#if defined(__x86_64) || defined(_M_AMD64) || defined(_M_X64)
#define OPENSSL_64_BIT
#define OPENSSL_X86_64
#elif defined(__x86) || defined(__i386) || defined(__i386__) || defined(_M_IX86)
#define OPENSSL_32_BIT
#define OPENSSL_X86
#elif defined(__AARCH64EL__) || defined(_M_ARM64)
#define OPENSSL_64_BIT
#define OPENSSL_AARCH64
#elif defined(__ARMEL__) || defined(_M_ARM)
#define OPENSSL_32_BIT
#define OPENSSL_ARM
#elif defined(__MIPSEL__) && !defined(__LP64__)
#define OPENSSL_32_BIT
#define OPENSSL_MIPS
#elif defined(__MIPSEL__) && defined(__LP64__)
#define OPENSSL_64_BIT
#define OPENSSL_MIPS64
#elif defined(__wasm__)
#define OPENSSL_32_BIT
#else
// Note BoringSSL only supports standard 32-bit and 64-bit two's-complement,
// little-endian architectures. Functions will not produce the correct answer
// on other systems. Run the crypto_test binary, notably
// crypto/compiler_test.cc, before adding a new architecture.
#error "Unknown target CPU"
#endif

The base.h file is inherited from BoringSSL. The allowlist approach may work well for BoringSSL but it won't work well for ring because the allowlist means that we necessarily must receive at least one PR per target architecture before that architecutre starts working.

We should remove things like the logic that defines OPENSSL_MIPS64 and any other similar architecture-specific macro that isn't actually used in the codebase, to make it clear which architectures actually have specialized logic. This will reduce base.h substantially.

Ahead of the #else that currently does #error, we can do something like this:

- #else
- #endif
+ #elif defined(RING_ASSUME_OPENSSL_32_BIT)
+ #define OPENSSL_32_BIT
+ #elif defined(RING_ASSUME_OPENSSL_64_BIT)
+ #define OPENSSL_64_BIT
  #else
  // Note BoringSSL only supports standard 32-bit and 64-bit two's-complement,
  // little-endian architectures. Functions will not produce the correct answer
  // on other systems. Run the crypto_test binary, notably
  // crypto/compiler_test.cc, before adding a new architecture.
  #error "Unknown target CPU"
  #endif

+ #if (defined(RING_ASSUME_OPENSSL_32_BIT) && defined(OPENSSL_64_BIT)) || (defined(RING_ASSUME_OPENSSL_64_BIT) && !defined(OPENSSL_64_BIT))
+ #error "RING_ASSUME_OPENSSL_{32,64}_BIT does not match OPENSSL_{32,64}_BIT`
+ #endif

Then build.rs can set RING_ASSUME_OPENSSL_32_BIT or RING_ASSUME_OPENSSL_64_BIT accordingly.

One complication is that currently we have:

#elif defined(__wasm__)
#define OPENSSL_32_BIT

My understanding is that WebAssembly should be able to properly support 64-bit integers and, if that is correct, we should ensure OPENSSL_64_BIT is defined for it since the 64-bit implementations are much faster. In particular, I think we need to find something better than target_pointer_width and sizeof(size_t) to determine the right bitness of the target.

Also in build.rs we currently have:

    if (target.arch == "wasm32" && target.os == "unknown")
        || (target.os == "linux" && target.is_musl && target.arch != "x86_64")
    {
        if let Ok(compiler) = c.try_get_compiler() {
            // TODO: Expand this to non-clang compilers in 0.17.0 if practical.
            if compiler.is_like_clang() {
                let _ = c.flag("-nostdlibinc");
                let _ = c.define("RING_CORE_NOSTDLIBINC", "1");
            }
        }
    }

I suggest we try to generalize that to every target_arch and every OS (besides macOS and Windows?), so that cross-compiling with clang won't require a sysroot.

[briansmith]

Run tests in CI (probably using qemu like we do for 32-bit ARM and Aarch64) for an architecture that doesn't have any assembly language implementations in use. I suggest wasm32-wasi.

Unfortunately wasm32-wasi doesn't look like a great choice because it doesn't support profiler builtins, which means it doesn't support code coverage measurement. Anybody have a suggestion for a single 32-bit target and a single 32-bit target that each meets these conditions?:

• The target is supported by qemu-user, so we can change mk/cargo.sh test --target={X} to run those tests on x86_64 (at least) hosts, and so we can run the tests in the tests jobs in GitHub Actions
• The Rust stdlib for it is built with profiler_builtins enabled so we can add the target to the coverage jobs in GitHub Actions.
• The target is supported by clang and llvm-tools so mk/check-symbol-prefixes.sh can be modified to work for the target.
• The target_arch isn't arm, aarch64, x86, or x86_64.

For my part in this, I am expanding the test suite to include more tests for the arithmetic for which we'll need to add fallbacks for (bn_mul_mont at least). That way, once the scripts in mk/ have been updated for whatever new presently-unsupported targets we start with, we'll be able to drop in the needed fallbacks and everything should "just work" and future contributors will have some assurance that their future PRs don't break this new support.

[ellenhp]

I'm curious if mips-unknown-linux-gnu would fit those criteria. It satisfies the first and last ones, at least. I won't have time to check on the middle two this week unless I can do it before work one day, but that's the big one that comes to mind.

[briansmith]

@LinusU had made a PR a while back that adds mipsel-unknown-linux-gnu support. I used that PR to draft PR #1556, which also tries to add riscv64gc-unknown-linux-gnu support. Unfortunately, neither target supports the profiler runtime.

[briansmith]

Here are some rust-lang/rust issues related to missing profiler built-in support:

• Code coverage and profiling don't work on non-X86_64 -musl targets because profiler_builtins isn't provided for them rust-lang/rust#79556 (Filed by me 2 years ago)
• Code coverage does not work on armv7-unknown-linux-gnueabihf due to missing profiler_builtins rust-lang/rust#79640 (Filed shortly thereafter)
  If there's somebody who can make progress in the rust-lang/rust repo to get the profiler built-ins supported on some targets that are acceptable for this, that would be great. Nightly-only would be fine, since it's just for code coverage measurement. This is really important because it makes reviewing code much easier.

[briansmith]

Besides the code coverage measurement issue, here are some additional things to consider:

• Adding these fallbacks may implicitly enable some additional functionality in wasm32 targets that isn't tested using wasm-bindgen, as wasm-bindgen requires boilerplate like this for each test module:

#[cfg(target_arch = "wasm32")]
use wasm_bindgen_test::{wasm_bindgen_test as test, wasm_bindgen_test_configure};

#[cfg(target_arch = "wasm32")]
wasm_bindgen_test_configure!(run_in_browser);

We should go add that boilerplate to all the tests as part of doing this work, or at least all the integration tests.

We should add a static assertion that the target is either 32-bit or 64-bit.

We should add a static assertion that the target is little-endian. Again, changes to support big endian targets are expected to be done on top of this work, and those changes are tracked in issue #1555.

[briansmith]

PR #1558 implements the fallback for bn_mul_mont and related CI changes that allows (at least) mipsel-unknown-linux-gnu to work, though code test coverage measurement doesn't work for that target yet.

[briansmith]

It seems like PR #1558 was the only thing needed here that isn't big-endian specific. Given rust-lang/rust#104304, it looks like the fallback logic will most likely end up being tested in QEMU using s390x once the big-endian support is added (see #1555), at least initially.

[Shnatsel]

rust-lang/rust#105389 has added profiler support on powerpc64le-unknown-linux-gnu. This change is already available on current nightly, and will reach stable in Rust 1.67.

So platform-independent code can also be tested on little-endian systems now.

[ofek]

When building for powerpc64le-unknown-linux-gnu I get the following compilation error:

error: failed to run custom build command for `ring v0.16.20`

Caused by:
  process didn't exit successfully: `/target/release/build/ring-2ec39d085cdcc776/build-script-build` (exit status: 101)
  --- stderr
  thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', /cargo/registry/src/index.crates.io-6f17d22bba15001f/ring-0.16.20/build.rs:358:10
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

It looks like there hasn't been a release in 2 years, have fixes for this been merged?

[darkbasic]

You can use https://github.com/IBM/ring/tree/ppc-0.16.20
Example: https://github.com/gentoo/guru/pull/90/files

[briansmith]

This is done. Verified while reviewing PR #1057. Also with PR #1297 the fallbacks will work for big-endian too.

[ofek]

Thank you very much Brian, and happy new year!