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

[Kees Cook <keescook@chromium.org>]

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

-- 
Kees Cook