Align with the other use of `OPENSSL_memcpy` in `curve25519_64_adx.h`.
`string.h` will no longer be needed.
Signed-off-by: Jiaqi Gao <jiaqi.gao@intel.com>
Although it works without these (we just refer the unwinder to the red
zone), older versions of libunwind seem to have a bug that cause it to
flakily fail to restore rbx without this. I've attempted to bisect the
problem, but the issue is very flaky and I've failed to find the culprit
four times now, so just give up and work around it. Explicit restores
match what we do in other files.
Hopefully this will clear some issues tha fiat-crypto's CI are running
into.
Change-Id: I6a19679a37cad8e93e6dee554b6a9b3b9b4bbe4a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62865
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
RBP pointed 8 bytes off of where it should be. I've left the RSP offsets
alone, though it does mean they're shifted by 8 from what they
previously were. Per Andres, the new version of CryptOpt will generate
an RBP-compatible prolog, but for now I've just fixed it up by hand.
(This part was already hand-written.)
Change-Id: I23720e76affff6fae46b8f85b0a509380ccc8bc0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62805
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
This CL adds the Shipped field (and may update the
License File field) in Chromium READMEs. Changes were
automatically created, so if you disagree with any of
them (e.g. a package is used only for testing purposes
and is not shipped), comment the suggested change and
why.
See the LSC doc at go/lsc-chrome-metadata.
Bug: b:285450740
Change-Id: I63755d8a42ea69ff6d3a4e0c21ddacd2b7ce9053
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61545
Auto-Submit: Anne Redulla <aredulla@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This allows operating systems to insist on IBT
enforcement as an exploit mitigation mechanism without
needing to make an exception for anything using a
bundled boringssl, such as chrome, mono, and qtwebengine.
Change-Id: Iac28dd3d2af177b89ffde10ae97bce23739feb94
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60625
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Trying to migrate Chromium to the "link all the asm files together"
strategy broke the aarch64 Android build because some of the ifdef'd out
assembly files were missing the .note.gnu.property section for BTI. If
we add support for IBT, that'll be another one.
To fix this, introduce <openssl/asm_base.h>, which must be included at
the start of every assembly file (before the target ifdefs). This does a
couple things:
- It emits BTI and noexecstack markers into every assembly file, even
those that ifdef themselves out.
- It resolves the MSan -> OPENSSL_NO_ASM logic, so we only need to do it
once.
- It defines the same OPENSSL_X86_64, etc., defines we set elsewhere, so
we can ensure they're consistent.
This required carving files up a bit. <openssl/base.h> has a lot of
things, such that trying to guard everything in it on __ASSEMBLER__
would be tedious. Instead, I moved the target defines to a new
<openssl/target.h>. Then <openssl/asm_base.h> is the new header that
pulls in all those things.
Bug: 542
Change-Id: I1682b4d929adea72908655fa1bb15765a6b3473b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60765
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Change-Id: I824560a5bddb9183e61c663ca4cbdc4530177e66
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60945
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: Andres Erbsen <andreser@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Chromium actually has a checker for unexpected exported symbols, which
caught this.
Change-Id: If1af4c26a3326eea185725f9e84c17f5d9f0f58e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60665
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
__builtin_ia32_addcarryx_u64 is, strictly speaking, an ADX intrinsic.
GCC and newer Clang seem to actually implement it without ADX, but
Clang 7 and older will actually try to generate ADX code with it. But
since the caller is not marked target("adx"), this fails to build.
Manually add ADX and BMI2 target attributes to all these functions. The
compiler should be free to use those instructions as these functions all
call into an ADX+BMI2 assembly function anyway. (Though it doesn't do
much with this.)
Note we cannot just annotate fiat_addcarryx_u64. Clang and GCC won't
inline across incompatible targets, so if we tag fiat_addcarryx_u64, we
need to tag the callers up the chain until we're willing to stop
inlining.
Change-Id: I855bb88fea666d92997984836e664292d90df5be
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60612
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
This fixes the generated Bazel build. Bazel is strict about having all
dependencies declared, which includes files that are #included into
other files. (It also is not particularly pleased about textual
headers and wants them declared in a separate place.)
The new fiat curve25519 assembly is currently split into a BoringSSL
half, and a more generic fiat half. For now, just move the BoringSSL
customizations directly into the fiat half. This isn't ideal, as we'd,
long term, like something where the fiat code can be made standalone.
But, to fix the build, just patch in the changes now and we can ponder
how to do this better later. (Build tools and conventions for assembly
are much less clear than C, sadly.)
Also add the .note.GNU-stack bit at the end.
Change-Id: I04aa733eabf8562dba42dee63a8fd25c86a59db9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60566
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
.type seems to be needed for the unwind tester to report symbols
correctly. I assume it does other useful things. .hidden
(.private_extern on macOS) keeps it from being exported out of shared
libraries. I'm not positive what .size does, but if ELF wants to know
the size of the function, we should probably tell it that.
Change-Id: Iaa2e4f8a72e81092ec1d81ee2177504c7dc89c76
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60465
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Andres Erbsen <andreser@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Did 75000 Ed25519 key generation operations in 1007110us (74470.5 ops/sec) [+26.9%]
Did 72000 Ed25519 signing operations in 1011133us (71207.2 ops/sec) [+25.5%]
Did 78000 Curve25519 base-point multiplication operations in 1006737us (77478.0 ops/sec) [+27.5%]
Change-Id: I32ca2056f42f9b92af315d8381e1b72be69dd331
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60386
Commit-Queue: Andres Erbsen <andreser@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Did 29000 Curve25519 arbitrary point multiplication operations in 1026074us (28263.1 ops/sec) [+31.2%]
Change-Id: I9c7d47a047dc68d37202b6cf40d7d12b5b4936f8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60385
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
The files no longer need to be patched because fiat-crypto now has its
own copy of our value barrier. It does, however, require syncing our
NO_ASM define with fiat's.
fiat-crypto is now licensed under any of MIT, BSD 1-clause, or Apache 2.
I've stuck with the MIT one as that's what we were previously importing.
No measurable perf difference before/after this CL, with GCC or Clang on
x86_64.
Change-Id: I2939fd517de37aabdea3ead49150135200a1b112
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52045
Reviewed-by: Adam Langley <agl@google.com>
Use Fiat Crypto for non-x86_64 platforms, like BoringSSL. Continue
using the nistz256 code on Windows, differently from BoringSSL.
Make *ring* more consistent with BoringSSL.
Originally, when we imported fiat-crypto field operations, the pipeline
was in early stages and the generated code had to be manually integrated
with the rest of the curve implementation, so we moved all our
supporting code to third_party/fiat for simplicity. Over time more
supporting code, like the table generators, landed there to be next to
its callers.
fiat-crypto now generates standalone files which we #include into the
supporting code. This moves the supporting code back to the usual
location. It also updates the README.md file to reflect the new
pipeline. (Most of it was a documentation of the old pipeline, which was
much more manual.)
Change-Id: I64db7235feb6566f0d3cd4db3a7146050edaf25a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/40904
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
I missed a symbol.
Change-Id: I83c6828620a54eaab26cad08b1714402a2758fc0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/40905
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This file gets #included into other files, so we shouldn't use generic
names like 'fe'. This will let us import other fiat-crypto curves in the
future, if we want them.
Change-Id: Ie4e222729bde7e4ccd368b86fb9048a2ea4a58ac
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/40824
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This pulls in the latest upstream files and reapplies our value_barrier patch.
In particular, https://github.com/mit-plv/fiat-crypto/pull/723 made 64-bit
P-256 faster!
32-bit x86, gcc
Before:
Did 3150 ECDH P-256 operations in 4027477us (782.1 ops/sec)
Did 9912 ECDSA P-256 signing operations in 4067212us (2437.1 ops/sec)
Did 3772 ECDSA P-256 verify operations in 4059197us (929.2 ops/sec)
Did 74800 Ed25519 key generation operations in 4020883us (18602.9 ops/sec)
Did 74000 Ed25519 signing operations in 4001827us (18491.6 ops/sec)
Did 21371 Ed25519 verify operations in 4024606us (5310.1 ops/sec)
Did 78000 Curve25519 base-point multiplication operations in 4051574us (19251.8 ops/sec)
Did 25133 Curve25519 arbitrary point multiplication operations in 4063280us (6185.4 ops/sec)
After:
Did 3250 ECDH P-256 operations in 4025179us (807.4 ops/sec) [+3.2%]
Did 10277 ECDSA P-256 signing operations in 4084926us (2515.8 ops/sec) [+3.2%]
Did 3895 ECDSA P-256 verify operations in 4048734us (962.0 ops/sec) [+3.5%]
Did 74480 Ed25519 key generation operations in 4002460us (18608.6 ops/sec) [+0.0%]
Did 74000 Ed25519 signing operations in 4004425us (18479.6 ops/sec) [-0.1%]
Did 21756 Ed25519 verify operations in 4038856us (5386.7 ops/sec) [+1.4%]
Did 78000 Curve25519 base-point multiplication operations in 4031991us (19345.3 ops/sec) [+0.5%]
Did 25133 Curve25519 arbitrary point multiplication operations in 4064925us (6182.9 ops/sec) [-0.0%]
x86_64, clang, OPENSSL_SMALL
Before:
Did 20090 ECDH P-256 operations in 4019408us (4998.2 ops/sec)
Did 56000 ECDSA P-256 signing operations in 4004370us (13984.7 ops/sec)
Did 23562 ECDSA P-256 verify operations in 4062283us (5800.2 ops/sec)
Did 127000 Ed25519 key generation operations in 4005053us (31709.9 ops/sec)
Did 128000 Ed25519 signing operations in 4021902us (31825.7 ops/sec)
Did 71000 Ed25519 verify operations in 4036015us (17591.6 ops/sec)
Did 132000 Curve25519 base-point multiplication operations in 4002101us (32982.7 ops/sec)
Did 93000 Curve25519 arbitrary point multiplication operations in 4023827us (23112.3 ops/sec)
After:
Did 22263 ECDH P-256 operations in 4005099us (5558.7 ops/sec) [+11.2%]
Did 61000 ECDSA P-256 signing operations in 4024810us (15156.0 ops/sec) [+8.4%]
Did 27426 ECDSA P-256 verify operations in 4038547us (6791.1 ops/sec) [+17.1%]
Did 128000 Ed25519 key generation operations in 4015033us (31880.2 ops/sec) [+0.5%]
Did 127000 Ed25519 signing operations in 4003894us (31719.1 ops/sec) [-0.3%]
Did 70000 Ed25519 verify operations in 4017446us (17424.0 ops/sec) [-1.0%]
Did 132000 Curve25519 base-point multiplication operations in 4006282us (32948.3 ops/sec) [-0.1%]
Did 93000 Curve25519 arbitrary point multiplication operations in 4025190us (23104.5 ops/sec) [-0.0%]
Change-Id: I2f705772899c701480ca0e0885e6b75dd1bb1f5d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/40746
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
It is tricky to create EC_FELEMs right now. This will simplify making them.
Change-Id: Icde518efed61381004e2e5569a45d653af48ca2a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/40585
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This fixes two issues. First, we have been lax about whether the
low-level inversion functions fail on zero input or output zero. Fix the
documentation and call the latter inv0 or inverse0 to match the
terminology used in draft-irtf-cfrg-hash-to-curve. (Although we may not
actually need inv0 given the optimization in D.2.)
This has no actual effect because the functions were only used in
contexts where the inputs were already guaranteed to be non-zero. Still,
we should be consistent here.
Second, ec_scalar_inv_montgomery and ec_scalar_inv_montgomery_vartime
claim to perform the same operation, but they do not. First, one
computed inv0 and the other computed inv (except only in some
implementations, so fix it to be consistent). Second, the former
computes inverses in the Montgomery domain, while the latter converts to
the Montgomery domain and then inverts. Rename it to
ec_scalar_to_montgomery_inv_vartime, which is... questionably
understandable but at least looks different.
Change-Id: I9b4829ce5013bdb9528078a093f41b1b158df265
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/40526
Reviewed-by: Adam Langley <agl@google.com>
Clang recognizes the (mask & a) | (~mask & b) pattern as a select. While
it often optimizes this into a cmov, it sometimes inserts branches
instead, particularly when it detects a string of cmovs with the same
condition.
In the long term, we need language-level support for expressing our
constraints. In the short term, introduce value barriers to prevent the
compiler from reasoning about our bit tricks. Thanks to Chandler Carruth
for suggesting this pattern. It should be reasonably robust, short of
value-based PGO or the compiler learning to reason about empty inline
assembly blocks.
Apply barriers to our various constant-time selects. We should invest
more in the valgrind-based tooling to figure out if there are other
instances.
Change-Id: Icc24ce36a61f7fec021a762c27197b9c5bd28c5d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/36484
Reviewed-by: Chandler Carruth <chandlerc@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Point addition formulas for short Weierstrass curves are often
incomplete and do not work for P + P. EC implementations usually rely on
constant-time operations never hitting this case, or at least it being
rare[0].
However, the condition checks several values. Our C functions use && and
||, and the P-256 assembly also branches. This can leak intermediate
values via a small side channel. Thanks to David Schrammel and Samuel
Weiser for reporting this.
nistz256 base point multiplication (keygen, ECDSA signing) is unaffected
due to ecp_nistz256_point_add_affine lacking a doubling case. nistp224
and nistp256 base point multiplication, on some compilers, are saved by
quirks of the "mixed" path. The generic code's base point multiplication
and all methods' arbitrary point multiplication (ECDH; ephemeral keys
makes this less interesting) are affected.
Fix the branches in the nistz256 assembly, and use bit operations in C.
Note the C versions are all different because P-224 believes true is 1,
P-256 believes true is any non-zero value, and the generic code believes
true is 0xf...f. This should be double-checked when reviewing.
Aside: The nistz256 assembly also special-cases nontrivial P + (-P) in
arbitrary point multiplication. Fortunately, the formulas in util.c hold
there and I believe one can show P + (-P) is unreachable for all curves.
Still, it would be nice to omit the branch if we can verify the assembly
works anyway.
[0] 03da376ff7/crypto/ec/ecp_nistp521.c (L1259)
Change-Id: I8958624cd6b5272e5076c6c1605ab089e85f4cb7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/36465
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
See I9c20b660ce4b58dc633588cfd5b2e97a40203ec3. Aside for p224-64.c, we'd
already split mul_public into a dedicated function, at which point it's
simpler to just have three functions.
This makes it clearer when we do and don't care about the doubling case
coming up and avoids a bunch of NULL checks.
Change-Id: I7c5dafa7f12f4f53937d912ba22c90cb5a786f04
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/36225
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
