| Message ID | AM6PR03MB51705D211EC8E7EA270627B1E4FF0@AM6PR03MB5170.eurprd03.prod.outlook.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Use new infrastructure to fix deadlocks in execve | expand |
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> Reviewed-by: Kees Cook <keescook@chromium.org> -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
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
On 3/11/20 8:10 PM, Kees Cook wrote: > 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? > The credentials of a thread that is currently executing execve is already set in the bprm structure, however the credential in the task structure is not yet changed, as well as the process memory map keeps stable until the exec_update_mutex is acquired. What is done with this functions is access the call stack of the process before the new executable is actually started. There would immediately be a severe security problem if we did not use any mutex as the check would be then with the old credential, but the stack trace would potentially reveal secret function calls that are done by a setuid program when it starts up. Bernd. > -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 >
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
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> --- fs/proc/base.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)