Re: [PATCH] binfmt_elf: fix return value in case of interpreter load failure
On 04/15, Andrew Morton wrote: > > On Fri, 12 Apr 2013 16:49:50 +0200 Matthieu CASTET > 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() 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] binfmt_elf: fix return value in case of interpreter load failure
On 04/15, Andrew Morton wrote: On Fri, 12 Apr 2013 16:49:50 +0200 Matthieu CASTET matthieu.cas...@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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] binfmt_elf: fix return value in case of interpreter load failure
On Fri, 12 Apr 2013 16:49:50 +0200 Matthieu CASTET
wrote:
> Hi Andrew,
>
> thanks for your quick review.
>
> Andrew Morton a __crit :
> > On Thu, 11 Apr 2013 15:53:09 +0200 Matthieu CASTET
> > 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(>interp_elf_ex,
> interpreter,
> _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)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] binfmt_elf: fix return value in case of interpreter load failure
On Fri, 12 Apr 2013 16:49:50 +0200 Matthieu CASTET matthieu.cas...@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.cas...@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)
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] binfmt_elf: fix return value in case of interpreter load failure
Hi Andrew,
thanks for your quick review.
Andrew Morton a écrit :
> On Thu, 11 Apr 2013 15:53:09 +0200 Matthieu CASTET
> 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(>interp_elf_ex,
interpreter,
_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,
>> _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;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] binfmt_elf: fix return value in case of interpreter load failure
Hi Andrew,
thanks for your quick review.
Andrew Morton a écrit :
On Thu, 11 Apr 2013 15:53:09 +0200 Matthieu CASTET
matthieu.cas...@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;
}
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] binfmt_elf: fix return value in case of interpreter load failure
On Thu, 11 Apr 2013 15:53:09 +0200 Matthieu CASTET
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,
> _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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] binfmt_elf: fix return value in case of interpreter load failure
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
Cc: Al Viro
Cc: Andrew Morton
---
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,
_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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] binfmt_elf: fix return value in case of interpreter load failure
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.cas...@parrot.com
Cc: Al Viro v...@zeniv.linux.org.uk
Cc: Andrew Morton a...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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.cas...@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;
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
