Commit 5c3c4ac8 authored by Vlad Tsyrklevich's avatar Vlad Tsyrklevich Committed by Commit Bot

Revert "[cfi-icall] Use ProtectedMemory for GetProcAddress"

This reverts commit d2cc1555.

Reason for revert: speculative revert, this might cause hangs on Linux component builds due to linker symbol resolution issues.

Original change's description:
> [cfi-icall] Use ProtectedMemory for GetProcAddress
> 
> Control Flow Integrity [1] indirect call (cfi-icall) checking can not
> verify that dynamically resolved function pointers call their intended
> function. Instead we place the pointer for GLGetProcAddress in
> ProtectedMemory, a wrapper for keeping variables in read-only memory
> except for when they are initialized.  After setting the pointer in
> protected memory we can use the UnsanitizedCfiCall wrapper to disable
> cfi-icall checking when calling it since we know it can not be tampered
> with.
> 
> [1] https://www.chromium.org/developers/testing/control-flow-integrity
> 
> Bug: 771365
> Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
> Change-Id: Ia79c1cfab8e00f88bcc437c34cbb3012537c015a
> Reviewed-on: https://chromium-review.googlesource.com/769654
> Commit-Queue: Peter Collingbourne <pcc@chromium.org>
> Reviewed-by: Kenneth Russell <kbr@chromium.org>
> Reviewed-by: Peter Collingbourne <pcc@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#517208}

TBR=kbr@chromium.org,pcc@chromium.org,vtsyrklevich@chromium.org

Change-Id: Ia474d99619795f9b2c40422858caf2a02461e462
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 771365
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/775596Reviewed-by: default avatarPeter Collingbourne <pcc@chromium.org>
Commit-Queue: Peter Collingbourne <pcc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517224}
parent 63e4d43d
...@@ -153,6 +153,8 @@ src:*net/proxy/proxy_config_service_linux.cc ...@@ -153,6 +153,8 @@ src:*net/proxy/proxy_config_service_linux.cc
# Calls to auto-generated stubs by ui/gl/generate_bindings.py # Calls to auto-generated stubs by ui/gl/generate_bindings.py
src:*ui/gl/gl_bindings_autogen_* src:*ui/gl/gl_bindings_autogen_*
# ui/gl/gl_implementation.c
fun:*GetGLProcAddress*
src:*components/os_crypt/* src:*components/os_crypt/*
src:*chrome/browser/password_manager/native_backend_libsecret* src:*chrome/browser/password_manager/native_backend_libsecret*
......
...@@ -12,8 +12,6 @@ ...@@ -12,8 +12,6 @@
#include "base/command_line.h" #include "base/command_line.h"
#include "base/logging.h" #include "base/logging.h"
#include "base/macros.h" #include "base/macros.h"
#include "base/memory/protected_memory.h"
#include "base/memory/protected_memory_cfi.h"
#include "base/memory/ptr_util.h" #include "base/memory/ptr_util.h"
#include "base/stl_util.h" #include "base/stl_util.h"
#include "base/strings/string_piece.h" #include "base/strings/string_piece.h"
...@@ -45,10 +43,7 @@ typedef std::vector<base::NativeLibrary> LibraryArray; ...@@ -45,10 +43,7 @@ typedef std::vector<base::NativeLibrary> LibraryArray;
GLImplementation g_gl_implementation = kGLImplementationNone; GLImplementation g_gl_implementation = kGLImplementationNone;
LibraryArray* g_libraries; LibraryArray* g_libraries;
// Place the function pointer for GetProcAddress in read-only memory after being GLGetProcAddressProc g_get_proc_address;
// resolved to prevent it being tampered with. See crbug.com/771365 for details.
PROTECTED_MEMORY_SECTION base::ProtectedMemory<GLGetProcAddressProc>
g_get_proc_address;
void CleanupNativeLibraries(void* unused) { void CleanupNativeLibraries(void* unused) {
if (g_libraries) { if (g_libraries) {
...@@ -157,8 +152,7 @@ void UnloadGLNativeLibraries() { ...@@ -157,8 +152,7 @@ void UnloadGLNativeLibraries() {
void SetGLGetProcAddressProc(GLGetProcAddressProc proc) { void SetGLGetProcAddressProc(GLGetProcAddressProc proc) {
DCHECK(proc); DCHECK(proc);
auto writer = base::AutoWritableMemory::Create(g_get_proc_address); g_get_proc_address = proc;
*g_get_proc_address = proc;
} }
GLFunctionPointerType GetGLProcAddress(const char* name) { GLFunctionPointerType GetGLProcAddress(const char* name) {
...@@ -172,9 +166,8 @@ GLFunctionPointerType GetGLProcAddress(const char* name) { ...@@ -172,9 +166,8 @@ GLFunctionPointerType GetGLProcAddress(const char* name) {
return proc; return proc;
} }
} }
if (*g_get_proc_address) { if (g_get_proc_address) {
GLFunctionPointerType proc = GLFunctionPointerType proc = g_get_proc_address(name);
base::UnsanitizedCfiCall(g_get_proc_address)(name);
if (proc) if (proc)
return proc; return proc;
} }
......
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