On Thu, Apr 14, 2022 at 11:10:18AM +0200, Niklas Cassel wrote:
bFLT binaries are usually created using elf2flt.
The linker script used by elf2flt has defined the .data section like the following for the last 19 years:
.data : { _sdata = . ; __data_start = . ; data_start = . ; *(.got.plt) *(.got) FILL(0) ; . = ALIGN(0x20) ; LONG(-1) . = ALIGN(0x20) ; ... }
It places the .got.plt input section before the .got input section. The same is true for the default linker script (ld --verbose) on most architectures except x86/x86-64.
The binfmt_flat loader should relocate all GOT entries until it encounters a -1 (the LONG(-1) in the linker script).
The problem is that the .got.plt input section starts with a GOTPLT header (which has size 16 bytes on elf64-riscv and 8 bytes on elf32-riscv), where the first word is set to -1. See the binutils implementation for riscv [1].
This causes the binfmt_flat loader to stop relocating GOT entries prematurely and thus causes the application to crash when running.
Fix this by skipping the whole GOTPLT header, since the whole GOTPLT header is reserved for the dynamic linker.
The GOTPLT header will only be skipped for bFLT binaries with flag FLAT_FLAG_GOTPIC set. This flag is unconditionally set by elf2flt if the supplied ELF binary has the symbol _GLOBAL_OFFSET_TABLE_ defined. ELF binaries without a .got input section should thus remain unaffected.
Tested on RISC-V Canaan Kendryte K210 and RISC-V QEMU nommu_virt_defconfig.
[1] https://sourceware.org/git/?p=binutils-gdb.git%3Ba=blob%3Bf=bfd/elfnn-riscv....
Cc: stable@vger.kernel.org Signed-off-by: Niklas Cassel niklas.cassel@wdc.com
Changes since v1: -Incorporated review comments from Eric Biederman.
Thanks! nit: please include a link to the archive so it's easier for people (and b4) to track earlier versions. i.e.: https://lore.kernel.org/linux-mm/20220412100338.437308-1-niklas.cassel@wdc.c...
RISC-V elf2flt patches are still not merged, they can be found here: https://github.com/floatious/elf2flt/tree/riscv
buildroot branch for k210 nommu (including this patch and elf2flt patches): https://github.com/floatious/buildroot/tree/k210-v14
fs/binfmt_flat.c | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-)
diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c index 626898150011..e5e2a03b39c1 100644 --- a/fs/binfmt_flat.c +++ b/fs/binfmt_flat.c @@ -440,6 +440,30 @@ static void old_reloc(unsigned long rl) /****************************************************************************/ +static inline u32 __user *skip_got_header(u32 __user *rp) +{
- if (IS_ENABLED(CONFIG_RISCV)) {
/** RISC-V has a 16 byte GOT PLT header for elf64-riscv* and 8 byte GOT PLT header for elf32-riscv.* Skip the whole GOT PLT header, since it is reserved* for the dynamic linker (ld.so).*/u32 rp_val0, rp_val1;if (get_user(rp_val0, rp))return rp;if (get_user(rp_val1, rp + 1))return rp;if (rp_val0 == 0xffffffff && rp_val1 == 0xffffffff)rp += 4;else if (rp_val0 == 0xffffffff)rp += 2;
Just so I understand; due to the FILL(0) and the ALIGN, val1 will be 0 (or more specifically, not -1) in all other cases, yes?
I probably would have written this as:
if (rp_val0 == 0xffffffff) rp += 2; if (rp_val1 == 0xffffffff) rp += 2;
But no need to change it. I expect the compiler would optimize it in the same thing anyway. :)
- }
- return rp;
+}
static int load_flat_file(struct linux_binprm *bprm, struct lib_info *libinfo, int id, unsigned long *extra_stack) { @@ -789,7 +813,8 @@ static int load_flat_file(struct linux_binprm *bprm, * image. */ if (flags & FLAT_FLAG_GOTPIC) {
for (rp = (u32 __user *)datapos; ; rp++) {
rp = skip_got_header((u32 * __user) datapos);for (; ; rp++) { u32 addr, rp_val; if (get_user(rp_val, rp)) return -EFAULT;-- 2.35.1
Eric and Damien, does this look good to you? I'll take this into the execve tree unless you'd like to see further changes.
Thanks!
-Kees