From 932a7226236386eaeb968ec97ece4864e2d981c3 Mon Sep 17 00:00:00 2001 From: Dan Klishch Date: Sat, 20 Apr 2024 14:48:44 -0400 Subject: [PATCH] LibC+LibELF: Do not override existing weak symbols during magic lookup Previously, the actual behavior of magic lookup and one described in its commit description have not matched. Instead of being weak definitions in a library that is always in the end of load order, the definitions were normal ones and thus were able to override other weak definitions in LibC. While this was consistent with how DynamicLoader resolves ambiguity between normal and weak relocations, this is not the behavior POSIX mandates -- we should always choose first available definition wrt load order. To fix this problem, the patch makes sure we don't define any of magic symbols in LibC. In addition to this, it makes all provided magic symbols functions (instead of objects), what renders MagicWeakSymbol class unnecessary. --- Userland/DynamicLoader/main.cpp | 17 ++-- .../Libraries/LibC/bits/dlfcn_integration.h | 12 --- Userland/Libraries/LibC/dlfcn.cpp | 10 +-- Userland/Libraries/LibC/libcinit.cpp | 18 +++- Userland/Libraries/LibC/link.cpp | 9 +- Userland/Libraries/LibC/pthread.cpp | 6 +- Userland/Libraries/LibC/ssp.cpp | 4 - Userland/Libraries/LibC/stdlib.cpp | 4 +- Userland/Libraries/LibC/sys/internals.h | 2 +- Userland/Libraries/LibELF/DynamicLinker.cpp | 84 ++++++++----------- 10 files changed, 70 insertions(+), 96 deletions(-) diff --git a/Userland/DynamicLoader/main.cpp b/Userland/DynamicLoader/main.cpp index efaf3eb2111..627658fa466 100644 --- a/Userland/DynamicLoader/main.cpp +++ b/Userland/DynamicLoader/main.cpp @@ -15,16 +15,10 @@ #include char* __static_environ[] = { nullptr }; // We don't get the environment without some libc workarounds.. +char** environ = __static_environ; -static void init_libc() -{ - environ = __static_environ; - __environ_is_malloced = false; - __stdio_is_initialized = false; - // Initialise the copy of libc included statically in Loader.so, - // initialisation of the dynamic libc.so is done by the DynamicLinker - __libc_init(); -} +// FIXME: Kernel should give us a random value for __stack_chk_guard. +uintptr_t __stack_chk_guard = 0xe0e6'066b'b7ea'c300; static void perform_self_relocations(auxv_t* auxvp) { @@ -119,7 +113,10 @@ void _entry(int argc, char** argv, char** envp) auxv_t* auxvp = (auxv_t*)++env; perform_self_relocations(auxvp); - init_libc(); + + // Initialize the copy of libc included statically in Loader.so, + // initialization of the dynamic libc.so is done by the DynamicLinker + __libc_init(0); int main_program_fd = -1; ByteString main_program_path; diff --git a/Userland/Libraries/LibC/bits/dlfcn_integration.h b/Userland/Libraries/LibC/bits/dlfcn_integration.h index 1acf07cf13f..00f3776b105 100644 --- a/Userland/Libraries/LibC/bits/dlfcn_integration.h +++ b/Userland/Libraries/LibC/bits/dlfcn_integration.h @@ -26,15 +26,3 @@ struct DlErrorMessage { struct __Dl_info; typedef struct __Dl_info Dl_info; - -typedef Result (*DlCloseFunction)(void*); -typedef Result (*DlOpenFunction)(char const*, int); -typedef Result (*DlSymFunction)(void*, char const*); -typedef Result (*DlAddrFunction)(void const*, Dl_info*); - -extern "C" { -extern DlCloseFunction __dlclose; -extern DlOpenFunction __dlopen; -extern DlSymFunction __dlsym; -extern DlAddrFunction __dladdr; -} diff --git a/Userland/Libraries/LibC/dlfcn.cpp b/Userland/Libraries/LibC/dlfcn.cpp index ed4a4000074..0fe1ffa401d 100644 --- a/Userland/Libraries/LibC/dlfcn.cpp +++ b/Userland/Libraries/LibC/dlfcn.cpp @@ -5,16 +5,16 @@ */ #include +#include #include #include #include #include -// These are filled in by the dynamic loader. -[[gnu::weak]] DlCloseFunction __dlclose; -[[gnu::weak]] DlOpenFunction __dlopen; -[[gnu::weak]] DlSymFunction __dlsym; -[[gnu::weak]] DlAddrFunction __dladdr; +[[gnu::weak]] Result __dlclose(void*) asm("__dlclose"); +[[gnu::weak]] Result __dlopen(char const*, int) asm("__dlopen"); +[[gnu::weak]] Result __dlsym(void*, char const*) asm("__dlsym"); +[[gnu::weak]] Result __dladdr(void const*, Dl_info*) asm("__dladdr"); // FIXME: use thread_local and a String once TLS works #ifdef NO_TLS diff --git a/Userland/Libraries/LibC/libcinit.cpp b/Userland/Libraries/LibC/libcinit.cpp index dabe2d3d2ca..dafa444683e 100644 --- a/Userland/Libraries/LibC/libcinit.cpp +++ b/Userland/Libraries/LibC/libcinit.cpp @@ -10,6 +10,8 @@ #include #include +[[gnu::weak]] char** __environ_value() asm("__environ_value"); + extern "C" { #ifdef NO_TLS @@ -17,10 +19,14 @@ int errno_storage; #else __thread int errno_storage; #endif -[[gnu::weak]] char** environ; bool __environ_is_malloced = false; -bool __stdio_is_initialized; -void* __auxiliary_vector; +bool __stdio_is_initialized = false; +void* __auxiliary_vector = reinterpret_cast(explode_byte(0xe1)); + +#ifndef _DYNAMIC_LOADER +char** environ = reinterpret_cast(explode_byte(0xe2)); +uintptr_t __stack_chk_guard; +#endif static void __auxiliary_vector_init(); @@ -29,8 +35,12 @@ int* __errno_location() return &errno_storage; } -void __libc_init() +void __libc_init([[maybe_unused]] uintptr_t cookie) { +#ifndef _DYNAMIC_LOADER + __stack_chk_guard = cookie; + environ = __environ_value(); +#endif __auxiliary_vector_init(); __malloc_init(); __stdio_init(); diff --git a/Userland/Libraries/LibC/link.cpp b/Userland/Libraries/LibC/link.cpp index f686ae18fc9..09896048d30 100644 --- a/Userland/Libraries/LibC/link.cpp +++ b/Userland/Libraries/LibC/link.cpp @@ -7,15 +7,10 @@ #include #include -extern "C" { - using DlIteratePhdrCallbackFunction = int (*)(struct dl_phdr_info*, size_t, void*); -using DlIteratePhdrFunction = int (*)(DlIteratePhdrCallbackFunction, void*); +[[gnu::weak]] extern int __dl_iterate_phdr(DlIteratePhdrCallbackFunction, void*) asm("__dl_iterate_phdr"); -[[gnu::weak]] DlIteratePhdrFunction __dl_iterate_phdr; - -int dl_iterate_phdr(int (*callback)(struct dl_phdr_info* info, size_t size, void* data), void* data) +extern "C" int dl_iterate_phdr(int (*callback)(struct dl_phdr_info* info, size_t size, void* data), void* data) { return __dl_iterate_phdr(callback, data); } -} diff --git a/Userland/Libraries/LibC/pthread.cpp b/Userland/Libraries/LibC/pthread.cpp index e84aac481c5..9bf89e4f72d 100644 --- a/Userland/Libraries/LibC/pthread.cpp +++ b/Userland/Libraries/LibC/pthread.cpp @@ -53,10 +53,10 @@ static thread_local SinglyLinkedList cleanup_handlers; static __thread bool pending_cancellation = false; -extern "C" { +[[gnu::weak]] extern ErrorOr __create_new_tls_region() asm("__create_new_tls_region"); +[[gnu::weak]] extern ErrorOr __free_tls_region(FlatPtr thread_pointer) asm("__free_tls_region"); -[[gnu::weak]] ErrorOr (*__create_new_tls_region)(); -[[gnu::weak]] ErrorOr (*__free_tls_region)(FlatPtr thread_pointer); +extern "C" { [[noreturn]] static void exit_thread(void* code, void* stack_location, size_t stack_size) { diff --git a/Userland/Libraries/LibC/ssp.cpp b/Userland/Libraries/LibC/ssp.cpp index 734ca7c561f..43810cddd13 100644 --- a/Userland/Libraries/LibC/ssp.cpp +++ b/Userland/Libraries/LibC/ssp.cpp @@ -17,10 +17,6 @@ extern "C" { -extern uintptr_t __stack_chk_guard; -// Populated by DynamicLinker in shared executables. -[[gnu::weak]] uintptr_t __stack_chk_guard = (uintptr_t)0xc6c7c8c9; - __attribute__((noreturn)) void __stack_chk_fail() { dbgln("Error: USERSPACE({}) Stack protector failure, stack smashing detected!", getpid()); diff --git a/Userland/Libraries/LibC/stdlib.cpp b/Userland/Libraries/LibC/stdlib.cpp index 7e933ee91a7..6bd70e77075 100644 --- a/Userland/Libraries/LibC/stdlib.cpp +++ b/Userland/Libraries/LibC/stdlib.cpp @@ -343,9 +343,9 @@ static T c_str_to_floating_point(char const* str, char** endptr) return 0; } -extern "C" { +[[gnu::weak]] extern void __call_fini_functions() asm("__call_fini_functions"); -[[gnu::weak]] void (*__call_fini_functions)(); +extern "C" { void exit(int status) { diff --git a/Userland/Libraries/LibC/sys/internals.h b/Userland/Libraries/LibC/sys/internals.h index f4624f3d1a8..415675e7451 100644 --- a/Userland/Libraries/LibC/sys/internals.h +++ b/Userland/Libraries/LibC/sys/internals.h @@ -13,7 +13,7 @@ __BEGIN_DECLS typedef void (*AtExitFunction)(void*); -extern void __libc_init(void); +extern void __libc_init(uintptr_t); extern void __malloc_init(void); extern void __stdio_init(void); extern void __begin_atexit_locking(void); diff --git a/Userland/Libraries/LibELF/DynamicLinker.cpp b/Userland/Libraries/LibELF/DynamicLinker.cpp index ac31004c468..16540fda131 100644 --- a/Userland/Libraries/LibELF/DynamicLinker.cpp +++ b/Userland/Libraries/LibELF/DynamicLinker.cpp @@ -31,6 +31,7 @@ #include #include #include +#include #include #include #include @@ -73,32 +74,7 @@ static StringView s_ld_library_path; static StringView s_main_program_pledge_promises; static ByteString s_loader_pledge_promises; -class MagicWeakSymbol : public RefCounted { - AK_MAKE_NONCOPYABLE(MagicWeakSymbol); - AK_MAKE_NONMOVABLE(MagicWeakSymbol); - -public: - template - MagicWeakSymbol(unsigned int type, T value) - { - m_storage = reinterpret_cast(value); - m_lookup_result.size = 8; - m_lookup_result.type = type; - m_lookup_result.address = VirtualAddress { &m_storage }; - m_lookup_result.bind = STB_GLOBAL; - } - - auto lookup_result() const - { - return m_lookup_result; - } - -private: - DynamicObject::SymbolLookupResult m_lookup_result; - uintptr_t m_storage; -}; - -static HashMap> s_magic_weak_symbols; +static HashMap s_magic_functions; Optional DynamicLinker::lookup_global_symbol(StringView name) { @@ -117,8 +93,10 @@ Optional DynamicLinker::lookup_global_symbol( // We don't want to allow local symbols to be pulled in to other modules } - if (auto magic_lookup = s_magic_weak_symbols.get(name); magic_lookup.has_value()) - weak_result = (*magic_lookup)->lookup_result(); + if (!weak_result.has_value()) { + if (auto magic_lookup = s_magic_functions.get(name); magic_lookup.has_value()) + weak_result = *magic_lookup; + } return weak_result; } @@ -344,10 +322,15 @@ static int __dl_iterate_phdr(DlIteratePhdrCallbackFunction callback, void* data) static void initialize_libc(DynamicObject& libc) { + uintptr_t stack_guard = get_random(); + // We include an additional hardening: zero the first byte of the stack guard to avoid leaking + // or overwriting the stack guard with C-style string functions. + stack_guard &= ~0xffULL; + auto res = libc.lookup_symbol("__libc_init"sv); VERIFY(res.has_value()); - typedef void libc_init_func(); - ((libc_init_func*)res.value().address.as_ptr())(); + using libc_init_func = decltype(__libc_init); + ((libc_init_func*)res.value().address.as_ptr())(stack_guard); } template @@ -661,6 +644,11 @@ static void __call_fini_functions() } } +static char** __environ_value() +{ + return s_envp; +} + static void read_environment_variables() { for (char** env = s_envp; *env; ++env) { @@ -692,24 +680,24 @@ void ELF::DynamicLinker::linker_main(ByteString&& main_program_path, int main_pr s_envp = envp; - uintptr_t stack_guard = get_random(); - -#ifdef AK_ARCH_64_BIT - // For 64-bit platforms we include an additional hardening: zero the first byte of the stack guard to avoid - // leaking or overwriting the stack guard with C-style string functions. - stack_guard &= ~0xffULL; -#endif - - s_magic_weak_symbols.set("environ"sv, make_ref_counted(STT_OBJECT, s_envp)); - s_magic_weak_symbols.set("__stack_chk_guard"sv, make_ref_counted(STT_OBJECT, stack_guard)); - s_magic_weak_symbols.set("__call_fini_functions"sv, make_ref_counted(STT_FUNC, __call_fini_functions)); - s_magic_weak_symbols.set("__create_new_tls_region"sv, make_ref_counted(STT_FUNC, __create_new_tls_region)); - s_magic_weak_symbols.set("__free_tls_region"sv, make_ref_counted(STT_FUNC, __free_tls_region)); - s_magic_weak_symbols.set("__dl_iterate_phdr"sv, make_ref_counted(STT_FUNC, __dl_iterate_phdr)); - s_magic_weak_symbols.set("__dlclose"sv, make_ref_counted(STT_FUNC, __dlclose)); - s_magic_weak_symbols.set("__dlopen"sv, make_ref_counted(STT_FUNC, __dlopen)); - s_magic_weak_symbols.set("__dlsym"sv, make_ref_counted(STT_FUNC, __dlsym)); - s_magic_weak_symbols.set("__dladdr"sv, make_ref_counted(STT_FUNC, __dladdr)); + auto define_magic_function = [&](StringView name, auto function) { + s_magic_functions.set(name, + DynamicObject::SymbolLookupResult { + .size = 8, + .address = VirtualAddress { reinterpret_cast(function) }, + .bind = STB_GLOBAL, + .type = STT_FUNC, + }); + }; + define_magic_function("__call_fini_functions"sv, __call_fini_functions); + define_magic_function("__create_new_tls_region"sv, __create_new_tls_region); + define_magic_function("__dl_iterate_phdr"sv, __dl_iterate_phdr); + define_magic_function("__dladdr"sv, __dladdr); + define_magic_function("__dlclose"sv, __dlclose); + define_magic_function("__dlopen"sv, __dlopen); + define_magic_function("__dlsym"sv, __dlsym); + define_magic_function("__environ_value"sv, __environ_value); + define_magic_function("__free_tls_region"sv, __free_tls_region); char* raw_current_directory = getcwd(nullptr, 0); s_cwd = raw_current_directory;