Re: [PATCH 3/4] proc: io_accounting: Use new infrastructure to fix deadlocks in execve

[ebiederm@xmission.com (Eric W. Biederman)]

Bernd Edlinger <bernd.edlinger@hotmail.de> writes:

> This changes do_io_accounting to use the new exec_update_mutex
> instead of cred_guard_mutex.
>
> This fixes possible deadlocks when the trace is accessing
> /proc/$pid/io for instance.
>
> This should be safe, as the credentials are only used for reading.

This is an improvement.

We probably want to do this just as an incremental step in making things
better but perhaps I am blind but I am not finding the reason for
guarding this with the cred_guard_mutex to be at all persuasive.

I think moving the ptrace_may_access check down to after the
unlock_task_sighand would be just as effective at addressing the
concerns raised in the original commit.  I think the task_lock provides
all of the barrier we need to make it safe to move the ptrace_may_access
checks safe.

The reason I say this is I don't see exec changing ->ioac.  Just
performing some I/O which would update the io accounting statistics.

Can anyone see if I am wrong?

Eric


commit 293eb1e7772b25a93647c798c7b89bf26c2da2e0
Author: Vasiliy Kulikov <segoon@openwall.com>
Date:   Tue Jul 26 16:08:38 2011 -0700

    proc: fix a race in do_io_accounting()
    
    If an inode's mode permits opening /proc/PID/io and the resulting file
    descriptor is kept across execve() of a setuid or similar binary, the
    ptrace_may_access() check tries to prevent using this fd against the
    task with escalated privileges.
    
    Unfortunately, there is a race in the check against execve().  If
    execve() is processed after the ptrace check, but before the actual io
    information gathering, io statistics will be gathered from the
    privileged process.  At least in theory this might lead to gathering
    sensible information (like ssh/ftp password length) that wouldn't be
    available otherwise.
    
    Holding task->signal->cred_guard_mutex while gathering the io
    information should protect against the race.
    
    The order of locking is similar to the one inside of ptrace_attach():
    first goes cred_guard_mutex, then lock_task_sighand().
    
    Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
    Cc: Al Viro <viro@zeniv.linux.org.uk>
    Cc: <stable@kernel.org>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>



> Signed-off-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
> ---
>  fs/proc/base.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 4fdfe4f..529d0c6 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2770,7 +2770,7 @@ static int do_io_accounting(struct task_struct *task, struct seq_file *m, int wh
>  	unsigned long flags;
>  	int result;
>  
> -	result = mutex_lock_killable(&task->signal->cred_guard_mutex);
> +	result = mutex_lock_killable(&task->signal->exec_update_mutex);
>  	if (result)
>  		return result;
>  
> @@ -2806,7 +2806,7 @@ static int do_io_accounting(struct task_struct *task, struct seq_file *m, int wh
>  	result = 0;
>  
>  out_unlock:
> -	mutex_unlock(&task->signal->cred_guard_mutex);
> +	mutex_unlock(&task->signal->exec_update_mutex);
>  	return result;
>  }