From 8c652f96d3852b97a49c331cd0bb02d22f3cb31b Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Fri, 24 Apr 2009 01:01:56 +0200 Subject: do_execve() must not clear fs->in_exec if it was set by another thread If do_execve() fails after check_unsafe_exec(), it clears fs->in_exec unconditionally. This is wrong if we race with our sub-thread which also does do_execve: Two threads T1 and T2 and another process P, all share the same ->fs. T1 starts do_execve(BAD_FILE). It calls check_unsafe_exec(), since ->fs is shared, we set LSM_UNSAFE but not ->in_exec. P exits and decrements fs->users. T2 starts do_execve(), calls check_unsafe_exec(), now ->fs is not shared, we set fs->in_exec. T1 continues, open_exec(BAD_FILE) fails, we clear ->in_exec and return to the user-space. T1 does clone(CLONE_FS /* without CLONE_THREAD */). T2 continues without LSM_UNSAFE_SHARE while ->fs is shared with another process. Change check_unsafe_exec() to return res = 1 if we set ->in_exec, and change do_execve() to clear ->in_exec depending on res. When do_execve() suceeds, it is safe to clear ->in_exec unconditionally. It can be set only if we don't share ->fs with another process, and since we already killed all sub-threads either ->in_exec == 0 or we are the only user of this ->fs. Also, we do not need fs->lock to clear fs->in_exec. Signed-off-by: Oleg Nesterov Acked-by: Roland McGrath Acked-by: Hugh Dickins Signed-off-by: Linus Torvalds --- fs/exec.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) (limited to 'fs/exec.c') diff --git a/fs/exec.c b/fs/exec.c index 052a961e41a..a2e6989dbc3 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1077,9 +1077,11 @@ int check_unsafe_exec(struct linux_binprm *bprm) if (p->fs->users > n_fs) { bprm->unsafe |= LSM_UNSAFE_SHARE; } else { - if (p->fs->in_exec) - res = -EAGAIN; - p->fs->in_exec = 1; + res = -EAGAIN; + if (!p->fs->in_exec) { + p->fs->in_exec = 1; + res = 1; + } } unlock_task_sighand(p, &flags); @@ -1284,6 +1286,7 @@ int do_execve(char * filename, struct linux_binprm *bprm; struct file *file; struct files_struct *displaced; + bool clear_in_exec; int retval; retval = unshare_files(&displaced); @@ -1306,8 +1309,9 @@ int do_execve(char * filename, goto out_unlock; retval = check_unsafe_exec(bprm); - if (retval) + if (retval < 0) goto out_unlock; + clear_in_exec = retval; file = open_exec(filename); retval = PTR_ERR(file); @@ -1355,9 +1359,7 @@ int do_execve(char * filename, goto out; /* execve succeeded */ - write_lock(¤t->fs->lock); current->fs->in_exec = 0; - write_unlock(¤t->fs->lock); current->in_execve = 0; mutex_unlock(¤t->cred_exec_mutex); acct_update_integrals(current); @@ -1377,9 +1379,8 @@ out_file: } out_unmark: - write_lock(¤t->fs->lock); - current->fs->in_exec = 0; - write_unlock(¤t->fs->lock); + if (clear_in_exec) + current->fs->in_exec = 0; out_unlock: current->in_execve = 0; -- cgit v1.2.3 From 437f7fdb607f32b737e4da9f14bebcfdac2c90c3 Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Fri, 24 Apr 2009 01:02:45 +0200 Subject: check_unsafe_exec: s/lock_task_sighand/rcu_read_lock/ write_lock(¤t->fs->lock) guarantees we can't wrongly miss LSM_UNSAFE_SHARE, this is what we care about. Use rcu_read_lock() instead of ->siglock to iterate over the sub-threads. We must see all CLONE_THREAD|CLONE_FS threads which didn't pass exit_fs(), it takes fs->lock too. With or without this patch we can miss the freshly cloned thread and set LSM_UNSAFE_SHARE, we don't care. Signed-off-by: Oleg Nesterov Acked-by: Roland McGrath [ Fixed lock/unlock typo - Hugh ] Acked-by: Hugh Dickins Signed-off-by: Linus Torvalds --- fs/exec.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) (limited to 'fs/exec.c') diff --git a/fs/exec.c b/fs/exec.c index a2e6989dbc3..a3a8ce83940 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1060,7 +1060,6 @@ EXPORT_SYMBOL(install_exec_creds); int check_unsafe_exec(struct linux_binprm *bprm) { struct task_struct *p = current, *t; - unsigned long flags; unsigned n_fs; int res = 0; @@ -1068,11 +1067,12 @@ int check_unsafe_exec(struct linux_binprm *bprm) n_fs = 1; write_lock(&p->fs->lock); - lock_task_sighand(p, &flags); + rcu_read_lock(); for (t = next_thread(p); t != p; t = next_thread(t)) { if (t->fs == p->fs) n_fs++; } + rcu_read_unlock(); if (p->fs->users > n_fs) { bprm->unsafe |= LSM_UNSAFE_SHARE; @@ -1083,8 +1083,6 @@ int check_unsafe_exec(struct linux_binprm *bprm) res = 1; } } - - unlock_task_sighand(p, &flags); write_unlock(&p->fs->lock); return res; -- cgit v1.2.3