Add basic support for s390x

[uweigand]

trafficstars

This adds basic support for Linux on s390x, using the fallback implementation of all algorithms and no assembler routines. This addresses https://github.com/briansmith/ring/issues/986 and makes "cargo test" fully pass on s390x.

There were two main changes I had to make to get this working:

• Provide a bn_mul_mont fallback implementation. Note that while there is a fallback implementation on the Rust side for limbs_mont_mul, some elliptic curve code directly calls bn_mul_mont on the C side, where there is currently no fallback. However, given that we have a bn_from_montgomery_in_place fallback, it is straightforward to add bn_mul_mont (along the lines of the limbs_mont_mul fallback).
• Support big-endian platforms. This consists of adding a number of endian access primitives in internal.h, and using them where required, currently in aes_nohw.c, poly1305.c, and the p256_scalar_bytes_from_limbs routine (for some reason, the p384 version of that routine is already endian-agnostic). I have chosen to use a plain C implementation of the primitives rather than bswap intrinsics, because that is easier to maintain, doesn't require platform #ifdef's, and still compiles to good code with any recent compiler.

@briansmith, please let me know if this approach makes sense to you or if this should be done differently.

Once this basic (unoptimized) support is in, we can then build on it by adding assembler optimizations (they're already present in OpenSSL, but would need to be copied over and adapted).

[uweigand]

Ping? Any comments on this approach?

[vimalk78]

hi @uweigand @briansmith, i am also interested in getting ring supported for s390x. Let me know if i can contribute in some way. From this PR, I was able to build ring using cross build --release --target s390x-unknown-linux-gnu btw, how do we get the above tests to run on the PR?

[vimalk78]

ping @briansmith @uweigand

[uweigand]

Hi @briansmith, let me try to make another attempt to get this going.

I've just updated the patch to use a different style of endian conversion, as you suggested. This keeps the CRYPTO_bswap4 implementation from boringssl and adds the corresponding CRYPTO_bswap8 implementation from there as well. The other endian-specific accessors are now all defined in term of CRYPTO_bswap4 / CRYPTO_bswap8 plus OPENSSL_memcpy (to handle any alignment issues if necessary).

Does this address your concerns?

If there's anything else I can do to help get this reviewed and merged, please let me know.

[uweigand]

Fixed merge conflict against current mainline. Any comments?

[Xynnn007]

Hi @briansmith , can this pr be merged now?

[uweigand]

Rebased and fixed merge conflicts against current mainline.

[uweigand]

Added support for cross-building and testing as requested in https://github.com/briansmith/ring/issues/1555. Split into three logically distinct commits.

[uweigand]

PR updated after https://github.com/briansmith/ring/pull/1558 was merged.

[uweigand]

Reworked big-endian support to ensure no asm changes on any little-endian target.

[wojciechozga]

Hi @briansmith , could this PR be merged or there is something more to do to get it accepted and merged into main?

I successfully compiled it on s390x. Tests passed.

[ecnelises]

This works well and can be the foundation of more big endian targets (like AIX). Any blockers?

[tnakaike]

I am expecting that this PR is merged because many components of confidential containers, which support s390x, are written in rust. For example, Key Broker Service uses rustls, which depends on ring, to enable https.

[kiszk]

@briansmith We would appreciate it if you could take a look. Any blockers for this PR?

[angelnu]

As Ring is used by PRISMA we would depend on this patch to be able to use Prima on s390x. As we (Metaco and Ripple) offer multi-arch builds for our SW this would help us a lot. We are looking at moving more components to Rust so this crypto library being available in s390x would be very appreciated.

[briansmith]

@uweigand Could you (or whoever is working on this at IBM/RedHat) email me at [email protected] so we could coordinate how we could get this finished without it being blocked on me, and how s390x (and PPC?) support can be best maintained going forward. I have some ideas and I want to hear your ideas.

[uweigand]

@uweigand Could you (or whoever is working on this at IBM/RedHat) email me at [email protected] so we could coordinate how we could get this finished without it being blocked on me, and how s390x (and PPC?) support can be best maintained going forward. I have some ideas and I want to hear your ideas.

I've now reached out to you via email.

[ecnelises]

Hi @uweigand could you please CC me ([email protected])? AIX stuff may be related, thanks.

[briansmith]

I am planning to do a release very soon, probably later this week. If you can update your PR then I will include it in the release. Thanks for your help here.

[briansmith]

I am planning to do a release very soon, probably later this week. If you can update your PR then I will include it in the release. Thanks for your help here.

Otherwise, if we do the 0.17 release without these changes, we can do a 0.17.1 with them this gets merged.

[uweigand]

I think I've addressed all comments above. Let me know if you would like me to change anything else.

[codecov[bot]]

Codecov Report

Merging #1297 (3062477) into main (bad63fb) will decrease coverage by 0.03%. The diff coverage is 52.94%.

@@            Coverage Diff             @@
##             main    #1297      +/-   ##
==========================================
- Coverage   95.98%   95.96%   -0.03%     
==========================================
  Files         134      134              
  Lines       19921    19945      +24     
  Branches      199      199              
==========================================
+ Hits        19122    19141      +19     
- Misses        762      765       +3     
- Partials       37       39       +2     

Files	Coverage Δ
crypto/fipsmodule/aes/aes_nohw.c	86.41% <100.00%> (-0.23%)	:arrow_down:
src/lib.rs	34.61% <0.00%> (ø)
crypto/internal.h	70.37% <46.15%> (+1.99%)	:arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[briansmith]

Note that you did all the work to enable s390x in CI but you didn't add s390x to the test, test-features, and coverage build matrices. We should do that so we have the right test coverage.

[uweigand]

Note that you did all the work to enable s390x in CI but you didn't add s390x to the test, test-features, and coverage build matrices. We should do that so we have the right test coverage.

Added to those three matrices.

[briansmith]

I will fix the --no-default-features build failure.

[briansmith]

Please rebase on top of main and resubmit.

[briansmith]

Why did the earlier version of this have changes to p256_shared.h but the newest one doesn't?

[uweigand]

Please rebase on top of main and resubmit.

Done.

Why did the earlier version of this have changes to p256_shared.h but the newest one doesn't?

Huh, I thought I added a comment but it seems to have disappeared. I've removed that change as re-testing showed it is no longer necessary in the current code base.

The change was to fix an endian issue in p256_scalar_bytes_from_limbs, but that function is now only called from the crypto/fipsmodule/ec/p256-nistz.c file, which in turn is only even built at all on the (little-endian) AARCH64 and X86_64 targets. So there is no need for this file to support big-endian platforms, and any code added there would be untested anyway.

[briansmith]

Good news: The refactoring to fix the --no-default-features build was merged into main.

Less good news: There's a merge conflict in crypto/internal.h from the latest BoringSSL merge. Also, PR #1663 moved the stuff in base.h to target.h. I will merge PR #1663 ASAP so you can rebase this on top of main for the release. I am hoping to finish the BoringSSL merge today so there shouldn't be any new conflicts.

[uweigand]

@briansmith: Rebased against current mainline to fix both merge conflicts.