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 <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Andrew Morton <[email protected]>
---
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
On Thu, 11 Apr 2013 15:53:09 +0200 Matthieu CASTET <[email protected]> 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;
Hi Andrew,
thanks for your quick review.
Andrew Morton a ?crit :
> On Thu, 11 Apr 2013 15:53:09 +0200 Matthieu CASTET <[email protected]> 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;
}
On Fri, 12 Apr 2013 16:49:50 +0200 Matthieu CASTET <[email protected]> wrote:
> Hi Andrew,
>
> thanks for your quick review.
>
> Andrew Morton a __crit :
> > On Thu, 11 Apr 2013 15:53:09 +0200 Matthieu CASTET <[email protected]> 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)
On 04/15, Andrew Morton wrote:
>
> On Fri, 12 Apr 2013 16:49:50 +0200 Matthieu CASTET <[email protected]> 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.