lkml.org 
[lkml]   [2020]   [Mar]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/4] proc: Use new infrastructure to fix deadlocks in execve
On Tue, Mar 10, 2020 at 06:45:32PM +0100, Bernd Edlinger wrote:
> This changes lock_trace to use the new exec_update_mutex
> instead of cred_guard_mutex.
>
> This fixes possible deadlocks when the trace is accessing
> /proc/$pid/stack for instance.
>
> This should be safe, as the credentials are only used for reading,
> and task->mm is updated on execve under the new exec_update_mutex.
>
> Signed-off-by: Bernd Edlinger <bernd.edlinger@hotmail.de>

I have the same question here as in 3/4. I should probably rescind my
Reviewed-by until I'm convinced about the security-safety of this -- why
is this not a race against cred changes?

-Kees

> ---
> fs/proc/base.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index ebea950..4fdfe4f 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -403,11 +403,11 @@ static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns,
>
> static int lock_trace(struct task_struct *task)
> {
> - int err = mutex_lock_killable(&task->signal->cred_guard_mutex);
> + int err = mutex_lock_killable(&task->signal->exec_update_mutex);
> if (err)
> return err;
> if (!ptrace_may_access(task, PTRACE_MODE_ATTACH_FSCREDS)) {
> - mutex_unlock(&task->signal->cred_guard_mutex);
> + mutex_unlock(&task->signal->exec_update_mutex);
> return -EPERM;
> }
> return 0;
> @@ -415,7 +415,7 @@ static int lock_trace(struct task_struct *task)
>
> static void unlock_trace(struct task_struct *task)
> {
> - mutex_unlock(&task->signal->cred_guard_mutex);
> + mutex_unlock(&task->signal->exec_update_mutex);
> }
>
> #ifdef CONFIG_STACKTRACE
> --
> 1.9.1

--
Kees Cook

\
 
 \ /
  Last update: 2020-03-11 20:11    [W:0.286 / U:0.940 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site