Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/50629 )
Change subject: rmodtool: Make memlayout symbols absolute and do not relocate them ......................................................................
rmodtool: Make memlayout symbols absolute and do not relocate them
Memlayout is a mechanism to define memory areas outside the normal program segment constructed by the linker. Therefore, it generally doesn't make sense to relocate memlayout symbols when the program is relocated. They tend to refer to things that are always in one specific spot, independent of where the program is loaded.
This hasn't really hurt us in the past because the use case we have for rmodules (ramstage on x86) just happens to not really need to refer to any memlayout-defined areas at the moment. But that use case may come up in the future so it's still worth fixing.
This patch declares all memlayout-defined symbols as ABSOLUTE() in the linker, which is then reflected in the symbol table of the generated ELF. We can then use that distinction to have rmodtool skip them when generating the relocation table for an rmodule. (Also rearrange rmodtool a little to make the primary string table more easily accessible to the rest of the code, so we can refer to symbol names in debug output.)
A similar problem can come up with userspace unit tests, but we cannot modify the userspace relocation toolchain (and for unfortunate historical reasons, it tries to relocate even absolute symbols). We'll just disable PIC and make those binaries fully static to avoid that issue.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: Ic51d9add3dc463495282b365c1b6d4a9bf11dbf2 Reviewed-on: https://review.coreboot.org/c/coreboot/+/50629 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org --- M src/include/memlayout.h M tests/Makefile.inc M util/cbfstool/rmodule.c M util/cbfstool/rmodule.h 4 files changed, 56 insertions(+), 38 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved
diff --git a/src/include/memlayout.h b/src/include/memlayout.h index bf830b7..424a28a 100644 --- a/src/include/memlayout.h +++ b/src/include/memlayout.h @@ -31,7 +31,7 @@
#define SYMBOL(name, addr) \ SET_COUNTER(name, addr) \ - _##name = .; + _##name = ABSOLUTE(.);
#define REGION(name, addr, size, expected_align) \ SYMBOL(name, addr) \ @@ -40,8 +40,8 @@ SYMBOL(e##name, addr + size)
#define ALIAS_REGION(name, alias) \ - _##alias = _##name; \ - _e##alias = _e##name; + _##alias = ABSOLUTE(_##name); \ + _e##alias = ABSOLUTE(_e##name); \
/* Declare according to SRAM/DRAM ranges in SoC hardware-defined address map. */ #define SRAM_START(addr) SYMBOL(sram, addr) @@ -92,7 +92,7 @@ #if ENV_DECOMPRESSOR #define DECOMPRESSOR(addr, sz) \ SYMBOL(decompressor, addr) \ - _edecompressor = _decompressor + sz; \ + _edecompressor = ABSOLUTE(_decompressor + sz); \ _ = ASSERT(_eprogram - _program <= sz, \ STR(decompressor exceeded its allotted size! (sz))); \ INCLUDE "decompressor/lib/program.ld" @@ -112,7 +112,7 @@ #if ENV_BOOTBLOCK #define BOOTBLOCK(addr, sz) \ SYMBOL(bootblock, addr) \ - _ebootblock = _bootblock + sz; \ + _ebootblock = ABSOLUTE(_bootblock + sz); \ _ = ASSERT(_eprogram - _program <= sz, \ STR(Bootblock exceeded its allotted size! (sz))); \ INCLUDE "bootblock/lib/program.ld" @@ -124,7 +124,7 @@ #if ENV_ROMSTAGE #define ROMSTAGE(addr, sz) \ SYMBOL(romstage, addr) \ - _eromstage = _romstage + sz; \ + _eromstage = ABSOLUTE(_romstage + sz); \ _ = ASSERT(_eprogram - _program <= sz, \ STR(Romstage exceeded its allotted size! (sz))); \ INCLUDE "romstage/lib/program.ld" @@ -136,7 +136,7 @@ #if ENV_RAMSTAGE #define RAMSTAGE(addr, sz) \ SYMBOL(ramstage, addr) \ - _eramstage = _ramstage + sz; \ + _eramstage = ABSOLUTE(_ramstage + sz); \ _ = ASSERT(_eprogram - _program <= sz, \ STR(Ramstage exceeded its allotted size! (sz))); \ INCLUDE "ramstage/lib/program.ld" @@ -160,7 +160,7 @@ #if ENV_SEPARATE_VERSTAGE #define VERSTAGE(addr, sz) \ SYMBOL(verstage, addr) \ - _everstage = _verstage + sz; \ + _everstage = ABSOLUTE(_verstage + sz); \ _ = ASSERT(_eprogram - _program <= sz, \ STR(Verstage exceeded its allotted size! (sz))); \ INCLUDE "verstage/lib/program.ld" @@ -179,7 +179,7 @@ #if ENV_POSTCAR #define POSTCAR(addr, sz) \ SYMBOL(postcar, addr) \ - _epostcar = _postcar + sz; \ + _epostcar = ABSOLUTE(_postcar + sz); \ _ = ASSERT(_eprogram - _program <= sz, \ STR(Aftercar exceeded its allotted size! (sz))); \ INCLUDE "postcar/lib/program.ld" diff --git a/tests/Makefile.inc b/tests/Makefile.inc index 56d5578..a10e9bf 100644 --- a/tests/Makefile.inc +++ b/tests/Makefile.inc @@ -48,6 +48,10 @@ TEST_LDFLAGS = -L$(cmockaobj)/src -lcmocka -Wl,-rpath=$(cmockaobj)/src TEST_LDFLAGS += -Wl,--gc-sections
+# Some memlayout symbols don't work with userspace relocation -- disable it. +TEST_CFLAGS += -fno-pie -fno-pic +TEST_LDFLAGS += -no-pie + # Extra attributes for unit tests, declared per test attributes:= srcs cflags mocks stage
diff --git a/util/cbfstool/rmodule.c b/util/cbfstool/rmodule.c index 429bbf3..258a4d8 100644 --- a/util/cbfstool/rmodule.c +++ b/util/cbfstool/rmodule.c @@ -72,7 +72,7 @@
/* Only these 6 relocations are expected to be found. */ return (type == R_ARM_ABS32 || type == R_ARM_THM_PC22 || - type == R_ARM_THM_JUMP24 || type == R_ARM_V4BX || + type == R_ARM_THM_JUMP24 || type == R_ARM_V4BX || type == R_ARM_CALL || type == R_ARM_JUMP24); }
@@ -137,6 +137,19 @@ }, };
+static int relocation_for_absolute_symbol(struct rmod_context *ctx, Elf64_Rela *r) +{ + Elf64_Sym *s = &ctx->pelf.syms[ELF64_R_SYM(r->r_info)]; + + if (s->st_shndx == SHN_ABS) { + DEBUG("Omitting relocation for absolute symbol: %s\n", + &ctx->strtab[s->st_name]); + return 1; + } + + return 0; +} + /* * Relocation processing loops. */ @@ -172,6 +185,9 @@ return -1; }
+ if (relocation_for_absolute_symbol(ctx, r)) + continue; + /* Allow the provided filter to have precedence. */ if (f != NULL) { filter_emit = f->filter(f, r); @@ -341,7 +357,7 @@
static int populate_sym(struct rmod_context *ctx, const char *sym_name, Elf64_Addr *addr, - int nsyms, const char *strtab, int optional) + int nsyms, int optional) { int i; Elf64_Sym *syms; @@ -351,7 +367,7 @@ for (i = 0; i < nsyms; i++) { if (syms[i].st_name == 0) continue; - if (strcmp(sym_name, &strtab[syms[i].st_name])) + if (strcmp(sym_name, &ctx->strtab[syms[i].st_name])) continue; DEBUG("%s -> 0x%llx\n", sym_name, (long long)syms[i].st_value); *addr = syms[i].st_value; @@ -371,7 +387,6 @@ static int populate_rmodule_info(struct rmod_context *ctx) { int i; - const char *strtab; struct parsed_elf *pelf; Elf64_Ehdr *ehdr; int nsyms; @@ -379,23 +394,6 @@ pelf = &ctx->pelf; ehdr = &pelf->ehdr;
- /* Obtain the string table. */ - strtab = NULL; - for (i = 0; i < ehdr->e_shnum; i++) { - if (ctx->pelf.strtabs[i] == NULL) - continue; - /* Don't use the section headers' string table. */ - if (i == ehdr->e_shstrndx) - continue; - strtab = buffer_get(ctx->pelf.strtabs[i]); - break; - } - - if (strtab == NULL) { - ERROR("No string table found.\n"); - return -1; - } - /* Determine number of symbols. */ nsyms = 0; for (i = 0; i < ehdr->e_shnum; i++) { @@ -406,18 +404,16 @@ break; }
- if (populate_sym(ctx, "_rmodule_params", &ctx->parameters_begin, - nsyms, strtab, 1)) + if (populate_sym(ctx, "_rmodule_params", &ctx->parameters_begin, nsyms, 1)) return -1;
- if (populate_sym(ctx, "_ermodule_params", &ctx->parameters_end, - nsyms, strtab, 1)) + if (populate_sym(ctx, "_ermodule_params", &ctx->parameters_end, nsyms, 1)) return -1;
- if (populate_sym(ctx, "_bss", &ctx->bss_begin, nsyms, strtab, 0)) + if (populate_sym(ctx, "_bss", &ctx->bss_begin, nsyms, 0)) return -1;
- if (populate_sym(ctx, "_ebss", &ctx->bss_end, nsyms, strtab, 0)) + if (populate_sym(ctx, "_ebss", &ctx->bss_end, nsyms, 0)) return -1;
return 0; @@ -425,7 +421,7 @@
static int add_section(struct elf_writer *ew, struct buffer *data, const char *name, - Elf64_Addr addr, Elf64_Word size) + Elf64_Addr addr, Elf64_Word size) { Elf64_Shdr shdr; int ret; @@ -452,7 +448,7 @@
static int write_elf(const struct rmod_context *ctx, const struct buffer *in, - struct buffer *out) + struct buffer *out) { int ret; int bit64; @@ -658,6 +654,22 @@ else ctx->xdr = &xdr_le;
+ /* Obtain the string table. */ + for (i = 0; i < pelf->ehdr.e_shnum; i++) { + if (pelf->strtabs[i] == NULL) + continue; + /* Don't use the section headers' string table. */ + if (i == pelf->ehdr.e_shstrndx) + continue; + ctx->strtab = buffer_get(pelf->strtabs[i]); + break; + } + + if (ctx->strtab == NULL) { + ERROR("No string table found.\n"); + return -1; + } + if (find_program_segment(ctx)) goto out;
diff --git a/util/cbfstool/rmodule.h b/util/cbfstool/rmodule.h index a62562b..ec0971e 100644 --- a/util/cbfstool/rmodule.h +++ b/util/cbfstool/rmodule.h @@ -29,6 +29,8 @@ struct parsed_elf pelf; /* Program segment. */ Elf64_Phdr *phdr; + /* Symbol string table. */ + char *strtab;
/* Collection of relocation addresses fixup in the module. */ Elf64_Xword nrelocs;