[PATCH] binfmt_flat: do not stop relocating GOT entries prematurely
Niklas Cassel
Niklas.Cassel at wdc.com
Wed Apr 13 11:13:19 PDT 2022
On Tue, Apr 12, 2022 at 09:52:13AM -0500, Eric W. Biederman wrote:
> Niklas Cassel <Niklas.Cassel at 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
More information about the linux-riscv
mailing list