| Message ID | 20220412100338.437308-1-niklas.cassel@wdc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | binfmt_flat: do not stop relocating GOT entries prematurely | expand |
On 4/12/22 19:03, Niklas Cassel wrote: > 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(-) > > 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. > + */ /* * The first word in the GOTPLT header is -1 on certain * architectures (on 64-bit, that is two u32 entries). * Ignore these entries so that we stop relocating GOT * entries when we encounter the first -1 entry after * the GOTPLT header. */ > + if (rp_val == 0xffffffff) { > + if (rp - (u32 __user *)datapos < 2) > + continue; 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 ? > break; > + } > if (rp_val) { > addr = calc_reloc(rp_val, libinfo, id, 0); > if (addr == RELOC_FAILED) {
On Tue, Apr 12, 2022 at 08:40:27PM +0900, Damien Le Moal wrote: > On 4/12/22 19:03, Niklas Cassel wrote: > > 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(-) > > > > 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. > > + */ > > /* > * The first word in the GOTPLT header is -1 on certain > * architectures (on 64-bit, that is two u32 entries). > * Ignore these entries so that we stop relocating GOT > * entries when we encounter the first -1 entry after > * the GOTPLT header. > */ Sure, I can update the comment when I send a v2. > > > + if (rp_val == 0xffffffff) { > > + if (rp - (u32 __user *)datapos < 2) > > + continue; > > 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. The whole GOT (.got.plt + .got) will be more than two words anyway, if there is a GOT (i.e. if flag FLAT_FLAG_GOTPIC is set in the bFLT binary), so the "end of GOT"/LONG(-1) will always come way after these first two entries anyway. Another reason why I don't fancy a 64-bit and 32-bit version is because some architectures might be 64-bit, but I assume that they can be running a 32-bit userland. (And in comparison with the ELF header that tells if the binary is 32-bit or 64-bit, I don't see something similar in the bFLT header.) Kind regards, Niklas
Niklas Cassel <Niklas.Cassel@wdc.com> writes: > On Tue, Apr 12, 2022 at 08:40:27PM +0900, Damien Le Moal wrote: >> On 4/12/22 19:03, Niklas Cassel wrote: >> > 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(-) >> > >> > 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. >> > + */ >> >> /* >> * The first word in the GOTPLT header is -1 on certain >> * architectures (on 64-bit, that is two u32 entries). >> * Ignore these entries so that we stop relocating GOT >> * entries when we encounter the first -1 entry after >> * the GOTPLT header. >> */ > > Sure, I can update the comment when I send a v2. > >> >> > + if (rp_val == 0xffffffff) { >> > + if (rp - (u32 __user *)datapos < 2) >> > + continue; >> >> 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. 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; } .... 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. > The whole GOT (.got.plt + .got) will be more than two words anyway, if > there is a GOT (i.e. if flag FLAT_FLAG_GOTPIC is set in the bFLT binary), > so the "end of GOT"/LONG(-1) will always come way after these first two > entries anyway. > > Another reason why I don't fancy a 64-bit and 32-bit version is because > some architectures might be 64-bit, but I assume that they can be running > a 32-bit userland. (And in comparison with the ELF header that tells if > the binary is 32-bit or 64-bit, I don't see something similar in the bFLT > header.) 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. 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. Does elf2flt really change all of these values to big-endian on little-endian platforms? Eric
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(-)