On Tue, Apr 12, 2022 at 09:52:13AM -0500, Eric W. Biederman wrote: > Niklas Cassel <Niklas.Cassel@xxxxxxx> 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
