Attempt to migrate away from bn_mul_mont into Rust #1278
Conversation
As part of the effort to support MIPS, we would need to avoid using bn_mul_mont on C code to avoid needing to add MIPS assembly. This commit attempts a literal port of the C code into Rust using limbs_mont_mul.
|
The code currently checks in Rust, but it fails to link as |
|
Hi, thanks for doing this. You've done almost everything exactly right. Either this weekend or next weekend, I'll experiment with tweaking this to work the way I think it should work, and I'll submit a PR for you to review the changes. |
|
Thank you. It's been an learning experience so far - I appreciate the feedback on these changes |
|
For the P-384 stuff, we need to replace the current C code with code that uses the Fiat Crypto P-384 code that BoringSSL also uses. That should solve the remaining blocker. I will do that in a separate PR when I have some time. |
|
This is now one of the things that is missing for supporting EC algorithms in Webassembly, too. I tried to find out if I could help with this, but it seems that BoringSSL doesn't use the Fiat Crypto P-384 code: https://boringssl.googlesource.com/boringssl/+/3359/third_party/fiat @briansmith do you maybe have suggestions how we could move this PR forward? |
|
I'm also interested in this PR. Use case would be to do EC in WebAssembly. |
There was a problem hiding this comment.
Thanks for this PR. I'm very sorry about the long delay in reviewing it. I do absolutely love the idea of replacing gfp_p256.c with Rust code. In order for this approach to work we'd need to do the analogous thing for gfp_p384.c, which is much more tricky. I spent some time today looking into doing that but it looks like it will be a more involved process than I presently have time for this month.
I used this PR as inspiration for #1558, which uses basically the same code in bigint but through the bn_mul_mont interface, unfortunately.
Thanks again.
| @@ -66,7 +66,6 @@ include = [ | |||
| "crypto/fipsmodule/ec/ecp_nistz.h", | |||
| "crypto/fipsmodule/ec/ecp_nistz384.h", | |||
| "crypto/fipsmodule/ec/ecp_nistz384.inl", | |||
| "crypto/fipsmodule/ec/gfp_p256.c", | |||
| @@ -1166,7 +1166,7 @@ fn greater_than(a: &Nonnegative, b: &Nonnegative) -> bool { | |||
|
|
|||
| #[derive(Clone)] | |||
| #[repr(transparent)] | |||
| struct N0([Limb; 2]); | |||
| pub(crate) struct N0(pub(crate) [Limb; 2]); | |||
There was a problem hiding this comment.
Instead of making the inner field pub(crate), we should replace the From<u64> for N0 with a const fn from(n0: u64) function so that we can use it instead.
| @@ -66,7 +66,7 @@ pub struct CommonOps { | |||
| pub b: Elem<R>, | |||
|
|
|||
| // In all cases, `r`, `a`, and `b` may all alias each other. | |||
| elem_mul_mont: unsafe extern "C" fn(r: *mut Limb, a: *const Limb, b: *const Limb), | |||
| elem_mul_mont: fn(r: &mut [Limb], a: &[Limb], b: &[Limb]), | |||
There was a problem hiding this comment.
We'll have to leave this as it was so that the P256 implementation can directly call the assembly/C code without any additional overhead.
| @@ -240,7 +240,7 @@ pub struct ScalarOps { | |||
| pub common: &'static CommonOps, | |||
|
|
|||
| scalar_inv_to_mont_impl: fn(a: &Scalar) -> Scalar<R>, | |||
| scalar_mul_mont: unsafe extern "C" fn(r: *mut Limb, a: *const Limb, b: *const Limb), | |||
| scalar_mul_mont: fn(r: &mut [Limb], a: &[Limb], b: &[Limb]), | |||
There was a problem hiding this comment.
Ditto: We'll have to leave this as it was so that the P256 implementation can directly call the assembly/C code without any additional overhead.
| for i in 1..rep { | ||
| p256_scalar_mul_mont(r, r, r); | ||
| } | ||
| } |
There was a problem hiding this comment.
Doing this would regress the performance on x86-64 and AArch64 targets that have an optimized implementation of this.
|
Thanks for the contribution. My understanding is that you just wanted to get MIPS and maybe other targets working. They are all working now, so I'm closing this. I appreciate your effort here. |
As part of the effort to support MIPS, we would need to avoid using bn_mul_mont on C code to avoid needing to add MIPS assembly.
This commit attempts a literal port of the C code into Rust using limbs_mont_mul.
I'm opening it early to collect feedback if this is how we should approach it.