| Message ID | 20220412100338.437308-1-niklas.cassel@wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | binfmt_flat: do not stop relocating GOT entries prematurely | expand |
On Tue, Apr 12, 2022 at 09:52:13AM -0500, Eric W. Biederman wrote: > Niklas Cassel <Niklas.Cassel@wdc.com> writes: (snip) > >> Would it be safer to check that the following rp_val is also -1 ? Also, > >> does this work with 32-bits arch ? Shouldn't the "< 2" be "< 1" for > >> 32-bits arch ? > > > > I think that checking that the previous entry is also -1 will not work, > > as it will just be a single entry for 32-bit. > > And I don't see the need to complicate this logic by having a 64-bit > > and a 32-bit version of the check. > > Handling 64bit in this binfmt_flat appears wrong. The code is > aggressively 32bit, and in at least some places does not have fields > large enough to handle a 64bit address. I expect it would take > a significant rewrite to support 64bit. Running "file" on the ELF supplied to elf2flt shows: ELF 64-bit LSB executable, UCB RISC-V, RVC, double-float ABI, version 1 (SYSV) (The code was compiled with -melf64lriscv.) And the resulting bFLT works perfectly fine with the existing fs/binfmt_flat.c (with the minor fix in $subject applied). The current relocation code probably won't work on systems where it needs to relocate something to an address > 4 GB, but the systems running bFLT rarely have that much memory. The k210 I'm testing on has 8 MB of memory. So I'm not arguing that we should change the u32 pointers to something else, my point was that: https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=bfd/elfnn-riscv.c;hb=binutils-2_38#l3275 bfd_put_NN (output_bfd, (bfd_vma) -1, htab->elf.sgotplt->contents); bfd_put_NN (output_bfd, (bfd_vma) 0, htab->elf.sgotplt->contents + GOT_ENTRY_SIZE); Where NN will be 64 for elf64-riscv and 32 for elf32-riscv: https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=bfd/Makefile.am;hb=binutils-2_38#l878 So while the GOTPLT header will always be two words, it will be 16 bytes ([0xffffffff 0xffffffff] [0x0 0x0]) on elf64-riscv and 8 bytes ([0xffffffff] [0x0]) on elf32-riscv. Both words are reserved for the dynamic linker (ld.so). > > > I think it would be better all-around if instead of applying the > adjustment in the loop, there was a test before the loop. > > Something like: > > static inline u32 __user *skip_got_header(u32 __user *rp) > { > if (IS_ENABLED(CONFIG_RISCV)) { > /* RISCV has a 2 word GOT PLT header */ > u32 rp_val; > if (get_user(rp_val, rp) == 0) { > if (rp_val == 0xffffffff) > rp += 2; > } > } > return rp; > } I like your suggestion, but perhaps change skip_got_header() to: static inline u32 __user *skip_got_header(u32 __user *rp) { if (IS_ENABLED(CONFIG_RISCV)) { /* * RISCV 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; } return rp; } What do you guys think? > > .... > > if (flags & FLAT_FLAG_GOTPIC) { > rp = skip_got_header((u32 * __user) datapos); > for (; ; rp++) { > u32 addr, rp_val; > if (get_user(rp_val, rp)) > return -EFAULT; > if (rp_val == 0xffffffff) > break; > if (rp_val) { > > > Alternately if nothing in the binary uses the header it would probably > be a good idea for elf2flt to simply remove the header. It is used by the dynamic linker (ld.so), so I don't think that we can remove it. The bFLT format only supports shared libraries when CONFIG_BINFMT_SHARED_FLAT. Looking at e.g. buildroot: https://github.com/buildroot/buildroot/blob/2022.02.1/arch/Config.in#L418 it seems that perhaps only m68k supports shared libraries for bFLT, but I suppose that elf2flt wants to keep the linker script the same for all archs. > Looking at the references you have given the only active architecture > supporting this header is riscv. So I think it would be good > documentation to have the functionality conditional upon RISCV. Fine by me. > > There is the very strange thing I see happening in the code. > Looking at the ordinary relocation code it appears that if > FLAT_FLAG_GOTPIC is set that first the address to relocate > is computed, then the address to relocate is read converted > from big endian to native endian (little endian on riscv?) > adjusted and written back. The relocation entries in the GOT are not converted using ntohl(): https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/binfmt_flat.c?h=v5.18-rc2#n799 The extra relocation entries tacked after the image's data segment are only converted using ntohl() if FLAT_FLAG_GOTPIC is _not_ set: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/binfmt_flat.c?h=v5.18-rc2#n851 > > Does elf2flt really change all of these values to big-endian on > little-endian platforms? Short answer, yes, but only for non-PIC code: https://github.com/uclinux-dev/elf2flt/blob/v2021.08/elf2flt.c#L826 The code is horrible to read because of all the ifdefs, I had to compile it with -E to actually see anything. Basically the code ends up looking like this: if (!pic_with_got) { switch (q->howto->type) { default: /* The alignment of the build host might be stricter than that of the target, so be careful. We store in network byte order. */ r_mem[0] = (sym_addr >> 24) & 0xff; r_mem[1] = (sym_addr >> 16) & 0xff; r_mem[2] = (sym_addr >> 8) & 0xff; r_mem[3] = sym_addr & 0xff; } } Kind regards, Niklas
diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c index 626898150011..b80009e6392e 100644 --- a/fs/binfmt_flat.c +++ b/fs/binfmt_flat.c @@ -793,8 +793,17 @@ static int load_flat_file(struct linux_binprm *bprm, u32 addr, rp_val; if (get_user(rp_val, rp)) return -EFAULT; - if (rp_val == 0xffffffff) + /* + * The first word in the GOTPLT header is -1 on certain + * architechtures. (On 64-bit, that is two u32 entries.) + * Ignore these entries, so that we stop relocating GOT + * entries first when we encounter the -1 after the GOT. + */ + if (rp_val == 0xffffffff) { + if (rp - (u32 __user *)datapos < 2) + continue; break; + } if (rp_val) { addr = calc_reloc(rp_val, libinfo, id, 0); if (addr == RELOC_FAILED) {
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 that has the first word (two u32 entries for 64-bit archs) set to -1. See e.g. the binutils implementation for architectures [1] [2] [3] [4]. This causes the binfmt_flat loader to stop relocating GOT entries prematurely and thus causes the application to crash when running. Fix this by ignoring -1 in the first two u32 entries in the .data section. A -1 will only be ignored for the first two entries for bFLT binaries with FLAT_FLAG_GOTPIC set, which is unconditionally set by elf2flt if the supplied ELF binary had the symbol _GLOBAL_OFFSET_TABLE_ defined, therefore ELF binaries without a .got input section should 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;a=blob;f=bfd/elfnn-riscv.c;hb=binutils-2_38#l3275 [2] https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=bfd/elfxx-tilegx.c;hb=binutils-2_38#l4023 [3] https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=bfd/elf32-tilepro.c;hb=binutils-2_38#l3633 [4] https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=bfd/elfnn-loongarch.c;hb=binutils-2_38#l2978 Cc: <stable@vger.kernel.org> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com> --- 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 | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)