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>
For the C files, rather than force the caller to juggle
crypto_linux_sources, etc., we just wrap the whole file in ifdefs and
ask the callers to link everything together.
Assembly is typically built by a different tool, so we have less room
here. However, there are really only two families of tools we care
about: gas (which runs the C preprocessor) and nasm (which has its own
preprocessor). Callers should be able to limit themselves to
special-casing Windows x86(_64) for NASM and then pass all the remaining
assembly files to their gas-like tool. File-wide ifdefs can take care of
the rest.
We're almost set up to allow this, except the files condition on
architecture, but not OS. Add __ELF__, __APPLE__, and _WIN32 conditions
as appropriate.
One subtlety: the semantics of .note.GNU-stack are that *any* unmarked
object file makes the stack executable. (In current GNU ld. lld doesn't
have this issue, and GNU ld claims they'll remove it in a later
release.) Empirically, this doesn't seem to apply to empty object files
but, to be safe, we should ensure all object files have the marking.
That leads to a second subtlety: on targets where @ is a comment,
@progbits is spelled %progbits, per [0]. If we want all .S files to work
in all targets, that includes these markers. Fortunately, %progbits
appears to work universally (see [1], [2], [3], [4]), so I've just
switched us to that spelling.
I've also tightened up the __arm__ and __aarch64__ checks to __ARMEL__
and __AARCH64EL__. We don't support big-endian Arm (or any other
platform) and, even if we did, the conditions in the assembly files
should match the conditions in the C files that pull them in.
This CL doesn't change our build to take advantage of this (though I'll
give it a go later), just makes it possible for builds to do it.
[0] https://sourceware.org/binutils/docs/as/Section.html
[1] https://patchwork.kernel.org/project/linux-crypto/patch/20170119212805.18049-1-dvlasenk@redhat.com/#20050285
[2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92820#c11
[3] https://sourceware.org/legacy-ml/gdb-patches/2016-01/msg00319.html
[4] de990b270d
Bug: 542
Change-Id: I0a8ded24423087c0da13bd0335cbd757d4eee65a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55626
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
I had a rewrite of the decrepit ciphers (CAST and Blowfish) to use
CRYPTO_{load,store}_u32_be and drop the old macros, but this is probably
not worth the effort to review. Instead, just fix the type in the macro.
Bug: 516
Change-Id: I1cdecc16f6108a6235f90cf9c2198bc797c6716e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54985
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Change-Id: I5e1d37106d7df8e8aaede295e8eb74c971553fd5
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54365
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
The C11 change has survived for three months now. Let's start freely
using static_assert. In C files, we need to include <assert.h> because
it is a macro. In C++ files, it is a keyword and we can just use it. (In
MSVC C, it is actually also a keyword as in C++, but close enough.)
I moved one assert from ssl3.h to ssl_lib.cc. We haven't yet required
C11 in our public headers, just our internal files.
Change-Id: Ic59978be43b699f2c997858179a9691606784ea5
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53665
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
These symbols were not marked OPENSSL_EXPORT, so they weren't really
usable externally anyway. They're also very sensitive to various build
configuration toggles, which don't always get reflected into projects
that include our headers. Move them to crypto/internal.h.
Change-Id: I79a1fcf0b24e398d75a9cc6473bae28ec85cb835
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50846
Reviewed-by: Adam Langley <agl@google.com>
Also use a slightly more conservative pattern. Instead of aligning the
pointer as a uintptr_t and casting back, compute the offset and advance
in pointer space. C guarantees that casting from pointer to uintptr_t
and back gives the same pointer, but general integer-to-pointer
conversions are generally implementation-defined. GCC does define it in
the useful way, but this makes fewer dependencies.
Change-Id: I70c7af735e892fe7a8333b78b39d7b1f3f1cdbef
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48405
Reviewed-by: Adam Langley <alangley@gmail.com>
Revert the names used in the BoringSSL C/asm code to the names used in
BoringSSL. This substantially reduces the diff between *ring* and
BoringSSL for these files.
Use a variant of BoringSSL's symbol prefixing machinery to semi-
automatically prefix FFI symbols with the `GFp_` prefix. The names aren't
all exactly the same as before, because previously we *replaced* a
symbol's original prefix with the `GFp_` prefix; now we're prepending
`GFp_`. In the future we'll use a different prefix entirely.
This paves the way for using different prefixes for each version so that
multiple versions of *ring* can be linked into an executable at once.
Clarify that there are no truncation issues on targets where the range
of |unsigned| is smaller than the range of |size_t|.
Ensure that |poly1305_state| is (still) large enough. This is a good
idea independently of this change, but is especially important because
switching the fields to |size_t| might have enlarged the structures.
Change-Id: I16e408229c28fcba6c3592603ddb9431cf1f142d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/44244
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Previously the OpenSSL implementation was being used. Switch to the BoringSSL
version.
Switching to the BoringSSL implementation will make it easier to refactor the CPU feature
detection, which is important for upcoming ports.
This switch will also implicitly add support for BTI and pointer authentication for
Poly1305.
This is based on BoringSSL 63d06626d3a104868eee622e8e56d9f2dd643366.
poly1305_vec.c requires SSE2 intrinsics and uint128_t. Unlike MSVC, clang-cl
supports uint128_t just fine (as long as we do not do division). This makes
ChaCha20-Poly1305 much faster on Windows when built with clang-cl.
Before:
Did 2219000 ChaCha20-Poly1305 (16 bytes) seal operations in 1016000us (2184055.1 ops/sec): 34.9 MB/s
Did 1279500 ChaCha20-Poly1305 (256 bytes) seal operations in 1016000us (1259350.4 ops/sec): 322.4 MB/s
Did 428250 ChaCha20-Poly1305 (1350 bytes) seal operations in 1015000us (421921.2 ops/sec): 569.6 MB/s
Did 84000 ChaCha20-Poly1305 (8192 bytes) seal operations in 1016000us (82677.2 ops/sec): 677.3 MB/s
Did 39750 ChaCha20-Poly1305 (16384 bytes) seal operations in 1015000us (39162.6 ops/sec): 641.6 MB/s
After:
Did 2096250 ChaCha20-Poly1305 (16 bytes) seal operations in 1015000us (2065270.9 ops/sec): 33.0 MB/s
Did 1453250 ChaCha20-Poly1305 (256 bytes) seal operations in 1016000us (1430364.2 ops/sec): 366.2 MB/s
Did 642500 ChaCha20-Poly1305 (1350 bytes) seal operations in 1015000us (633004.9 ops/sec): 854.6 MB/s
Did 136250 ChaCha20-Poly1305 (8192 bytes) seal operations in 1016000us (134104.3 ops/sec): 1098.6 MB/s
Did 69750 ChaCha20-Poly1305 (16384 bytes) seal operations in 1016000us (68651.6 ops/sec): 1124.8 MB/s
(Benchmarks gathered in VM, but this is a significant difference.)
Change-Id: Ia0a856e75995623c5621d2e48d61d945c41b17de
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/39345
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This dates to https://boringssl-review.googlesource.com/2850, which was
done in response to an ARM crash. I assume the ARM crash was due to
poly1305_arm.c casting pointers around, which is technically UB, even on
x86 since C says it is UB to cast pointers if the value would be
unaligned. (Also I believe it's a strict aliasing violation, though the
compilers really ought to give us a sanitizer for it if they're excited
about that optimization.)
Replace with memcpy, which any reasonable compiler would compile the
same on platforms that support unaligned access. ARM does support it
these days, so perhaps the crash came from an older ARM?
Benchmarks showed no difference with this CL.
Change-Id: I022bdb84f95e45c143ad19359f646ee1416d5ae9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/39344
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
C's specification text around pointer arithmetic is buggy and fails to
account for empty spans. Empty spans are typically represented as
ptr=NULL and len=0, so (T*)NULL + 0 must be defined for ptr + len to
reliably work. C++ does not have this bug and specifies this correctly.
See https://crbug.com/1019588.
This language bug has made its way over to newer versions of UBSan,
which enforce this. In the short term, add bogus length checks as a
workaround. However, unlike the memcpy language bug, we cannot address
this systematically. In the long term, we need to switch libcrypto to
building as C++ for a real fix.
To test this, update our clang revision to that in
https://chromium-review.googlesource.com/c/chromium/src/+/1879890. Note
that clang revision was later reverted in Chromium for seemingly
unrelated reasons.
This newer UBSan also catches a memcpy/OPENSSL_memcpy issue in
siphash.c, from the earlier C NULL bug we'd been working around.
Bug: chromium:1019588, chromium:1019644
Change-Id: I460e547c8cd740db68da8cc2a3a970276ec92e90
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/38584
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
When building with |OPENSSL_NO_ASM|, the section that marks assembly
files as no-exec-stack will currently be omitted. That results in an
empty assembly file but that's still enough to trigger warnings:
warning: crypto_tests/trampoline-x86_64.o: missing .note.GNU-stack section implies executable stack
This change makes it so that the section marker will always be emitted,
even if the file is otherwise empty.
Change-Id: I2d08d34ed9dbe9e9592c88dcd42d3ba4fa3d7652
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/38084
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
GNU-based toolchains on ELF platforms default the stack to executable
and rely on a .note.GNU-stack section in *each* object file to flip it
off. The compiler knows to do this for its object files, but assembly
does everything by hand. See this link for details:
https://www.airs.com/blog/archives/518
We do this in the cmake build by passing -Wa,--noexecstack to the
assembler. However, since we have to deal with many buildsystems, it
would be more robust to put it in the source.
It's unclear whether this should be gated on ELF or Linux. The Gentoo
and Ubuntu documents recommend checking for Linux with gas, but only ELF
with NASM.
https://wiki.gentoo.org/wiki/Hardened/GNU_stack_quickstarthttps://wiki.ubuntu.com/SecurityTeam/Roadmap/ExecutableStacks
At the same time, these links suggest it is an ELF-wide issue and not
just Linux:
https://github.com/golang/go/issues/5392https://reviews.freebsd.org/D11033https://github.com/openssl/openssl/issues/4575 also discusses this but
the rationale lists both ELF and non-ELF platforms, so it's unclear.
Treat it as ELF-wide for now. We can revisit this if necessary.
Update-Note: If there is a build failure due to .note.GNU-stack, holler.
Change-Id: Ic59096aa1fc2bf5380a412c9991de22cb46c0faf
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/37984
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
MSAN is incompatible with hand-written assembly code. Previously we
required that OPENSSL_NO_ASM be set when building with MSAN, and the
CMake build would take care of this. However, with other build systems
it wasn't always so easy.
This change automatically disables assembly when the compiler is
configured for MSAN.
Change-Id: I6c219120f62d16b99bafc2efb02948ecbecaf87f
Reviewed-on: https://boringssl-review.googlesource.com/31724
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>
The assembly files need some includes. Also evp.h has some conflicting
macros. Finally, md5.c's pattern of checking if a function name is
defined needs to switch to checking MD5_ASM.
Change-Id: Ib1987ba6f279144f0505f6951dead53968e05f20
Reviewed-on: https://boringssl-review.googlesource.com/31704
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
The fipsmodule is still separate as that's a lot of build mess. (Though
that too may be worth pulling in eventually. CMake usually has different
opinions on generated files if they're in the same directory. We might
be able to avoid the set_source_properties(GENERATED) thing.)
Change-Id: Ie1f9345009044d4f0e7541ca779e01bdc5ad62f6
Reviewed-on: https://boringssl-review.googlesource.com/31586
Reviewed-by: Adam Langley <agl@google.com>
We currently write a mix of "if (FOO)" and "if(FOO)". While the former looks
more like a usual language, CMake believes everything, even "if" and "else", is
just a really really funny function call (a "command").
We should pick something for consistency. Upstream CMake writes "if(FOO)", so
go with that one.
Change-Id: I67e0eb650a52670110b417312a362c9f161c8721
Reviewed-on: https://boringssl-review.googlesource.com/30807
Reviewed-by: Adam Langley <agl@google.com>
This avoids upsetting the C compiler. UBSan is offended by the alignment
violations in those functions. The business with offset is also
undefined behavior (pointer arithmetic is supposed to stay within a
single object).
There is a small performance cost, however:
Before:
Did 6636000 ChaCha20-Poly1305 (16 bytes) seal operations in 5000475us (1327073.9 ops/sec): 21.2 MB/s
Did 832000 ChaCha20-Poly1305 (1350 bytes) seal operations in 5003481us (166284.2 ops/sec): 224.5 MB/s
Did 155000 ChaCha20-Poly1305 (8192 bytes) seal operations in 5026933us (30833.9 ops/sec): 252.6 MB/s
After:
Did 6508000 ChaCha20-Poly1305 (16 bytes) seal operations in 5000160us (1301558.4 ops/sec): 20.8 MB/s
Did 831000 ChaCha20-Poly1305 (1350 bytes) seal operations in 5002865us (166104.8 ops/sec): 224.2 MB/s
Did 155000 ChaCha20-Poly1305 (8192 bytes) seal operations in 5013204us (30918.4 ops/sec): 253.3 MB/s
(Tested with the no-asm build which disables the custom stitched mode
assembly and ends up using this one.)
Change-Id: I76d74183f1e04ad3726463a8871ee64be04ce674
Reviewed-on: https://boringssl-review.googlesource.com/22784
Reviewed-by: Adam Langley <agl@google.com>
crypto/{asn1,x509,x509v3,pem} were skipped as they are still OpenSSL
style.
Change-Id: I3cd9a60e1cb483a981aca325041f3fbce294247c
Reviewed-on: https://boringssl-review.googlesource.com/19504
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
The built-in CMake support seems to basically work, though it believes
you want to build a fat binary which doesn't work with how we build
perlasm. (We'd need to stop conditioning on CMAKE_SYSTEM_PROCESSOR at
all, wrap all the generated assembly files in ifdefs, and convince the
build to emit more than one. Probably not worth bothering for now.)
We still, of course, need to actually test the assembly on iOS before
this can be shipped anywhere.
BUG=48
Change-Id: I6ae71d98d706be03142b82f7844d1c9b02a2b832
Reviewed-on: https://boringssl-review.googlesource.com/14645
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
(These files weren't being built anyway.)
Change-Id: Id6c8d211b9ef867bdb7d83153458f9ad4e29e525
Reviewed-on: https://boringssl-review.googlesource.com/13205
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Most C standard library functions are undefined if passed NULL, even
when the corresponding length is zero. This gives them (and, in turn,
all functions which call them) surprising behavior on empty arrays.
Some compilers will miscompile code due to this rule. See also
https://www.imperialviolet.org/2016/06/26/nonnull.html
Add OPENSSL_memcpy, etc., wrappers which avoid this problem.
BUG=23
Change-Id: I95f42b23e92945af0e681264fffaf578e7f8465e
Reviewed-on: https://boringssl-review.googlesource.com/12928
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
We don't support big-endian so this could only slow down whatever
platforms weren't listed in the #if.
Change-Id: Ie36f862663d947f591dd4896e6a2ab19122bbc0d
Reviewed-on: https://boringssl-review.googlesource.com/12202
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
The Poly1305 state defined in the header file is just a 512-byte buffer.
The vector code aligns to 64 bytes but the non-vector code did not.
Since we have lots of space to spare, this change causes the non-vector
code to also align to 64 bytes.
Change-Id: I77e26616a709e770d6eb23df47d9e292742625d7
Reviewed-on: https://boringssl-review.googlesource.com/12201
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
