Calling conventions must specify how to handle arguments smaller than a
machine word. Should the caller pad them up to a machine word size with
predictable values (zero/sign-extended), or should the callee tolerate
an arbitrary bit pattern?
Annoyingly, I found no text in either SysV or Win64 ABI documentation
describing any of this and resorted to experiment. The short answer is
that callees must tolerate an arbitrary bit pattern on x86_64, which
means we must test this. See the comment in abi_test::internal::ToWord
for the long answer.
CHECK_ABI now, if the type of the parameter is smaller than
crypto_word_t, fills the remaining bytes with 0xaa. This is so the
number is out of bounds for code expecting either zero or sign
extension. (Not that crypto assembly has any business seeing negative
numbers.)
Doing so reveals a bug in ecp_nistz256_ord_sqr_mont. The rep parameter
is typed int, but the code expected uint64_t. In practice, the compiler
will always compile this correctly because:
- On both Win64 and SysV, rep is a register parameter.
- The rep parameter is always a constant, so the compiler has no reason
to leave garbage in the upper half.
However, I was indeed able to get a bug out of GCC via:
uint64_t foo = (1ull << 63) | 2; // Some global the compiler can't
// prove constant.
ecp_nistz256_ord_sqr_mont(res, a, foo >> 1);
Were ecp_nistz256_ord_sqr_mont a true int-taking function, this would
act like ecp_nistz256_ord_sqr_mont(res, a, 1). Instead, it hung. Fix
this by having it take a full-width word.
This mess has several consequences:
- ABI testing now ideally needs a functional testing component to fully cover
this case. A bad input might merely produce the wrong answer. Still,
this is fairly effective as it will cause most code to either segfault
or loop forever. (Not the enc parameter to AES however...)
- We cannot freely change the type of assembly function prototypes. If the
prototype says int or unsigned, it must be ignoring the upper half and
thus "fixing" it to size_t cannot have handled the full range. (Unless
it was simply wrong of the parameter is already bounded.) If the
prototype says size_t, switching to int or unsigned will hit this type
of bug. The former is a safer failure mode though.
- The simplest path out of this mess: new assembly code should *only*
ever take word-sized parameters. This is not a tall order as the bad
parameters are usually ints that should have been size_t.
Calling conventions are hard.
Change-Id: If8254aff8953844679fbce4bd3e345e5e2fa5213
Reviewed-on: https://boringssl-review.googlesource.com/c/34627
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
While gdb can figure it out, libunwind requires CFI directives to
unwind a leaf function, even though the directives are trivial.
Adding them matches what GCC outputs, and likely gdb has many
heuristics that less complex tools (e.g. profilers) may not.
Bug: 181
Change-Id: I25c72152de33109a29710a828aeb99c608dd0470
Reviewed-on: https://boringssl-review.googlesource.com/c/33964
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This was also caught by the in-progress unwind tester. There are two
issues here. First, .cfi_endproc must come after ret to fully cover the
function. More importantly, this function is confused about whether it
has a frame pointer or not.
It looks like it does (movq %rsp, %rbp), and annotates accordingly, but
it does not actually use the frame pointer. It cannot. $y4 is rbp and
gets clobbered immediately after the preamble!
Remove this instruction and align the CFI annotations with a
frame-pointer-less function.
Bug: 181
Change-Id: I47b5f9798b3bcee1748e537b21c173d312a14b42
Reviewed-on: https://boringssl-review.googlesource.com/c/33947
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This was caught by in-progress work to test unwind information. It was
incorrect at two instructions: immediately before we jump to
.Lpoint_double_shortcut$x. This is needed because
ecp_nistz256_point_add$x tries to be clever about not unwinding the
stack frame in its tail call.
It's also unlikely that the SEH handlers in this file are correct at
this point, but that will be handled separately while overhauling
everything else here. (For Win64, probably the only ABI-compliant option
is to just properly unwind the stack frame. Without a custom handler,
Win64 unwind codes are very restrictive.)
Bug: 181
Change-Id: I9f576d868850312d6c14d1386f8fbfa85021b347
Reviewed-on: https://boringssl-review.googlesource.com/c/33946
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This reverts commit e907ed4c4b. CPUID
checks have been added so hopefully this time sticks.
Change-Id: I5e0e5b87427c1230132681f936b3c70bac8263b8
Reviewed-on: https://boringssl-review.googlesource.com/c/32924
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This reverts commit 3d450d2844. It fails
SDE, looks like a missing CPUID check before using vector instructions.
Change-Id: I6b7dd71d9e5b1f509d2e018bd8be38c973476b4e
Reviewed-on: https://boringssl-review.googlesource.com/c/32864
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
This commit improves the performance of ECDSA signature verification
(over NIST P-256 curve) for x86 platforms. The speedup is by a factor of 1.15x.
It does so by:
1) Leveraging the fact that the verification does not need
to run in constant time. To this end, we implemented:
a) the function ecp_nistz256_points_mul_public in a similar way to
the current ecp_nistz256_points_mul function by removing its constant
time features.
b) the Binary Extended Euclidean Algorithm (BEEU) in x86 assembly to
replace the current modular inverse function used for the inversion.
2) The last step in the ECDSA_verify function compares the (x) affine
coordinate with the signature (r) value. Converting x from the Jacobian's
representation to the affine coordinate requires to perform one inversions
(x_affine = x * z^(-2)). We save this inversion and speed up the computations
by instead bringing r to x (r_jacobian = r*z^2) which is faster.
The measured results are:
Before (on a Kaby Lake desktop with gcc-5):
Did 26000 ECDSA P-224 signing operations in 1002372us (25938.5 ops/sec)
Did 11000 ECDSA P-224 verify operations in 1043821us (10538.2 ops/sec)
Did 55000 ECDSA P-256 signing operations in 1017560us (54050.9 ops/sec)
Did 17000 ECDSA P-256 verify operations in 1051280us (16170.8 ops/sec)
After (on a Kaby Lake desktop with gcc-5):
Did 27000 ECDSA P-224 signing operations in 1011287us (26698.7 ops/sec)
Did 11640 ECDSA P-224 verify operations in 1076698us (10810.8 ops/sec)
Did 55000 ECDSA P-256 signing operations in 1016880us (54087.0 ops/sec)
Did 20000 ECDSA P-256 verify operations in 1038736us (19254.2 ops/sec)
Before (on a Skylake server platform with gcc-5):
Did 25000 ECDSA P-224 signing operations in 1021651us (24470.2 ops/sec)
Did 10373 ECDSA P-224 verify operations in 1046563us (9911.5 ops/sec)
Did 50000 ECDSA P-256 signing operations in 1002774us (49861.7 ops/sec)
Did 15000 ECDSA P-256 verify operations in 1006471us (14903.6 ops/sec)
After (on a Skylake server platform with gcc-5):
Did 25000 ECDSA P-224 signing operations in 1020958us (24486.8 ops/sec)
Did 10373 ECDSA P-224 verify operations in 1046359us (9913.4 ops/sec)
Did 50000 ECDSA P-256 signing operations in 1003996us (49801.0 ops/sec)
Did 18000 ECDSA P-256 verify operations in 1021604us (17619.4 ops/sec)
Developers and authors:
***************************************************************************
Nir Drucker (1,2), Shay Gueron (1,2)
(1) Amazon Web Services Inc.
(2) University of Haifa, Israel
***************************************************************************
Change-Id: Idd42a7bc40626bce974ea000b61fdb5bad33851c
Reviewed-on: https://boringssl-review.googlesource.com/c/31304
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Build (and carry) issues are now resolved (as far as we know). Let's try
this again...
Measurements on a Skylake VM (so a little noisy).
Before:
Did 3135 RSA 2048 signing operations in 3015866us (1039.5 ops/sec)
Did 89000 RSA 2048 verify (same key) operations in 3007271us (29594.9 ops/sec)
Did 66000 RSA 2048 verify (fresh key) operations in 3014363us (21895.2 ops/sec)
Did 324 RSA 4096 signing operations in 3004364us (107.8 ops/sec)
Did 23126 RSA 4096 verify (same key) operations in 3003398us (7699.9 ops/sec)
Did 21312 RSA 4096 verify (fresh key) operations in 3017043us (7063.9 ops/sec)
Did 31040 ECDH P-256 operations in 3024273us (10263.6 ops/sec)
Did 91000 ECDSA P-256 signing operations in 3019740us (30135.0 ops/sec)
Did 25678 ECDSA P-256 verify operations in 3046975us (8427.4 ops/sec)
After:
Did 3640 RSA 2048 signing operations in 3035845us (1199.0 ops/sec)
Did 129000 RSA 2048 verify (same key) operations in 3003691us (42947.2 ops/sec)
Did 105000 RSA 2048 verify (fresh key) operations in 3029935us (34654.2 ops/sec)
Did 510 RSA 4096 signing operations in 3014096us (169.2 ops/sec)
Did 38000 RSA 4096 verify (same key) operations in 3092814us (12286.5 ops/sec)
Did 34221 RSA 4096 verify (fresh key) operations in 3003817us (11392.5 ops/sec)
Did 38000 ECDH P-256 operations in 3061758us (12411.2 ops/sec)
Did 116000 ECDSA P-256 signing operations in 3001637us (38645.6 ops/sec)
Did 35100 ECDSA P-256 verify operations in 3023872us (11607.6 ops/sec)
Tested with Intel SDE.
Change-Id: Ib27c0d6012d14274e331ab03f958e5a0c8b7e885
Reviewed-on: https://boringssl-review.googlesource.com/28104
Reviewed-by: Adam Langley <agl@google.com>
This imports the assembly portion of
eb7916960bf50f436593abe3d5f2e0592d291017 from upstream. Note the
OPENSSL_ia32cap_P bits were tweaked to be delocate-compatible. Those
should be reviewed against the original file.
Change-Id: I19eef722225bb7928275e3d93890f80aa2f8734d
Reviewed-on: https://boringssl-review.googlesource.com/27589
Reviewed-by: Adam Langley <alangley@gmail.com>
This was done by OpenSSL with the kind permission of Intel. This change
is imported from upstream's commit
dcf6e50f48e6bab92dcd2dacb27fc17c0de34199.
Change-Id: Ie8d3b700cd527a6e8cf66e0728051b2acd8cc6b9
Reviewed-on: https://boringssl-review.googlesource.com/25588
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
This imports 384e6de4c7e35e37fb3d6fbeb32ddcb5eb0d3d3f and
79ca382d4762c58c4b92fceb4e202e90c71292ae from upstream.
Differences from upstream:
- We've removed a number of unused functions.
- We never imported 3ff08e1dde56747011a702a9a5aae06cfa8ae5fc, which was
to give the assembly control over the memory layout in the tables. So
our "gather" is "select" (which is implemented the same because the
memory layout never did change) and our "scatter" is in C.
Change-Id: I90d4a17da9f5f693f4dc4706887dec15f010071b
Reviewed-on: https://boringssl-review.googlesource.com/25586
Reviewed-by: Adam Langley <agl@google.com>
As of upstream's 6aa36e8e5a062e31543e7796f0351ff9628832ce, the
corresponding file in OpenSSL has both an Intel and OpenSSL copyright
blocks. To properly sync up with OpenSSL, use the OpenSSL copyright
block and our version of the Intel copyright block.
Change-Id: I4dc072a11390a54d0ce38ec0b8893e48f52638de
Reviewed-on: https://boringssl-review.googlesource.com/25585
Reviewed-by: Adam Langley <agl@google.com>
Using ADX instructions requires relatively new assemblers. Conscrypt are
currently using Yasm 1.2.0. Revert these for the time being to unbreak
their build.
Change-Id: Iaba5761ccedcafaffb5ca79a8eaf7fa565583c32
Reviewed-on: https://boringssl-review.googlesource.com/19244
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
We can test these with Intel SDE now. The AVX2 code just affects the two
select functions while the ADX code is a separate implementation.
Haswell numbers:
Before:
Did 84630 ECDH P-256 operations in 10031494us (8436.4 ops/sec)
Did 206000 ECDSA P-256 signing operations in 10015055us (20569.0 ops/sec)
Did 77256 ECDSA P-256 verify operations in 10064556us (7676.0 ops/sec)
After:
Did 86112 ECDH P-256 operations in 10015008us (8598.3 ops/sec)
Did 211000 ECDSA P-256 signing operations in 10025104us (21047.2 ops/sec)
Did 79344 ECDSA P-256 verify operations in 10017076us (7920.9 ops/sec)
Skylake numbers:
Before:
Did 75684 ECDH P-256 operations in 10016019us (7556.3 ops/sec)
Did 185000 ECDSA P-256 signing operations in 10012090us (18477.7 ops/sec)
Did 72885 ECDSA P-256 verify operations in 10027154us (7268.8 ops/sec)
After:
Did 89598 ECDH P-256 operations in 10032162us (8931.1 ops/sec)
Did 203000 ECDSA P-256 signing operations in 10019739us (20260.0 ops/sec)
Did 87040 ECDSA P-256 verify operations in 10000441us (8703.6 ops/sec)
The code was slightly patched for delocate.go compatibility.
Change-Id: Ic44ced4eca65c656bbe07d5a7fee91ec6925eb59
Reviewed-on: https://boringssl-review.googlesource.com/18967
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Drop some redundant instructions in reduction in ecp_nistz256_sqr_montx.
(Imported from upstream's 8fc063dcc9668589fd95533d25932396d60987f9.)
I believe this is a no-op for us as we do not currently enable the
ADX-based optimizations.
Change-Id: I34a5f5ffb965d59c67f6b9f0ca7937e49ba6e820
Reviewed-on: https://boringssl-review.googlesource.com/16884
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
The names in the P-224 code collided with the P-256 code and thus many
of the functions and constants in the P-224 code have been prefixed.
Change-Id: I6bcd304640c539d0483d129d5eaf1702894929a8
Reviewed-on: https://boringssl-review.googlesource.com/15847
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
