Re: [PATCH V2 1/2] exec: don't wait for zombie threads with cred_guard_mutex held

[Eric W. Biederman]

I am slowly working my way through this and I have found something that
I have an issue with.

Oleg Nesterov <oleg@xxxxxxxxxx> 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@xxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxx>
    Reviewed-by: Oleg Nesterov <oleg@xxxxxxxxxx>
    Acked-by: Roland McGrath <roland@xxxxxxxxxx>
    Acked-by: David Howells <dhowells@xxxxxxxxxx>
    Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
    Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>

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