From 6ea1ff702e03197211ffeedda0cbe0ea8cb15f43 Mon Sep 17 00:00:00 2001 From: Derek Bruening Date: Tue, 10 Dec 2024 16:07:51 -0500 Subject: [PATCH 1/2] i#7120: Do not add addend for rela jump slot relocation Despite "rela" relocations having an explicit addend value and it being set to non-0 in a new Debian glibc, the addend is assumed to *not* be added to the symbol value when relocating on x86_64 and aarch64 (it does seem to be added on RISCV). This is not obvious and not well documented; we have to just behave like existing loaders behave from experimentation/examination. (Yet another reason to possibly invert the private loader and let the private copy of ld.so do all the loading and relocating: #5437). Tested on a machine where nearly every client test in our suite was crashing after a glibc update: now they pass. Unfortunately it's not simple to make automated tests for this: we don't have an existing framework for relocation testing and it would take non-trivial effort to construct that, beyond the scope of this fix. Fixes #7120 --- core/unix/loader.c | 8 +++++--- core/unix/module_elf.c | 22 +++++++++++++++++----- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/core/unix/loader.c b/core/unix/loader.c index f011287725a..bfbf8ddcc57 100644 --- a/core/unix/loader.c +++ b/core/unix/loader.c @@ -787,8 +787,9 @@ privload_os_finalize(privmod_t *privmod) GLRO_dl_tls_static_size_OFFS = last_large_load_offs; GLRO_dl_tls_static_align_OFFS = last_large_load_offs + sizeof(void *); LOG(GLOBAL, LOG_LOADER, 2, - "%s: for glibc 2.34+ workaround found offsets 0x%x 0x%x\n", __FUNCTION__, - GLRO_dl_tls_static_size_OFFS, GLRO_dl_tls_static_align_OFFS); + "%s: for glibc 2.34+ workaround found offsets 0x%x 0x%x for glro %p\n", + __FUNCTION__, GLRO_dl_tls_static_size_OFFS, GLRO_dl_tls_static_align_OFFS, + glro); } # endif if (GLRO_dl_tls_static_size_OFFS == 0) { @@ -827,7 +828,8 @@ privload_os_finalize(privmod_t *privmod) LOG(GLOBAL, LOG_LOADER, 2, "%s: glibc 2.34+ workaround succeeded\n", __FUNCTION__); } - LOG(GLOBAL, LOG_LOADER, 2, "%s: calling %s\n", __FUNCTION__, LIBC_EARLY_INIT_NAME); + LOG(GLOBAL, LOG_LOADER, 2, "%s: calling %s @%p\n", __FUNCTION__, LIBC_EARLY_INIT_NAME, + libc_early_init); (*libc_early_init)(true); #endif /* LINUX */ } diff --git a/core/unix/module_elf.c b/core/unix/module_elf.c index 2a3ae3977cc..d896ae1badb 100644 --- a/core/unix/module_elf.c +++ b/core/unix/module_elf.c @@ -1,5 +1,5 @@ /* ******************************************************************************* - * Copyright (c) 2012-2022 Google, Inc. All rights reserved. + * Copyright (c) 2012-2024 Google, Inc. All rights reserved. * Copyright (c) 2011 Massachusetts Institute of Technology All rights reserved. * Copyright (c) 2008-2010 VMware, Inc. All rights reserved. * *******************************************************************************/ @@ -1466,8 +1466,8 @@ module_relocate_symbol(ELF_REL_TYPE *rel, os_privmod_data_t *pd, bool is_rela) const char *name; bool resolved; - /* XXX: we assume ELF_REL_TYPE and ELF_RELA_TYPE only differ at the end, - * i.e. with or without r_addend. + /* ELF_REL_TYPE and ELF_RELA_TYPE differ in where the addend comes from: + * stored in the target location, or in rel->r_addend. */ if (is_rela) addend = ((ELF_RELA_TYPE *)rel)->r_addend; @@ -1486,7 +1486,8 @@ module_relocate_symbol(ELF_REL_TYPE *rel, os_privmod_data_t *pd, bool is_rela) ".so has relocation inside PT_DYNAMIC section"); r_type = (uint)ELF_R_TYPE(rel->r_info); - LOG(GLOBAL, LOG_LOADER, 5, "%s: reloc @ %p type=%d\n", r_addr, r_type); + LOG(GLOBAL, LOG_LOADER, 5, "%s: reloc @ %p type=%d is_rela=%d addend=0x%zx\n", + __FUNCTION__, r_addr, r_type, is_rela, addend); /* handle the most common case, i.e. ELF_R_RELATIVE */ if (r_type == ELF_R_RELATIVE) { @@ -1581,7 +1582,18 @@ module_relocate_symbol(ELF_REL_TYPE *rel, os_privmod_data_t *pd, bool is_rela) #ifndef RISCV64 /* FIXME i#3544: Check whether ELF_R_DIRECT with !is_rela is OK */ case ELF_R_GLOB_DAT: #endif - case ELF_R_JUMP_SLOT: *r_addr = (reg_t)res + addend; break; + case ELF_R_JUMP_SLOT: + // Neither aarch64 nor x86_64 add the addend for these types, yet riscv does. + // This is not obvious and not well documented; we have to just behave like + // existing loaders behave from experimentation/examination. + // Yet another reason to possibly invert the private loader and let + // the private copy of ld.so do all the loading and relocating: i#5437. +#if defined(AARCH64) || defined(X86) + *r_addr = (reg_t)res; +#else + *r_addr = (reg_t)res + addend; +#endif + break; case ELF_R_DIRECT: *r_addr = (reg_t)res + (is_rela ? addend : *r_addr); break; case ELF_R_COPY: if (sym != NULL) From ed773678aea440ed569985d6765a9dc1ae82db89 Mon Sep 17 00:00:00 2001 From: Derek Bruening Date: Tue, 10 Dec 2024 19:51:50 -0500 Subject: [PATCH 2/2] Update comment in privload_relocate_symbol to match --- core/unix/loader.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/unix/loader.c b/core/unix/loader.c index bfbf8ddcc57..66fae21d8c6 100644 --- a/core/unix/loader.c +++ b/core/unix/loader.c @@ -1215,8 +1215,8 @@ privload_relocate_symbol(ELF_REL_TYPE *rel, os_privmod_data_t *opd, bool is_rela uint r_type; reg_t addend; - /* XXX: we assume ELF_REL_TYPE and ELF_RELA_TYPE only differ at the end, - * i.e. with or without r_addend. + /* ELF_REL_TYPE and ELF_RELA_TYPE differ in where the addend comes from: + * stored in the target location, or in rel->r_addend. */ if (is_rela) addend = ((ELF_RELA_TYPE *)rel)->r_addend;