|author||David Benjamin <email@example.com>||Mon Jan 28 04:06:57 2019 +0000|
|committer||CQ bot account: firstname.lastname@example.org <email@example.com>||Mon Jan 28 21:09:40 2019 +0000|
Test and fix an ABI issue with small parameters. 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 <firstname.lastname@example.org> Reviewed-by: Adam Langley <email@example.com>
BoringSSL is a fork of OpenSSL that is designed to meet Google's needs.
Although BoringSSL is an open source project, it is not intended for general use, as OpenSSL is. We don't recommend that third parties depend upon it. Doing so is likely to be frustrating because there are no guarantees of API or ABI stability.
Programs ship their own copies of BoringSSL when they use it and we update everything as needed when deciding to make API changes. This allows us to mostly avoid compromises in the name of compatibility. It works for us, but it may not work for you.
BoringSSL arose because Google used OpenSSL for many years in various ways and, over time, built up a large number of patches that were maintained while tracking upstream OpenSSL. As Google's product portfolio became more complex, more copies of OpenSSL sprung up and the effort involved in maintaining all these patches in multiple places was growing steadily.
Currently BoringSSL is the SSL library in Chrome/Chromium, Android (but it's not part of the NDK) and a number of other apps/programs.
There are other files in this directory which might be helpful: