From b9f7966e001ca19241d6a87472b1999bea1cde37 Mon Sep 17 00:00:00 2001 From: Tim Schumacher Date: Thu, 30 Jun 2022 11:31:56 +0200 Subject: [PATCH] LibC: Move stack canary initialization before the global constructors Once again, QEMU creates threads while running its constructors, which is a recipe for disaster if we switch out the stack guard while that is already running in the background. To solve that, move initialization to our LibC initialization stage, which is before any actual external initialization code runs. --- Userland/Libraries/LibC/crt0.cpp | 16 ---------------- Userland/Libraries/LibELF/DynamicLinker.cpp | 6 ++++++ 2 files changed, 6 insertions(+), 16 deletions(-) diff --git a/Userland/Libraries/LibC/crt0.cpp b/Userland/Libraries/LibC/crt0.cpp index 9324674514..afe1a86d9b 100644 --- a/Userland/Libraries/LibC/crt0.cpp +++ b/Userland/Libraries/LibC/crt0.cpp @@ -32,17 +32,6 @@ NAKED void _start(int, char**, char**) int _entry(int argc, char** argv, char** env) { - size_t original_stack_chk = __stack_chk_guard; - - // We can't directly overwrite __stack_chk_guard using arc4random_buf, - // as it doesn't know that the stack canary changed and it would instantly - // cause a stack protector failure when returning. - size_t new_stack_chk = 0; - arc4random_buf(&new_stack_chk, sizeof(new_stack_chk)); - - if (new_stack_chk != 0) - __stack_chk_guard = new_stack_chk; - environ = env; __environ_is_malloced = false; __begin_atexit_locking(); @@ -55,11 +44,6 @@ int _entry(int argc, char** argv, char** env) exit(status); - // We should never get here, but if we ever do, make sure to - // restore the stack guard to the value we entered _start with. - // Then we won't trigger the stack canary check on the way out. - __stack_chk_guard = original_stack_chk; - return 20150614; } } diff --git a/Userland/Libraries/LibELF/DynamicLinker.cpp b/Userland/Libraries/LibELF/DynamicLinker.cpp index 7e1a6800db..5e1fe96a20 100644 --- a/Userland/Libraries/LibELF/DynamicLinker.cpp +++ b/Userland/Libraries/LibELF/DynamicLinker.cpp @@ -259,6 +259,12 @@ static void initialize_libc(DynamicObject& libc) VERIFY(res.has_value()); *((char***)res.value().address.as_ptr()) = s_envp; + // __stack_chk_guard should be initialized before anything significant (read: global constructors) is running. + // This is not done in __libc_init, as we definitely have to return from that, and it might affect Loader as well. + res = libc.lookup_symbol("__stack_chk_guard"sv); + VERIFY(res.has_value()); + arc4random_buf(res.value().address.as_ptr(), sizeof(size_t)); + res = libc.lookup_symbol("__environ_is_malloced"sv); VERIFY(res.has_value()); *((bool*)res.value().address.as_ptr()) = false;