Re: [PATCH v10 1/2] elf: Allow architectures to parse properties on the main executable

[Kees Cook <keescook@chromium.org>]

On Mon, Feb 28, 2022 at 01:06:05PM +0000, Mark Brown wrote:
> Currently the ELF code only attempts to parse properties on the image
> that will start execution, either the interpreter or for statically linked
> executables the main executable. The expectation is that any property
> handling for the main executable will be done by the interpreter. This is
> a bit inconsistent since we do map the executable and is causing problems
> for the arm64 BTI support when used in conjunction with systemd's use of
> seccomp to implement MemoryDenyWriteExecute which stops the dynamic linker
> adjusting the permissions of executable segments.
> 
> Allow architectures to handle properties for both the dynamic linker and
> main executable, adjusting arch_parse_elf_properties() to have a new
> flag is_interp flag as with arch_elf_adjust_prot() and calling it for
> both the main executable and any intepreter.
> 
> The user of this code, arm64, is adapted to ensure that there is no
> functional change.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> Tested-by: Jeremy Linton <jeremy.linton@arm.com>
> Reviewed-by: Dave Martin <Dave.Martin@arm.com>
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> ---
>  arch/arm64/include/asm/elf.h |  3 ++-
>  fs/binfmt_elf.c              | 32 +++++++++++++++++++++++---------
>  include/linux/elf.h          |  4 +++-
>  3 files changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h
> index 97932fbf973d..5cc002376abe 100644
> --- a/arch/arm64/include/asm/elf.h
> +++ b/arch/arm64/include/asm/elf.h
> @@ -259,6 +259,7 @@ struct arch_elf_state {
>  
>  static inline int arch_parse_elf_property(u32 type, const void *data,
>  					  size_t datasz, bool compat,
> +					  bool has_interp, bool is_interp,
>  					  struct arch_elf_state *arch)

Adding more and more args to a functions like this gives me the sense
that some kind of argument structure is needed.

Once I get enough unit testing written in here, I'm hoping to refactor
a bunch of this. To the future! :)

> @@ -828,6 +832,7 @@ static int load_elf_binary(struct linux_binprm *bprm)
>  	unsigned long error;
>  	struct elf_phdr *elf_ppnt, *elf_phdata, *interp_elf_phdata = NULL;
>  	struct elf_phdr *elf_property_phdata = NULL;
> +	struct elf_phdr *interp_elf_property_phdata = NULL;
>  	unsigned long elf_bss, elf_brk;
>  	int bss_prot = 0;
>  	int retval, i;
> @@ -865,6 +870,9 @@ static int load_elf_binary(struct linux_binprm *bprm)
>  	for (i = 0; i < elf_ex->e_phnum; i++, elf_ppnt++) {
>  		char *elf_interpreter;
>  
> +		if (interpreter && elf_property_phdata)
> +			break;
> +

This is not okay. This introduces a memory resource leak for malicious
ELF files with multiple INTERP headers.

>  		if (elf_ppnt->p_type == PT_GNU_PROPERTY) {
>  			elf_property_phdata = elf_ppnt;
>  			continue;
> @@ -919,7 +927,7 @@ static int load_elf_binary(struct linux_binprm *bprm)
>  		if (retval < 0)
>  			goto out_free_dentry;
>  
> -		break;
> +		continue;

Because of this.

As a fix, I'd expect the PT_INTERP test to be updated:

                if (interpreter || elf_ppnt->p_type != PT_INTERP)
                        continue;


>  
>  out_free_interp:
>  		kfree(elf_interpreter);
> @@ -963,12 +971,11 @@ static int load_elf_binary(struct linux_binprm *bprm)
>  			goto out_free_dentry;
>  
>  		/* Pass PT_LOPROC..PT_HIPROC headers to arch code */
> -		elf_property_phdata = NULL;
>  		elf_ppnt = interp_elf_phdata;
>  		for (i = 0; i < interp_elf_ex->e_phnum; i++, elf_ppnt++)
>  			switch (elf_ppnt->p_type) {
>  			case PT_GNU_PROPERTY:
> -				elf_property_phdata = elf_ppnt;
> +				interp_elf_property_phdata = elf_ppnt;
>  				break;
>  
>  			case PT_LOPROC ... PT_HIPROC:
> @@ -979,10 +986,17 @@ static int load_elf_binary(struct linux_binprm *bprm)
>  					goto out_free_dentry;
>  				break;
>  			}
> +
> +		retval = parse_elf_properties(interpreter,
> +					      interp_elf_property_phdata,
> +					      true, true, &arch_state);
> +		if (retval)
> +			goto out_free_dentry;
> +
>  	}
>  
> -	retval = parse_elf_properties(interpreter ?: bprm->file,
> -				      elf_property_phdata, &arch_state);
> +	retval = parse_elf_properties(bprm->file, elf_property_phdata,
> +				      interpreter, false, &arch_state);
>  	if (retval)
>  		goto out_free_dentry;
>  
> diff --git a/include/linux/elf.h b/include/linux/elf.h
> index c9a46c4e183b..1c45ecf29147 100644
> --- a/include/linux/elf.h
> +++ b/include/linux/elf.h
> @@ -88,13 +88,15 @@ struct arch_elf_state;
>  #ifndef CONFIG_ARCH_USE_GNU_PROPERTY
>  static inline int arch_parse_elf_property(u32 type, const void *data,
>  					  size_t datasz, bool compat,
> +					  bool has_interp, bool is_interp,
>  					  struct arch_elf_state *arch)
>  {
>  	return 0;
>  }
>  #else
>  extern int arch_parse_elf_property(u32 type, const void *data, size_t datasz,
> -				   bool compat, struct arch_elf_state *arch);
> +				   bool compat, bool has_interp, bool is_interp,
> +				   struct arch_elf_state *arch);
>  #endif
>  
>  #ifdef CONFIG_ARCH_HAVE_ELF_PROT
> -- 
> 2.30.2
> 

-- 
Kees Cook