lkml.org 
[lkml]   [2010]   [Feb]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: Split 'flush_old_exec' into two functions - 221af7f87b97431e3ee21ce4b0e77d5411cf1549
    Linus Torvalds wrote:
    >
    > On Tue, 2 Feb 2010, Michal Simek wrote:
    >> Would it be possible to cc me or send that patches to linux-next? I am doing
    >> every day tests and report results on my site. I would be able to catch up
    >> bugs earlier.
    >
    > Normally, that would happen, but this patch got applied early _literally_
    > because I wanted it to hit -rc6 rather than wait any longer. So it had
    > only a day or two of discussion, and probably just a few hours from the
    > final version.

    ok. I just wanted to be sure.

    >
    > That said, I think I may have found the cause.
    >
    > Peter: look at setup_new_exec(), and realize that it got moved _down_ to
    > after all the personality setting. So far, so good, that was the
    > intention, but look at what it does:
    >
    > current->flags &= ~PF_RANDOMIZE;
    >
    > and look at how fs/binfmt_elf.c does it not just after the personality
    > setting, but also after
    >
    > if (!(current->personality & ADDR_NO_RANDOMIZE) && randomize_va_space)
    > current->flags |= PF_RANDOMIZE;
    >
    > so it looks like you may have moved it down too much.
    >
    > I think you did that because you wanted to do that
    >
    > arch_pick_mmap_layout(current->mm);
    >
    > in setup_new_exec(). Which makes total sense, but it all means that the
    > whole preparatory patch did way more than my initial one (which put
    > setup_new_exec() right after flush_old_exec())
    >
    > In fact, it looks like PF_RANDOMIZE never gets set with the new code, but
    > I didn't check if it might not happen somewhere else.
    >
    > But while I doubt that clearing PF_RANDOMIZE will break anything, the
    > movement also affects other thigns. Lookie here:
    >
    > if (elf_read_implies_exec(loc->elf_ex, executable_stack))
    > current->personality |= READ_IMPLIES_EXEC;
    >
    > also happens before setup_new_exec(), and then setup_new_exec() does
    >
    > current->personality &= ~bprm->per_clear;
    >
    > where that per_clear mask may be PER_CLEAR_ON_SETID. Which contains
    > READ_IMPLIES_EXEC.
    >
    > So we now always clear READ_IMPLIES_EXEC for setuid applications.
    >
    > Anyway, I'm not sure this is it, but that's two examples of something that
    > did change unintentionally.
    >
    > Michael, mind trying this (UNTESTED!) patch?

    Just Michal. :-) No worries about.

    It makes conceptual sense,
    > and moves some more of the flushing of the old process state up to
    > "flush_old_exec()" rather than doing it late in "setup_new_exec()".

    yes, your patch works. I tested it on QEMU and on real hw and I can't
    see any visible problem. I will do more test tomorrow.

    Thanks,
    Michal

    >
    > (I suspect we should also move the signal/fd flushing there, but I doubt
    > it matters)
    >
    > Linus
    >
    > ---
    > fs/exec.c | 10 +++++-----
    > 1 files changed, 5 insertions(+), 5 deletions(-)
    >
    > diff --git a/fs/exec.c b/fs/exec.c
    > index 675c3f4..0790a10 100644
    > --- a/fs/exec.c
    > +++ b/fs/exec.c
    > @@ -961,6 +961,11 @@ int flush_old_exec(struct linux_binprm * bprm)
    > goto out;
    >
    > bprm->mm = NULL; /* We're using it now */
    > +
    > + current->flags &= ~PF_RANDOMIZE;
    > + flush_thread();
    > + current->personality &= ~bprm->per_clear;
    > +
    > return 0;
    >
    > out:
    > @@ -997,9 +1002,6 @@ void setup_new_exec(struct linux_binprm * bprm)
    > tcomm[i] = '\0';
    > set_task_comm(current, tcomm);
    >
    > - current->flags &= ~PF_RANDOMIZE;
    > - flush_thread();
    > -
    > /* Set the new mm task size. We have to do that late because it may
    > * depend on TIF_32BIT which is only updated in flush_thread() on
    > * some architectures like powerpc
    > @@ -1015,8 +1017,6 @@ void setup_new_exec(struct linux_binprm * bprm)
    > set_dumpable(current->mm, suid_dumpable);
    > }
    >
    > - current->personality &= ~bprm->per_clear;
    > -
    > /*
    > * Flush performance counters when crossing a
    > * security domain:


    --
    Michal Simek, Ing. (M.Eng)
    PetaLogix - Linux Solutions for a Reconfigurable World
    w: www.petalogix.com p: +61-7-30090663,+42-0-721842854 f: +61-7-30090663


    \
     
     \ /
      Last update: 2010-02-02 20:57    [W:0.030 / U:0.364 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site