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%3Ba=blob%3Bf=bfd/elfnn-riscv....
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%3Ba=blob%3Bf=bfd/Makefile.am%...
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/b...
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/b...
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