Bernd Edlinger <bernd.edlinger@xxxxxxxxxx> writes: > On 3/5/20 10:16 PM, Eric W. Biederman wrote: >> >> The cred_guard_mutex is problematic. The cred_guard_mutex is held >> over the userspace accesses as the arguments from userspace are read. >> The cred_guard_mutex is held of PTRACE_EVENT_EXIT as the the other >> threads are killed. The cred_guard_mutex is held over >> "put_user(0, tsk->clear_child_tid)" in exit_mm(). >> >> Any of those can result in deadlock, as the cred_guard_mutex is held >> over a possible indefinite userspace waits for userspace. >> >> Add exec_update_mutex that is only held over exec updating process >> with the new contents of exec, so that code that needs not to be >> confused by exec changing the mm and the cred in ways that can not >> happen during ordinary execution of a process can take. >> >> The plan is to switch the users of cred_guard_mutex to >> exed_udpate_mutex one by one. This lets us move forward while still >> being careful and not introducing any regressions. >> >> Link: https://lore.kernel.org/lkml/20160921152946.GA24210@xxxxxxxxxxxxxx/ >> Link: https://lore.kernel.org/lkml/AM6PR03MB5170B06F3A2B75EFB98D071AE4E60@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ >> Link: https://lore.kernel.org/linux-fsdevel/20161102181806.GB1112@xxxxxxxxxx/ >> Link: https://lore.kernel.org/lkml/20160923095031.GA14923@xxxxxxxxxx/ >> Link: https://lore.kernel.org/lkml/20170213141452.GA30203@xxxxxxxxxx/ >> Ref: 45c1a159b85b ("Add PTRACE_O_TRACEVFORKDONE and PTRACE_O_TRACEEXIT facilities.") >> Ref: 456f17cd1a28 ("[PATCH] user-vm-unlock-2.5.31-A2") >> Signed-off-by: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx> >> --- >> fs/exec.c | 4 ++++ >> include/linux/sched/signal.h | 9 ++++++++- >> kernel/fork.c | 1 + >> 3 files changed, 13 insertions(+), 1 deletion(-) >> >> diff --git a/fs/exec.c b/fs/exec.c >> index c243f9660d46..ad7b518f906d 100644 >> --- a/fs/exec.c >> +++ b/fs/exec.c >> @@ -1182,6 +1182,7 @@ static int de_thread(struct linux_binprm *bprm, struct task_struct *tsk) >> release_task(leader); >> } >> >> + mutex_lock(¤t->signal->exec_update_mutex); >> bprm->unrecoverable = true; >> sig->group_exit_task = NULL; >> sig->notify_count = 0; >> @@ -1425,6 +1426,8 @@ static void free_bprm(struct linux_binprm *bprm) >> { >> free_arg_pages(bprm); >> if (bprm->cred) { >> + if (bprm->unrecoverable) >> + mutex_unlock(¤t->signal->exec_update_mutex); >> mutex_unlock(¤t->signal->cred_guard_mutex); >> abort_creds(bprm->cred); >> } >> @@ -1474,6 +1477,7 @@ void install_exec_creds(struct linux_binprm *bprm) >> * credentials; any time after this it may be unlocked. >> */ >> security_bprm_committed_creds(bprm); >> + mutex_unlock(¤t->signal->exec_update_mutex); >> mutex_unlock(¤t->signal->cred_guard_mutex); >> } >> EXPORT_SYMBOL(install_exec_creds); >> diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h >> index 88050259c466..a29df79540ce 100644 >> --- a/include/linux/sched/signal.h >> +++ b/include/linux/sched/signal.h >> @@ -224,7 +224,14 @@ struct signal_struct { >> >> struct mutex cred_guard_mutex; /* guard against foreign influences on >> * credential calculations >> - * (notably. ptrace) */ >> + * (notably. ptrace) >> + * Deprecated do not use in new code. >> + * Use exec_update_mutex instead. >> + */ >> + struct mutex exec_update_mutex; /* Held while task_struct is being >> + * updated during exec, and may have >> + * inconsistent permissions. >> + */ >> } __randomize_layout; >> >> /* >> diff --git a/kernel/fork.c b/kernel/fork.c >> index 60a1295f4384..12896a6ecee6 100644 >> --- a/kernel/fork.c >> +++ b/kernel/fork.c >> @@ -1594,6 +1594,7 @@ static int copy_signal(unsigned long clone_flags, struct task_struct *tsk) >> sig->oom_score_adj_min = current->signal->oom_score_adj_min; >> >> mutex_init(&sig->cred_guard_mutex); >> + mutex_init(&sig->exec_update_mutex); >> >> return 0; >> } >> > Don't you need to add something like this to init/init_task.c ? > > .exec_update_mutex = __MUTEX_INITIALIZER(init_signals.exec_update_mutex), Yes. I overlooked that. Thank you. Eric
