arithmetic internals: Restore old signature of bn_mul_mont. #2253
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2253 +/- ##
=======================================
Coverage 96.81% 96.82%
=======================================
Files 168 168
Lines 20720 20740 +20
Branches 483 487 +4
=======================================
+ Hits 20061 20082 +21
Misses 554 554
+ Partials 105 104 -1 ☔ View full report in Codecov by Sentry. |
| // |num| must be at least 4, at least on x86. | ||
| // BoringSSL documents the return value as being 1 or 0, but some | ||
| // implementations (e.g. 32-bit ARM) return different values on | ||
| // success. |
There was a problem hiding this comment.
In very recent versions of BoringSSL, where bn_mul_mont is written in C, it actually does return 1 or 0 as defined in BoringSSL's version of bn/internal.h. However, before that refactoring was done, it was documented to return 1 or 0, but it would sometimes return non-zero values.
In particular, 32-bit ARM bn_mul_mont_nohw looks like it returns a pointer, which will be non-NULL, instead of 1 or 0. Looks like the NEON version does the same.
I think it would be better to change BoringSSL to do what ring is currently doing, namely just making all the bn_mul_mont_* functions that bn_mul_mont calls return void. Especially since there's no great way to handle them failing when we've met the preconditions.
WDYT @davidben?
There was a problem hiding this comment.
In particular, we need to go through all the 64-bit implementations and ensure that they aren't returning a pointer that gets interpreted as a 32-bit int, because it is possible for (int)p == 0 when p != 0 for a pointer p, when sizeof(int) < sizeof(void*).
There was a problem hiding this comment.
Also, maybe we could remove the extra instruction in each implementation that truncates num to 32-bits while we're at it.
There was a problem hiding this comment.
It seems like 32-bit ARM is the only unusual case.
I did verify that the aarch64 version does mov x0,#1 in each of the three functions that implement bn_mul_mont.
Also, I verified in x86_64-mont.pl that there is a mov \$1,%rax just prior to the epilogue of each function.
In x86-mont.pl, the last instruction of the function is &mov ("eax",1);
(I verified that all the functions in x86_4-mont5.pl, which aren't actually bn_mul_mont have a void return type. )
There was a problem hiding this comment.
Hmm? Not sure I'm following the pointer business. Here's 32-bit Arm.
https://boringssl.googlesource.com/boringssl/+/refs/heads/master/crypto/fipsmodule/bn/asm/armv4-mont.pl#262
In BoringSSL, it returns `int`. Have it do that again, to make the next bit of refactoring easier, where we replace `bn_mul_mont` with a Rust function that dispatches to a bunch of other functions, all of which also are declared as having the same semantics. The good thing is that this restores the assembly code to match BoringSSL. The bad thing is that this some bits of dead code to handle zero return values that we'll never see.
|
It is just as much work to ensure that every implementation correctly sets the return value as it is to change things to safely let them just assume that |
In BoringSSL, it returns
int. Have it do that again, to make the next bit of refactoring easier, where we replacebn_mul_montwith a Rust function that dispatches to a bunch of other functions, all of which also are declared as having the same semantics.The good thing is that this restores the assembly code to match BoringSSL.
The bad thing is that this some bits of dead code to handle zero return values that we'll never see.