Commit 3388b79f authored by Chris Dziemborowicz's avatar Chris Dziemborowicz Committed by Commit Bot

Fix crash in GlibcGetSizeEstimate caused by Clang CFI

GlibcGetSizeEstimate calls a function pointer whose value is obtained
dynamically with a call to dlsym. When compiled with Clang's Control
Flow Integrity (CFI) checks (in particular -fsanitize=cfi-icall), the
compiler cannot reason about the contents of the function pointer, so it
elects to emit a ud2 "Undefined Instruction" opcode to kill the process
instead of calling the function:

  00000000055eaa50 <_ZN12_GLOBAL__N_120GlibcGetSizeEstimateEPKN4base9allocator17AllocatorDispatchEPvS5_$bd8608a96952500691d600ccbd08bddd.cfi>:
   55eaa50:   55                      push   %rbp
   55eaa51:   48 89 e5                mov    %rsp,%rbp
   55eaa54:   8a 05 9e e9 dd 05       mov    0x5dde99e(%rip),%al        # b3c93f8 <_ZGVZN12_GLOBAL__N_120GlibcGetSizeEstimateEPKN4base9allocator17AllocatorDispatchEPvS5_E6fn_ptr>
   55eaa5a:   84 c0                   test   %al,%al
   55eaa5c:   75 36                   jne    55eaa94 <_ZN12_GLOBAL__N_120GlibcGetSizeEstimateEPKN4base9allocator17AllocatorDispatchEPvS5_$bd8608a96952500691d600ccbd08bddd.cfi+0x44>
   55eaa5e:   48 8d 3d 93 e9 dd 05    lea    0x5dde993(%rip),%rdi        # b3c93f8 <_ZGVZN12_GLOBAL__N_120GlibcGetSizeEstimateEPKN4base9allocator17AllocatorDispatchEPvS5_E6fn_ptr>
   55eaa65:   e8 56 39 d6 ff          callq  534e3c0 <__cxa_guard_acquire>
   55eaa6a:   85 c0                   test   %eax,%eax
   55eaa6c:   74 26                   je     55eaa94 <_ZN12_GLOBAL__N_120GlibcGetSizeEstimateEPKN4base9allocator17AllocatorDispatchEPvS5_$bd8608a96952500691d600ccbd08bddd.cfi+0x44>
   55eaa6e:   48 8d 35 79 e9 95 fb    lea    -0x46a1687(%rip),%rsi        # f493ee <_ZN5blinkL36kDefaultNativeMemorySamplingIntervalE+0x1d8c8e>
   55eaa75:   48 c7 c7 ff ff ff ff    mov    $0xffffffffffffffff,%rdi
   55eaa7c:   e8 8f 9d 76 05          callq  ad54810 <dlsym@plt>
   55eaa81:   48 89 05 68 e9 dd 05    mov    %rax,0x5dde968(%rip)        # b3c93f0 <_ZZN12_GLOBAL__N_120GlibcGetSizeEstimateEPKN4base9allocator17AllocatorDispatchEPvS5_E6fn_ptr>
   55eaa88:   48 8d 3d 69 e9 dd 05    lea    0x5dde969(%rip),%rdi        # b3c93f8 <_ZGVZN12_GLOBAL__N_120GlibcGetSizeEstimateEPKN4base9allocator17AllocatorDispatchEPvS5_E6fn_ptr>
   55eaa8f:   e8 dc 39 d6 ff          callq  534e470 <__cxa_guard_release>
   55eaa94:   0f 0b                   ud2
   55eaa96:   cc                      int3
   55eaa97:   cc                      int3
   55eaa98:   cc                      int3
   55eaa99:   cc                      int3
   55eaa9a:   cc                      int3
   55eaa9b:   cc                      int3
   55eaa9c:   cc                      int3
   55eaa9d:   cc                      int3
   55eaa9e:   cc                      int3
   55eaa9f:   cc                      int3

This change uses the base::UnsanitizedCfiCall utility to bypass the
cfi-icall check for this function call. This is necessary in cases where
a dynamically loaded function needs to be called.

This change also stores the function pointer in read-only memory using
base::ProtectedMemory, which is the recommended pattern to prevent the
pointer from being modified to hijack control flow. See, for example,
the implementation of GetMonitors [1] and https://crbug.com/771365.

[1] https://cs.chromium.org/chromium/src/ui/base/x/x11_display_util.cc?l=23-58&rcl=5cdcaedb360ea99534aed54eddd552275705f1b9

Note: The issue is reproducible in Chromium Linux x64 builds built with
is_cfi=true (the default) and use_allocator=none (not the default). By
default, Chromium Linux x64 builds use_allocator=tcmalloc instead. The
issue was originally encountered in a build of Chromium Embedded
Framework, which builds with is_cfi=true and use_allocator=none for
Linux x64 builds.

Change-Id: I286db32657b76d8dddd9b99902144cab584e3547
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1868513
Auto-Submit: Chris Dziemborowicz <chrisdz@google.com>
Reviewed-by: default avatarPrimiano Tucci <primiano@chromium.org>
Commit-Queue: Chris Dziemborowicz <chrisdz@google.com>
Cr-Commit-Position: refs/heads/master@{#707328}
parent 3d69076b
...@@ -3,6 +3,7 @@ ...@@ -3,6 +3,7 @@
// found in the LICENSE file. // found in the LICENSE file.
#include "base/allocator/allocator_shim.h" #include "base/allocator/allocator_shim.h"
#include "base/memory/protected_memory_cfi.h"
#include <dlfcn.h> #include <dlfcn.h>
#include <malloc.h> #include <malloc.h>
...@@ -22,6 +23,10 @@ void __libc_free(void* ptr); ...@@ -22,6 +23,10 @@ void __libc_free(void* ptr);
namespace { namespace {
using base::allocator::AllocatorDispatch; using base::allocator::AllocatorDispatch;
using MallocUsableSizeFunction = decltype(malloc_usable_size)*;
PROTECTED_MEMORY_SECTION base::ProtectedMemory<MallocUsableSizeFunction>
g_MallocUsableSizeFunction;
void* GlibcMalloc(const AllocatorDispatch*, size_t size, void* context) { void* GlibcMalloc(const AllocatorDispatch*, size_t size, void* context) {
return __libc_malloc(size); return __libc_malloc(size);
...@@ -59,12 +64,11 @@ size_t GlibcGetSizeEstimate(const AllocatorDispatch*, ...@@ -59,12 +64,11 @@ size_t GlibcGetSizeEstimate(const AllocatorDispatch*,
// resolve it instead. This should be safe because glibc (and hence dlfcn) // resolve it instead. This should be safe because glibc (and hence dlfcn)
// does not use malloc_size internally and so there should not be a risk of // does not use malloc_size internally and so there should not be a risk of
// recursion. // recursion.
using MallocUsableSizeFunction = decltype(malloc_usable_size)*; static base::ProtectedMemory<MallocUsableSizeFunction>::Initializer init(
static MallocUsableSizeFunction fn_ptr = &g_MallocUsableSizeFunction, reinterpret_cast<MallocUsableSizeFunction>(
reinterpret_cast<MallocUsableSizeFunction>( dlsym(RTLD_NEXT, "malloc_usable_size")));
dlsym(RTLD_NEXT, "malloc_usable_size"));
return fn_ptr(address); return base::UnsanitizedCfiCall(g_MallocUsableSizeFunction)(address);
} }
} // namespace } // namespace
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment