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.
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?
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()".
(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: