[PATCH v2 5/5] exec: Add a exec_update_mutex to replace cred_guard_mutex
From: Eric W. Biederman <hidden>
Date: 2020-03-08 21:41:11
Also in:
linux-api, linux-fsdevel, linux-mm, lkml, stable
Subsystem:
exec & binfmt api, elf, filesystems (vfs and infrastructure), memory management - core, scheduler, the rest · Maintainers:
Kees Cook, Alexander Viro, Christian Brauner, Andrew Morton, David Hildenbrand, Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot, Linus Torvalds
Possibly related (same subject, not in this thread)
- 2020-03-17 · Re: [PATCH v2 5/5] exec: Add a exec_update_mutex to replace cred_guard_mutex · Kirill Tkhai <hidden>
- 2020-03-15 · Re: [PATCH v2 5/5] exec: Add a exec_update_mutex to replace cred_guard_mutex · Bernd Edlinger <hidden>
- 2020-03-15 · Re: [PATCH v2 5/5] exec: Add a exec_update_mutex to replace cred_guard_mutex · Bernd Edlinger <hidden>
- 2020-03-13 · Re: [PATCH v2 5/5] exec: Add a exec_update_mutex to replace cred_guard_mutex · Kirill Tkhai <hidden>
- 2020-03-13 · Re: [PATCH v2 5/5] exec: Add a exec_update_mutex to replace cred_guard_mutex · Bernd Edlinger <hidden>
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. The plan is to switch the users of cred_guard_mutex to exec_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@dhcp22.suse.cz/ (local) Link: https://lore.kernel.org/lkml/AM6PR03MB5170B06F3A2B75EFB98D071AE4E60@AM6PR03MB5170.eurprd03.prod.outlook.com/ (local) Link: https://lore.kernel.org/linux-fsdevel/20161102181806.GB1112@redhat.com/ (local) Link: https://lore.kernel.org/lkml/20160923095031.GA14923@redhat.com/ (local) Link: https://lore.kernel.org/lkml/20170213141452.GA30203@redhat.com/ (local) 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" <redacted> --- fs/exec.c | 9 +++++++++ include/linux/sched/signal.h | 9 ++++++++- init/init_task.c | 1 + kernel/fork.c | 1 + 4 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/fs/exec.c b/fs/exec.c
index d820a7272a76..ffeebb1f167b 100644
--- a/fs/exec.c
+++ b/fs/exec.c@@ -1014,6 +1014,7 @@ static int exec_mmap(struct mm_struct *mm) { struct task_struct *tsk; struct mm_struct *old_mm, *active_mm; + int ret; /* Notify parent that we're no longer interested in the old VM */ tsk = current;
@@ -1034,6 +1035,11 @@ static int exec_mmap(struct mm_struct *mm) return -EINTR; } } + + ret = mutex_lock_killable(&tsk->signal->exec_update_mutex); + if (ret) + return ret; + task_lock(tsk); active_mm = tsk->active_mm; membarrier_exec_mmap(mm);
@@ -1438,6 +1444,8 @@ static void free_bprm(struct linux_binprm *bprm) { free_arg_pages(bprm); if (bprm->cred) { + if (!bprm->mm) + mutex_unlock(¤t->signal->exec_update_mutex); mutex_unlock(¤t->signal->cred_guard_mutex); abort_creds(bprm->cred); }
@@ -1487,6 +1495,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/init/init_task.c b/init/init_task.c
index 9e5cbe5eab7b..bd403ed3e418 100644
--- a/init/init_task.c
+++ b/init/init_task.c@@ -26,6 +26,7 @@ static struct signal_struct init_signals = { .multiprocess = HLIST_HEAD_INIT, .rlim = INIT_RLIMITS, .cred_guard_mutex = __MUTEX_INITIALIZER(init_signals.cred_guard_mutex), + .exec_update_mutex = __MUTEX_INITIALIZER(init_signals.exec_update_mutex), #ifdef CONFIG_POSIX_TIMERS .posix_timers = LIST_HEAD_INIT(init_signals.posix_timers), .cputimer = {
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; }
--
2.25.0