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/
