Messages in this thread | | | From | (Eric W. Biederman) | Date | Fri, 17 Feb 2017 00:42:08 +1300 | Subject | Re: [PATCH V2 1/2] exec: don't wait for zombie threads with cred_guard_mutex held |
| |
I am slowly working my way through this and I have found something that I have an issue with.
Oleg Nesterov <oleg@redhat.com> writes:
> - In any case we should limit the scope of cred_guard_mutex in execve paths. > It is not clear why do we take it at the start of execve, and worse, it is > not clear why we do we actually overload this mutex to exclude other threads > (except check_unsafe_exec() but this is solveable). The original motivation > was signal->in_exec_mm protection but this idea was replaced by 3c77f8457221 > ("exec: make argv/envp memory visible to oom-killer"). It is just ugly to > call copy_strings/etc with this mutex held.
The original changes that introduced cred_guard_mutex are: a6f76f23d297 ("CRED: Make execve() take advantage of copy-on-write credentials") d84f4f992cbd ("CRED: Inaugurate COW credentials")
So I don't think you actually have your history right.
Beyond that there is a compelling reason to have exec appear atomic from the perspective of ptrace_attach. If the operation is a setuid exec and the tracer does not have permission to trace the original or the result of the exec there could be some significant information leakage if the exec operation is not atomic from the perspective of ptrace_attach.
This is the kind of thing that could easily defeat address space layout randomization of setuid executables if we get this wrong. So I don't think this is a small thing we are talking about.
And while I would like to merge this patch, a patch description that appears to encourage people to make changes that will create security vulnerabilities does not make me comfortable.
Additionally your comment makes me nervous when you are wondering why we take this mutex to exclude other threads and I look in the git history and see:
commit 9b1bf12d5d51bca178dea21b04a0805e29d60cf1 Author: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> Date: Wed Oct 27 15:34:08 2010 -0700
signals: move cred_guard_mutex from task_struct to signal_struct
Oleg Nesterov pointed out we have to prevent multiple-threads-inside-exec itself and we can reuse ->cred_guard_mutex for it. Yes, concurrent execve() has no worth.
Let's move ->cred_guard_mutex from task_struct to signal_struct. It naturally prevent multiple-threads-inside-exec.
Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> Reviewed-by: Oleg Nesterov <oleg@redhat.com> Acked-by: Roland McGrath <roland@redhat.com> Acked-by: David Howells <dhowells@redhat.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
AKA it was your idea and you reviewed the patch that made it happen.
So while I fully agree we have issues here that we need to address and fix your patch description does not inspire confidence.
Eric
| |