| Message ID | 20220414091018.896737-1-niklas.cassel@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] binfmt_flat: do not stop relocating GOT entries prematurely on riscv | expand |
On Thu, Apr 14, 2022 at 11:10:18AM +0200, 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 > (which has size 16 bytes on elf64-riscv and 8 bytes on elf32-riscv), where > the first word is set to -1. See the binutils implementation for riscv [1]. > > This causes the binfmt_flat loader to stop relocating GOT entries > prematurely and thus causes the application to crash when running. > > Fix this by skipping the whole GOTPLT header, since the whole GOTPLT header > is reserved for the dynamic linker. > > The GOTPLT header will only be skipped for bFLT binaries with flag > FLAT_FLAG_GOTPIC set. This flag is unconditionally set by elf2flt if the > supplied ELF binary has the symbol _GLOBAL_OFFSET_TABLE_ defined. > ELF binaries without a .got input section should thus 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 > > Cc: <stable@vger.kernel.org> > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com> > --- > Changes since v1: > -Incorporated review comments from Eric Biederman. Thanks! nit: please include a link to the archive so it's easier for people (and b4) to track earlier versions. i.e.: https://lore.kernel.org/linux-mm/20220412100338.437308-1-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 | 27 ++++++++++++++++++++++++++- > 1 file changed, 26 insertions(+), 1 deletion(-) > > diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c > index 626898150011..e5e2a03b39c1 100644 > --- a/fs/binfmt_flat.c > +++ b/fs/binfmt_flat.c > @@ -440,6 +440,30 @@ static void old_reloc(unsigned long rl) > > /****************************************************************************/ > > +static inline u32 __user *skip_got_header(u32 __user *rp) > +{ > + if (IS_ENABLED(CONFIG_RISCV)) { > + /* > + * RISC-V 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; Just so I understand; due to the FILL(0) and the ALIGN, val1 will be 0 (or more specifically, not -1) in all other cases, yes? I probably would have written this as: if (rp_val0 == 0xffffffff) rp += 2; if (rp_val1 == 0xffffffff) rp += 2; But no need to change it. I expect the compiler would optimize it in the same thing anyway. :) > + } > + return rp; > +} > + > static int load_flat_file(struct linux_binprm *bprm, > struct lib_info *libinfo, int id, unsigned long *extra_stack) > { > @@ -789,7 +813,8 @@ static int load_flat_file(struct linux_binprm *bprm, > * image. > */ > if (flags & FLAT_FLAG_GOTPIC) { > - for (rp = (u32 __user *)datapos; ; rp++) { > + rp = skip_got_header((u32 * __user) datapos); > + for (; ; rp++) { > u32 addr, rp_val; > if (get_user(rp_val, rp)) > return -EFAULT; > -- > 2.35.1 > Eric and Damien, does this look good to you? I'll take this into the execve tree unless you'd like to see further changes. Thanks! -Kees
On Thu, Apr 14, 2022 at 11:10:18AM +0200, Niklas Cassel wrote: > bFLT binaries are usually created using elf2flt. > [...] Hm, something in the chain broke DKIM, but I can't see what: ✗ [PATCH v2] binfmt_flat: do not stop relocating GOT entries prematurely on riscv ✗ BADSIG: DKIM/wdc.com Konstantin, do you have a process for debugging these? I don't see the "normal" stuff that breaks DKIM (i.e. a trailing mailing list footer, etc)
On 4/15/22 08:27, Kees Cook wrote: > On Thu, Apr 14, 2022 at 11:10:18AM +0200, Niklas Cassel wrote: >> bFLT binaries are usually created using elf2flt. >> [...] > > Hm, something in the chain broke DKIM, but I can't see what: > > ✗ [PATCH v2] binfmt_flat: do not stop relocating GOT entries prematurely on riscv > ✗ BADSIG: DKIM/wdc.com Hu... WDC emails are not spams :) No clue what is going on here. I can check with our IT if they changed something though. > > Konstantin, do you have a process for debugging these? I don't see the > "normal" stuff that breaks DKIM (i.e. a trailing mailing list footer, etc) >
On 4/14/22 18:10, 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 > (which has size 16 bytes on elf64-riscv and 8 bytes on elf32-riscv), where > the first word is set to -1. See the binutils implementation for riscv [1]. > > This causes the binfmt_flat loader to stop relocating GOT entries > prematurely and thus causes the application to crash when running. > > Fix this by skipping the whole GOTPLT header, since the whole GOTPLT header > is reserved for the dynamic linker. > > The GOTPLT header will only be skipped for bFLT binaries with flag > FLAT_FLAG_GOTPIC set. This flag is unconditionally set by elf2flt if the > supplied ELF binary has the symbol _GLOBAL_OFFSET_TABLE_ defined. > ELF binaries without a .got input section should thus 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 > > Cc: <stable@vger.kernel.org> > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com> > --- > Changes since v1: > -Incorporated review comments from Eric Biederman. > > 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 | 27 ++++++++++++++++++++++++++- > 1 file changed, 26 insertions(+), 1 deletion(-) > > diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c > index 626898150011..e5e2a03b39c1 100644 > --- a/fs/binfmt_flat.c > +++ b/fs/binfmt_flat.c > @@ -440,6 +440,30 @@ static void old_reloc(unsigned long rl) > > /****************************************************************************/ > > +static inline u32 __user *skip_got_header(u32 __user *rp) > +{ > + if (IS_ENABLED(CONFIG_RISCV)) { > + /* > + * RISC-V 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; This looks good to me. But thinking more about it, do we really need to check what the content of the header is ? Why not simply replace this entire hunk with: return rp + sizeof(unsigned long) * 2; to ignore the 16B (or 8B for 32-bits arch) header regardless of what the header word values are ? Are there any case where the header is *not* present ? > + } > + return rp; > +} > + > static int load_flat_file(struct linux_binprm *bprm, > struct lib_info *libinfo, int id, unsigned long *extra_stack) > { > @@ -789,7 +813,8 @@ static int load_flat_file(struct linux_binprm *bprm, > * image. > */ > if (flags & FLAT_FLAG_GOTPIC) { > - for (rp = (u32 __user *)datapos; ; rp++) { > + rp = skip_got_header((u32 * __user) datapos); > + for (; ; rp++) { > u32 addr, rp_val; > if (get_user(rp_val, rp)) > return -EFAULT; Regardless of the above nit, feel free to add: Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
On Fri, Apr 15, 2022 at 08:51:27AM +0900, Damien Le Moal wrote: > On 4/14/22 18:10, Niklas Cassel wrote: (snip) > This looks good to me. But thinking more about it, do we really need to > check what the content of the header is ? Why not simply replace this > entire hunk with: > > return rp + sizeof(unsigned long) * 2; > > to ignore the 16B (or 8B for 32-bits arch) header regardless of what the > header word values are ? Are there any case where the header is *not* > present ? Considering that I haven't been able to find any real specification that describes the bFLT format. (No, the elf2flt source is no specification.) This whole format seems kind of fragile. I realize that checking the first one or two entries after data start is not the most robust thing, but I still prefer it over skipping blindly. Especially considering that only m68k seems to support shared libraries with bFLT. So even while this header is reserved for ld.so, it will most likely only be used on m68k bFLT binaries.. so perhaps elf2flt some day decides to strip away this header on all bFLT binaries except for m68k? bFLT seems to currently be at version 4, perhaps such a change would require a version bump.. Or not? (Now, if there only was a spec.. :P) Kind regards, Niklas > > > + } > > + return rp; > > +} > > + > > static int load_flat_file(struct linux_binprm *bprm, > > struct lib_info *libinfo, int id, unsigned long *extra_stack) > > { > > @@ -789,7 +813,8 @@ static int load_flat_file(struct linux_binprm *bprm, > > * image. > > */ > > if (flags & FLAT_FLAG_GOTPIC) { > > - for (rp = (u32 __user *)datapos; ; rp++) { > > + rp = skip_got_header((u32 * __user) datapos); > > + for (; ; rp++) { > > u32 addr, rp_val; > > if (get_user(rp_val, rp)) > > return -EFAULT; > > Regardless of the above nit, feel free to add: > > Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> > > > -- > Damien Le Moal > Western Digital Research
On 4/15/22 09:30, Niklas Cassel wrote: > On Fri, Apr 15, 2022 at 08:51:27AM +0900, Damien Le Moal wrote: >> On 4/14/22 18:10, Niklas Cassel wrote: > > (snip) > >> This looks good to me. But thinking more about it, do we really need to >> check what the content of the header is ? Why not simply replace this >> entire hunk with: >> >> return rp + sizeof(unsigned long) * 2; >> >> to ignore the 16B (or 8B for 32-bits arch) header regardless of what the >> header word values are ? Are there any case where the header is *not* >> present ? > > Considering that I haven't been able to find any real specification that > describes the bFLT format. (No, the elf2flt source is no specification.) > This whole format seems kind of fragile. > > I realize that checking the first one or two entries after data start is > not the most robust thing, but I still prefer it over skipping blindly. > > Especially considering that only m68k seems to support shared libraries > with bFLT. So even while this header is reserved for ld.so, it will most > likely only be used on m68k bFLT binaries.. so perhaps elf2flt some day > decides to strip away this header on all bFLT binaries except for m68k? > > bFLT seems to currently be at version 4, perhaps such a change would > require a version bump.. Or not? (Now, if there only was a spec.. :P) The header skip is only for riscv since you have that "if (IS_ENABLED(CONFIG_RISCV)) {". So whatever you do under that if will not affect other architectures. The patch will be a nop for them. So if we are sure that we can just skip the first 16B/8B for riscv, I would not bother checking the header content. But as mentioned, the current code is fine too. Both approaches are fine with me but I prefer the simpler one :) > > > Kind regards, > Niklas > >> >>> + } >>> + return rp; >>> +} >>> + >>> static int load_flat_file(struct linux_binprm *bprm, >>> struct lib_info *libinfo, int id, unsigned long *extra_stack) >>> { >>> @@ -789,7 +813,8 @@ static int load_flat_file(struct linux_binprm *bprm, >>> * image. >>> */ >>> if (flags & FLAT_FLAG_GOTPIC) { >>> - for (rp = (u32 __user *)datapos; ; rp++) { >>> + rp = skip_got_header((u32 * __user) datapos); >>> + for (; ; rp++) { >>> u32 addr, rp_val; >>> if (get_user(rp_val, rp)) >>> return -EFAULT; >> >> Regardless of the above nit, feel free to add: >> >> Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> >> >> >> -- >> Damien Le Moal >> Western Digital Research
On Fri, Apr 15, 2022 at 09:56:38AM +0900, Damien Le Moal wrote: > On 4/15/22 09:30, Niklas Cassel wrote: > > On Fri, Apr 15, 2022 at 08:51:27AM +0900, Damien Le Moal wrote: > >> On 4/14/22 18:10, Niklas Cassel wrote: (snip) > So if we are sure that we can just skip the first 16B/8B for riscv, I > would not bother checking the header content. But as mentioned, the > current code is fine too. That was my point, I'm not sure that we can be sure that we can always skip it in the future. E.g. if the elf2flt linker script decides to swap the order of .got and .got.plt for some random reason in the future, we would skip data that really should have been relocated. So I think that it is better to keep it, even if it is a bit verbose. Kind regards, Niklas
On 4/15/22 10:08, Niklas Cassel wrote: > On Fri, Apr 15, 2022 at 09:56:38AM +0900, Damien Le Moal wrote: >> On 4/15/22 09:30, Niklas Cassel wrote: >>> On Fri, Apr 15, 2022 at 08:51:27AM +0900, Damien Le Moal wrote: >>>> On 4/14/22 18:10, Niklas Cassel wrote: > > (snip) > >> So if we are sure that we can just skip the first 16B/8B for riscv, I >> would not bother checking the header content. But as mentioned, the >> current code is fine too. > > That was my point, I'm not sure that we can be sure that we can always > skip it in the future. E.g. if the elf2flt linker script decides to swap > the order of .got and .got.plt for some random reason in the future, > we would skip data that really should have been relocated. Good point. Your current patch is indeed better then. BUT that would also mean that the skip header function needs to be called inside the loop then, no ? If the section orders are reversed, we would still need to skip that header in the middle of the relocation loop... > > So I think that it is better to keep it, even if it is a bit verbose. > > > Kind regards, > Niklas
On Thu, Apr 14, 2022 at 04:05:54PM -0700, Kees Cook wrote: > On Thu, Apr 14, 2022 at 11:10:18AM +0200, Niklas Cassel wrote: (snip) > > +static inline u32 __user *skip_got_header(u32 __user *rp) > > +{ > > + if (IS_ENABLED(CONFIG_RISCV)) { > > + /* > > + * RISC-V 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; > > Just so I understand; due to the FILL(0) and the ALIGN, val1 will be 0 > (or more specifically, not -1) in all other cases, yes? For elf64-riscv with a .got.plt header: rp+0: -1, rp+1: -1, rp+2: 0, rp+3: 0 For elf32-riscv with a .got.plt header: rp+0: -1, rp+1: 0 At least riscv binutils 2.32, 2.37 and 2.38 all create a .got.plt header even when there are no .got.plt entries following the header. Even if the .got.plt section was empty, there will still be data in the .got section, so rp+0 will still not be -1. If there is no data in the .got section, then the _GLOBAL_OFFSET_TABLE_ symbol will not be defined, so elf2flt will not set the FLAT_FLAG_GOTPIC flag. (This code is only executed if that flag is set.) Kind regards, Niklas
On Thu, Apr 14, 2022 at 04:27:38PM -0700, Kees Cook wrote: > On Thu, Apr 14, 2022 at 11:10:18AM +0200, Niklas Cassel wrote: > > bFLT binaries are usually created using elf2flt. > > [...] > > Hm, something in the chain broke DKIM, but I can't see what: > > ✗ [PATCH v2] binfmt_flat: do not stop relocating GOT entries prematurely on riscv > ✗ BADSIG: DKIM/wdc.com > > Konstantin, do you have a process for debugging these? I don't see the > "normal" stuff that breaks DKIM (i.e. a trailing mailing list footer, etc) It's our usual friend "c=simple/simple" -- vger just doesn't work with that. See here for reasons: https://lore.kernel.org/lkml/20211214150032.nioelgvmase7yyus@meerkat.local/ You should try to convince wdc.com mail admins to set it to c=relaxed/simple, or you can cc all your patches to patches@lists.linux.dev (which does work with c=simple/simple), and then b4 will give those priority treatment. -K
On Fri, Apr 15, 2022 at 10:13:31AM +0900, Damien Le Moal wrote: > On 4/15/22 10:08, Niklas Cassel wrote: > > On Fri, Apr 15, 2022 at 09:56:38AM +0900, Damien Le Moal wrote: > >> On 4/15/22 09:30, Niklas Cassel wrote: > >>> On Fri, Apr 15, 2022 at 08:51:27AM +0900, Damien Le Moal wrote: > >>>> On 4/14/22 18:10, Niklas Cassel wrote: > > > > (snip) > > > >> So if we are sure that we can just skip the first 16B/8B for riscv, I > >> would not bother checking the header content. But as mentioned, the > >> current code is fine too. > > > > That was my point, I'm not sure that we can be sure that we can always > > skip it in the future. E.g. if the elf2flt linker script decides to swap > > the order of .got and .got.plt for some random reason in the future, > > we would skip data that really should have been relocated. > > Good point. Your current patch is indeed better then. BUT that would also > mean that the skip header function needs to be called inside the loop > then, no ? If the section orders are reversed, we would still need to skip > that header in the middle of the relocation loop... So this is theoretical, but if the sections were swapped in the linker script, and we have the patch in $subject applied, we will not skip data that needs to be relocated. But after relocating all the entries in the .got section we will still break too early, if we actually had any .got.plt entries after the .got.plt header. The .got.plt entries would not get relocated. However, the elf2flt maintainer explicitly asked ut to fix the kernel or binutils, so that they can continue using the exact same linker script that it has been using forever. (And we shouldn't need to change binutils just for the bFLT format.) So the chance that the linker script changes in practice is really small. (This .got.plt vs .got hasn't changed in 19 years.) But if it does, we will just have one problem instead of two :) However, I think that applying this patch is sufficient for now, since it makes the code work with the existing elf2flt linker script. Adapting the code to also handle this theoretical layout of the linker script would just complicate things even more. I'm not even sure if we would be able to handle this case, since the information about the .got and .got.plt section sizes is lost once the ELF has been converted to bFLT. Kind regards, Niklas
On 4/15/22 11:11, Niklas Cassel wrote: > On Fri, Apr 15, 2022 at 10:13:31AM +0900, Damien Le Moal wrote: >> On 4/15/22 10:08, Niklas Cassel wrote: >>> On Fri, Apr 15, 2022 at 09:56:38AM +0900, Damien Le Moal wrote: >>>> On 4/15/22 09:30, Niklas Cassel wrote: >>>>> On Fri, Apr 15, 2022 at 08:51:27AM +0900, Damien Le Moal wrote: >>>>>> On 4/14/22 18:10, Niklas Cassel wrote: >>> >>> (snip) >>> >>>> So if we are sure that we can just skip the first 16B/8B for riscv, I >>>> would not bother checking the header content. But as mentioned, the >>>> current code is fine too. >>> >>> That was my point, I'm not sure that we can be sure that we can always >>> skip it in the future. E.g. if the elf2flt linker script decides to swap >>> the order of .got and .got.plt for some random reason in the future, >>> we would skip data that really should have been relocated. >> >> Good point. Your current patch is indeed better then. BUT that would also >> mean that the skip header function needs to be called inside the loop >> then, no ? If the section orders are reversed, we would still need to skip >> that header in the middle of the relocation loop... > > So this is theoretical, but if the sections were swapped in the linker > script, and we have the patch in $subject applied, we will not skip data > that needs to be relocated. But after relocating all the entries in the > .got section we will still break too early, if we actually had any > .got.plt entries after the .got.plt header. The .got.plt entries would > not get relocated. > > However, the elf2flt maintainer explicitly asked ut to fix the kernel or > binutils, so that they can continue using the exact same linker script > that it has been using forever. (And we shouldn't need to change binutils > just for the bFLT format.) > > So the chance that the linker script changes in practice is really small. > (This .got.plt vs .got hasn't changed in 19 years.) > > But if it does, we will just have one problem instead of two :) > However, I think that applying this patch is sufficient for now, > since it makes the code work with the existing elf2flt linker script. > > Adapting the code to also handle this theoretical layout of the linker > script would just complicate things even more. I'm not even sure if we > would be able to handle this case, since the information about the .got > and .got.plt section sizes is lost once the ELF has been converted to > bFLT. OK. All good then. I maintain my reviewed-by tag :)
On Thu, Apr 14, 2022 at 09:26:10PM -0400, Konstantin Ryabitsev wrote: > On Thu, Apr 14, 2022 at 04:27:38PM -0700, Kees Cook wrote: > > On Thu, Apr 14, 2022 at 11:10:18AM +0200, Niklas Cassel wrote: > > > bFLT binaries are usually created using elf2flt. > > > [...] > > > > Hm, something in the chain broke DKIM, but I can't see what: > > > > ✗ [PATCH v2] binfmt_flat: do not stop relocating GOT entries prematurely on riscv > > ✗ BADSIG: DKIM/wdc.com > > > > Konstantin, do you have a process for debugging these? I don't see the > > "normal" stuff that breaks DKIM (i.e. a trailing mailing list footer, etc) > > It's our usual friend "c=simple/simple" -- vger just doesn't work with that. > See here for reasons: > > https://lore.kernel.org/lkml/20211214150032.nioelgvmase7yyus@meerkat.local/ > > You should try to convince wdc.com mail admins to set it to c=relaxed/simple, > or you can cc all your patches to patches@lists.linux.dev (which does work > with c=simple/simple), and then b4 will give those priority treatment. Ah-ha! Thank you! :)
On Thu, 14 Apr 2022 11:10:18 +0200, 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) ; > ... > } > > [...] Applied to for-next/execve, thanks! [1/1] binfmt_flat: do not stop relocating GOT entries prematurely on riscv https://git.kernel.org/kees/c/a767e6fd68d2
On 15/4/22 10:30, Niklas Cassel wrote: > On Fri, Apr 15, 2022 at 08:51:27AM +0900, Damien Le Moal wrote: >> On 4/14/22 18:10, Niklas Cassel wrote: > > (snip) > >> This looks good to me. But thinking more about it, do we really need to >> check what the content of the header is ? Why not simply replace this >> entire hunk with: >> >> return rp + sizeof(unsigned long) * 2; >> >> to ignore the 16B (or 8B for 32-bits arch) header regardless of what the >> header word values are ? Are there any case where the header is *not* >> present ? > > Considering that I haven't been able to find any real specification that > describes the bFLT format. (No, the elf2flt source is no specification.) > This whole format seems kind of fragile. > > I realize that checking the first one or two entries after data start is > not the most robust thing, but I still prefer it over skipping blindly. > > Especially considering that only m68k seems to support shared libraries > with bFLT. So even while this header is reserved for ld.so, it will most > likely only be used on m68k bFLT binaries.. so perhaps elf2flt some day > decides to strip away this header on all bFLT binaries except for m68k? FWIW there has been talk for a couple of years now to remove the shared library support for m68k. It doesn't get used - probably not for a very long time now. And most likely doesn't even work anymore. Regards Greg > bFLT seems to currently be at version 4, perhaps such a change would > require a version bump.. Or not? (Now, if there only was a spec.. :P) > > > Kind regards, > Niklas > >> >>> + } >>> + return rp; >>> +} >>> + >>> static int load_flat_file(struct linux_binprm *bprm, >>> struct lib_info *libinfo, int id, unsigned long *extra_stack) >>> { >>> @@ -789,7 +813,8 @@ static int load_flat_file(struct linux_binprm *bprm, >>> * image. >>> */ >>> if (flags & FLAT_FLAG_GOTPIC) { >>> - for (rp = (u32 __user *)datapos; ; rp++) { >>> + rp = skip_got_header((u32 * __user) datapos); >>> + for (; ; rp++) { >>> u32 addr, rp_val; >>> if (get_user(rp_val, rp)) >>> return -EFAULT; >> >> Regardless of the above nit, feel free to add: >> >> Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> >> >> >> -- >> Damien Le Moal >> Western Digital Research
diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c index 626898150011..e5e2a03b39c1 100644 --- a/fs/binfmt_flat.c +++ b/fs/binfmt_flat.c @@ -440,6 +440,30 @@ static void old_reloc(unsigned long rl) /****************************************************************************/ +static inline u32 __user *skip_got_header(u32 __user *rp) +{ + if (IS_ENABLED(CONFIG_RISCV)) { + /* + * RISC-V 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; +} + static int load_flat_file(struct linux_binprm *bprm, struct lib_info *libinfo, int id, unsigned long *extra_stack) { @@ -789,7 +813,8 @@ static int load_flat_file(struct linux_binprm *bprm, * image. */ if (flags & FLAT_FLAG_GOTPIC) { - for (rp = (u32 __user *)datapos; ; rp++) { + rp = skip_got_header((u32 * __user) datapos); + for (; ; rp++) { u32 addr, rp_val; if (get_user(rp_val, rp)) return -EFAULT;
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 (which has size 16 bytes on elf64-riscv and 8 bytes on elf32-riscv), where the first word is set to -1. See the binutils implementation for riscv [1]. This causes the binfmt_flat loader to stop relocating GOT entries prematurely and thus causes the application to crash when running. Fix this by skipping the whole GOTPLT header, since the whole GOTPLT header is reserved for the dynamic linker. The GOTPLT header will only be skipped for bFLT binaries with flag FLAT_FLAG_GOTPIC set. This flag is unconditionally set by elf2flt if the supplied ELF binary has the symbol _GLOBAL_OFFSET_TABLE_ defined. ELF binaries without a .got input section should thus 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 Cc: <stable@vger.kernel.org> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com> --- Changes since v1: -Incorporated review comments from Eric Biederman. 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 | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-)