linux-kernel - Re: [PATCH] binfmt_elf: fix return value in case of interpreter load failure

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20130411150401.8bf008e05f5dd2239277e2c1@linux-foundation.org>
Date:	Thu, 11 Apr 2013 15:04:01 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Matthieu CASTET <matthieu.castet@...rot.com>
Cc:	linux-kernel@...r.kernel.org, Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH] binfmt_elf: fix return value in case of interpreter
 load failure

On Thu, 11 Apr 2013 15:53:09 +0200 Matthieu CASTET <matthieu.castet@...rot.com> wrote:

> The current code return the address instead of using PTR_ERR.

I don't understand what you mean here - please describe this error in
much more detail.  Help people to identify the section of code which
is being discussed.

> Also the check is done after adding e_entry. This can cause weird behaviour
> because -errno + loc->interp_elf_ex.e_entry can produce a valid address.

Which check?

> Add a check to test load error before adding entry address. Also in this
> case send SIGKILL instead of SIGSEGV to match what is done when loading binary.
> 
> ...
>
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -900,18 +900,21 @@ static int load_elf_binary(struct linux_binprm *bprm)
>  					    interpreter,
>  					    &interp_map_addr,
>  					    load_bias);
> -		if (!IS_ERR((void *)elf_entry)) {
> -			/*
> -			 * load_elf_interp() returns relocation
> -			 * adjustment
> -			 */
> -			interp_load_addr = elf_entry;
> -			elf_entry += loc->interp_elf_ex.e_entry;
> +		if (BAD_ADDR(elf_entry)) {
> +			force_sig(SIGKILL, current);
> +			retval = IS_ERR((void *)elf_entry) ?
> +					PTR_ERR((void *)elf_entry) : -EINVAL;

Thats's a bit verbose - "PTR_ERR((void *)elf_entry)" is equivalent to
"elf_entry".  I suppose we can do it this way to document the intent or
something.

It would be helpful if load_elf_interp() had some documentation
describing its return value btw.


> +			goto out_free_dentry;
>  		}
> +		/*
> +		 * load_elf_interp() returns relocation
> +		 * adjustment

This can now be converted to a single-line comment.

> +		 */
> +		interp_load_addr = elf_entry;
> +		elf_entry += loc->interp_elf_ex.e_entry;
>  		if (BAD_ADDR(elf_entry)) {
>  			force_sig(SIGSEGV, current);
> -			retval = IS_ERR((void *)elf_entry) ?
> -					(int)elf_entry : -EINVAL;
> +			retval = -EINVAL;
>  			goto out_free_dentry;
>  		}
>  		reloc_func_desc = interp_load_addr;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/