Fix data alignment issues found by UBSan #554
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Many places in our code simply cast arbitrary byte arrays to Soter header structure and expect that to work. Type-punning misaligned data is undefined behavior (and may cause crashes on some ARM systems). Set soter_container_hdr_t to be 1-byte aligned (usually it's expected to have 4-byte alignment). This fixes the inconsistency and forces the compiler to generate safe code. On x86 this does not really change anything since it allows misaligned loads, but ARM code now behaves differently. See for yourself in Godbolt. There is no portable way to set data structure alignment nicely. GCC uses __attribute__((packed)) and Clang support that as well. However, MSVC does not support that and uses a special pragma. Both GCC and Clang support #pragma pack, so let's roll with that.
This reverts commit 0d97f5b (partially) The blacklist has been introduced because there were *too many* functions using soter_container_hdr_t which had incorrect alignment. Since now we use correct alignment for Soter containers, we can enable Undefined Behavior sanitizer back and remove the blacklist. We can also enable UBSan for GCC now. Note that Ed25519 implementation still causes UBSan warnings. We'll deal with that separately.
These functions are written by level 80 crypto C gurus so they are as free from undefined behavior as Chuck Norris. Their behavior is *the* behavior. By definition. Daniel J. Bernstein is never wrong, except the times when he is. Mark all of them with a special attribute that ignores certain warnings from Undefined Behavior Sanitizer. Almost all of them are actually not undefined behavior (but often implementation-defined behavior). The only exception is "shift" which warns about left shift of negative signed values, which is genuine undefined behavior (by C standard). Some places here like to shift "-1" values as signed integers. However, on x86 and ARM this particular behavior is probably okay, so we'll let that slide. (The side effect here is that carry flag may get undefined value and it may affect future conditionals in the code.)
Here instead of converting uint8_t* into a uint64_t* which has different alignment we should use memmove() in order to copy bytes safely. (Don't worry, it will be inlined.)
ilammy
requested review from
Lagovas,
shadinua,
storojs72 and
vixentael
as code owners
so true.
let's look precisely on mobile tests after merging this PR. i'm 99% sure that nothing will break, but just in case |
vixentael
approved these changes
Nov 8, 2019
shadinua
approved these changes
Nov 8, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR #548 has introduced a blacklist for some of the issues found by undefined behavior sanitizer. We have silenced the warnings until we resolved them, and disabled UBSan runs for GCC completely. While that PR has been cooking, I have found a proper way to fix the warnings. That allows us to remove the blacklist and mark the remaining warnings in a way supported by GCC.
Make
soter_container_hdr_tpackedMany places in our code simply cast arbitrary byte arrays to Soter header structure and expect that to work. Type-punning misaligned data is undefined behavior (and may cause crashes on some ARM systems).
Set
soter_container_hdr_tto be 1-byte aligned (usually it's expected to have 4-byte alignment). This fixes the inconsistency and forces the compiler to generate safe code. On x86 this does not really change anything since it allows misaligned loads, but ARM code now behaves differently. See for yourself in Godbolt.Revert "Blacklist some UBSan warnings"
We can also enable UBSan for GCC now.
Mark Ed25519 functions for UBSan
These functions are written by level 80 crypto C gurus so they are as free from undefined behavior as Chuck Norris. Their behavior is the behavior. By definition. Daniel J. Bernstein is never wrong, except the times when he is.
There are caveats (see commit message for details), but they are acceptable given the architectures we support.
Avoid misaligned writes
There were some stray misaligned writes besides from
soter_container_hdr_t. Fix them.Checklist
Benchmark results are attached(not applicable)Public API has proper documentation(not needed, this is a private thing)Example projects and code samples are updated if needed(no API changes)Changelog is updated if needed(nothing interesting for the users)