Re: [PATCH] binfmt_flat: do not stop relocating GOT entries prematurely

[Niklas Cassel <Niklas.Cassel@wdc.com>]

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