[PATCH] binfmt_elf: fix return value in case of interpreter load failure

[PATCH] binfmt_elf: fix return value in case of interpreter load failure

[Matthieu CASTET]

The current code return the address instead of using PTR_ERR.

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.

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.

Signed-off-by: Matthieu CASTET <matthieu.castet@parrot.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 fs/binfmt_elf.c |   21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 3939829..8397f80 100644
--- 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;
+			goto out_free_dentry;
 		}
+		/*
+		 * 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(SIGSEGV, current);
-			retval = IS_ERR((void *)elf_entry) ?
-					(int)elf_entry : -EINVAL;
+			retval = -EINVAL;
 			goto out_free_dentry;
 		}
 		reloc_func_desc = interp_load_addr;
-- 
1.7.10.4

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

[Andrew Morton]

On Thu, 11 Apr 2013 15:53:09 +0200 Matthieu CASTET <matthieu.castet@parrot.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;

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

[Matthieu CASTET]

Hi Andrew,

thanks for your quick review.

Andrew Morton a écrit :
> On Thu, 11 Apr 2013 15:53:09 +0200 Matthieu CASTET <matthieu.castet@parrot.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.

I was speaking of


 elf_entry = load_elf_interp(&loc->interp_elf_ex,
                        interpreter,
                        &interp_map_addr,
                        load_bias);
[...]
        if (BAD_ADDR(elf_entry)) {
            force_sig(SIGSEGV, current);
            retval = IS_ERR((void *)elf_entry) ?
                    (int)elf_entry : -EINVAL;
            goto out_free_dentry;
        }

and was expecting we should use PTR_ERR when IS_ERR is true to match what is
done in [1].

But didn't saw that PTR_ERR((void *)elf_entry) and (int)elf_entry are equivalent.

> 
>> 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?

I am really confused here. Reading again the code this can't happen because if
load_elf_interp return -errno


We don't enter this condition
>         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;
>         }
we still have -errno here
>         if (BAD_ADDR(elf_entry)) {
>             force_sig(SIGSEGV, current);
>             retval = IS_ERR((void *)elf_entry) ?
>                     (int)elf_entry : -EINVAL;
>             goto out_free_dentry;
>         }


Sorry for my mistake.

The only valid remaining part of my patch is to return SIGKILL when
load_elf_interp fail (IS_ERR((void *)elf_entry) is true) (for example load
address of linker is bad) instead of SIGSEGV. This will follow what is done when
loading binary.

But is it even worth doing?


> 
>> 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.
Ok, I see.
Note that [1] use PTR_ERR but elf_map already return unsigned long like
load_elf_interp.



Matthieu



[1]
        error = elf_map(bprm->file, load_bias + vaddr, elf_ppnt,
                elf_prot, elf_flags, 0);
        if (BAD_ADDR(error)) {
            send_sig(SIGKILL, current, 0);
            retval = IS_ERR((void *)error) ?
                PTR_ERR((void*)error) : -EINVAL;
            goto out_free_dentry;
        }

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

[Andrew Morton]

On Fri, 12 Apr 2013 16:49:50 +0200 Matthieu CASTET <matthieu.castet@parrot.com> wrote:

> Hi Andrew,
> 
> thanks for your quick review.
> 
> Andrew Morton a __crit :
> > On Thu, 11 Apr 2013 15:53:09 +0200 Matthieu CASTET <matthieu.castet@parrot.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.
> 
> I was speaking of
> 
> 
>  elf_entry = load_elf_interp(&loc->interp_elf_ex,
>                         interpreter,
>                         &interp_map_addr,
>                         load_bias);
> [...]
>         if (BAD_ADDR(elf_entry)) {
>             force_sig(SIGSEGV, current);
>             retval = IS_ERR((void *)elf_entry) ?
>                     (int)elf_entry : -EINVAL;
>             goto out_free_dentry;
>         }
> 
> and was expecting we should use PTR_ERR when IS_ERR is true to match what is
> done in [1].
> 
> But didn't saw that PTR_ERR((void *)elf_entry) and (int)elf_entry are equivalent.
> 
> > 
> >> 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?
> 
> I am really confused here. Reading again the code this can't happen because if
> load_elf_interp return -errno
> 
> 
> We don't enter this condition
> >         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;
> >         }
> we still have -errno here
> >         if (BAD_ADDR(elf_entry)) {
> >             force_sig(SIGSEGV, current);
> >             retval = IS_ERR((void *)elf_entry) ?
> >                     (int)elf_entry : -EINVAL;
> >             goto out_free_dentry;
> >         }
> 
> 
> Sorry for my mistake.
> 
> The only valid remaining part of my patch is to return SIGKILL when
> load_elf_interp fail (IS_ERR((void *)elf_entry) is true) (for example load
> address of linker is bad) instead of SIGSEGV. This will follow what is done when
> loading binary.
> 
> But is it even worth doing?

SIGSEGV can be caught so that would be a user-visible change.  I just
don't know what the implications of such a change would be :(

(hopefully cc's Oleg)

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

[Oleg Nesterov]

On 04/15, Andrew Morton wrote:
>
> On Fri, 12 Apr 2013 16:49:50 +0200 Matthieu CASTET <matthieu.castet@parrot.com> wrote:
>
> > The only valid remaining part of my patch is to return SIGKILL when
> > load_elf_interp fail (IS_ERR((void *)elf_entry) is true) (for example load
> > address of linker is bad) instead of SIGSEGV. This will follow what is done when
> > loading binary.
> >
> > But is it even worth doing?
>
> SIGSEGV can be caught

Actually it can't be, flush_signal_handlers() was already called.
SIGSEGV can be blocked/ignored after that, but please note that
force_sig_info(SIGSEGV) will unblock and set SIG_DFL if necessary.

In short, force_sig() will actuallu kill the task in any case.

But: afaics send_sig(SIGSEGV) above load_elf_interp() is wrong,
we should either use SIGKILL (which can't be ignored/blocked) or
force_sig.

> that would be a user-visible change.

Yes. waitpid(&status) can notice the difference.

> I just
> don't know what the implications of such a change would be :(

Mee too... Looks harmless but still.

OTOH, I do not know why/when we should use SIGKILL or SIGSEGV in
this code.

Oleg.