Rework crash protection #1427
Rework crash protection #1427
Conversation
src/safeAccess.cpp
Outdated
| u32 SafeAccess::skipLoad(instruction_t* pc) { | ||
| if ((pc >= (void*)load && pc < load_end) || | ||
| (pc >= (void*)load32 && pc < load32_end)) { | ||
| #if defined(__x86_64__) || defined(__i386__) |
There was a problem hiding this comment.
What about push [mem]; pop reg?
There was a problem hiding this comment.
I now generate desired instruction explicitly with inline assembler.
| make COMMIT_TAG=$HASH FAT_BINARY=true release -j | ||
| ;; | ||
| *) | ||
| make COMMIT_TAG=$HASH CC=/usr/local/musl/bin/musl-gcc release coverage -j |
There was a problem hiding this comment.
We can keep the coverage build if we exclude the SafeAccess functions with __attribute__((no_profile_instrument_function)).
There was a problem hiding this comment.
Returned coverage back. Inline assembler solves the problem.
test/native/safeAccessTest.cpp
Outdated
| void* deadbeef = (void*)(uintptr_t)0xdeadbeefdeadbeefULL; | ||
|
|
||
| ASSERT_EQ(SafeAccess::load(bad_ptr), 0); | ||
| ASSERT_EQ(SafeAccess::load(&good_ptr), good_ptr); |
There was a problem hiding this comment.
Can we take address of another variable in the good_ptr declaration, so bad_ptr and good_ptr are both void**? This might make these tests easier to read.
|
OK, I gave up fighting with compiler quirks. Rewrote implementation with inline assembler for x86 and arm64. Other architectures fall back to C++ implementation which is good enough for optimized builds (debug builds probably don't work on these architectures, but we do not officially support them anyway). |
| asm volatile("movl (%1), %0" : "=a"(ret) : "r"(ptr), "S"(default_value)); | ||
| #elif defined(__i386__) | ||
| int32_t ret; | ||
| asm volatile("movl (%1), %0" : "=a"(ret) : "r"(ptr), "a"(default_value)); |
There was a problem hiding this comment.
This can be mov, equivalent to movl x86 32 bits.
There was a problem hiding this comment.
Yes, they are the same on x86-32.
| void* SafeAccess::load(void** ptr, void* default_value) { | ||
| #if defined(__x86_64__) | ||
| void* ret; | ||
| asm volatile("mov (%1), %0" : "=a"(ret) : "r"(ptr), "S"(default_value)); |
There was a problem hiding this comment.
In the output default_value/rsi is optimized away and it is passed in esi not rsi from the caller:
load(void**, void*):
mov rax,QWORD PTR [rdi]
load_end:
ret
...
mov esi,0x99
...
call loadhttps://godbolt.org/z/6nvef3zsb
It seems this could lead to incorrect default_value when higher bits of rsi is not clear, given that StackFrame::arg1 uses rsi.
There was a problem hiding this comment.
In the output default_value/rsi is optimized away
I intentionally use the register designated for the second argument by the ABI (rsi/esi), so that SafeAccess::load compiles to a single mov instruction.
It seems this could lead to incorrect default_value when higher bits of rsi
No: mov esi, int32 clears high bits of rsi.
Compiler uses this instruction because it's smaller in size.
3 participants
Description
Revised implementation of the mechanism for loading from potentially unsafe locations (aka SafeAccess). Previous implementation was fragile and worked incorrectly in certain cases.
Summary of changes
SafeAccessfor constant arguments. This is needed when compiling with clang, which does not supportnocloneattribute.SafeAccess::loadfunctions. Now there are just 2 variants ofloadand a singleskipLoad.Related issues
#1412
How has this been tested?
Added SafeAccess unit tests.
Compiled with gcc and clang with different levels of optimizations.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.