Q: proc: hold cred_guard_mutex in check_mem_permission()

* Q: proc: hold cred_guard_mutex in check_mem_permission()
@ 2011-09-28 20:20 Oleg Nesterov
  2011-09-29  7:13 ` Stephen Wilson
  0 siblings, 1 reply; 5+ messages in thread
From: Oleg Nesterov @ 2011-09-28 20:20 UTC (permalink / raw)
  To: Stephen Wilson, Al Viro; +Cc: Johannes Weiner, linux-kernel

Another change we probably need to backport, 18f661bc
"proc: hold cred_guard_mutex in check_mem_permission()".

>From the changelog:

    Avoid a potential race when task exec's and we get a new ->mm but
    check against the old credentials in ptrace_may_access().

Could you please explain? How can we race with exec?

This task is either current, or it is TASK_TRACED and we are the
tracer. In the latter case nobody can resume it except SIGKILL.
And the killed task obviously can't exec.

Afaics, all we need is: we should read task->mm after the
task_is_traced() check, we do not need the mutex.

IOW, what do you think about the patch below? I have no idea how
can I test it (and it wasn't even applied/compiled).

Also, I'd appreciate if you can explain the PTRACE_MODE_ATTACH
check. Again, we are already the tracer.

Oleg.

--- x/fs/proc/base.c
+++ x/fs/proc/base.c
@@ -194,38 +194,31 @@ static int proc_root_link(struct inode *
 	return result;
 }
 
-static struct mm_struct *__check_mem_permission(struct task_struct *task)
+static int __check_mem_permission(struct task_struct *task)
 {
-	struct mm_struct *mm;
-
-	mm = get_task_mm(task);
-	if (!mm)
-		return ERR_PTR(-EINVAL);
-
 	/*
 	 * A task can always look at itself, in case it chooses
 	 * to use system calls instead of load instructions.
 	 */
 	if (task == current)
-		return mm;
+		return 0;
 
 	/*
 	 * If current is actively ptrace'ing, and would also be
 	 * permitted to freshly attach with ptrace now, permit it.
 	 */
-	if (task_is_stopped_or_traced(task)) {
+	if (task_is_traced(task)) {
 		int match;
 		rcu_read_lock();
 		match = (ptrace_parent(task) == current);
 		rcu_read_unlock();
 		if (match && ptrace_may_access(task, PTRACE_MODE_ATTACH))
-			return mm;
+			return 0;
 	}
 
 	/*
 	 * No one else is allowed.
 	 */
-	mmput(mm);
 	return ERR_PTR(-EPERM);
 }
 
@@ -238,18 +231,11 @@ static struct mm_struct *check_mem_permi
 	struct mm_struct *mm;
 	int err;
 
-	/*
-	 * Avoid racing if task exec's as we might get a new mm but validate
-	 * against old credentials.
-	 */
-	err = mutex_lock_killable(&task->signal->cred_guard_mutex);
+	err = __check_mem_permission(task);
 	if (err)
 		return ERR_PTR(err);
 
-	mm = __check_mem_permission(task);
-	mutex_unlock(&task->signal->cred_guard_mutex);
-
-	return mm;
+	return get_task_mm(task) ?: ERR_PTR(-EINVAL);
 }
 
 struct mm_struct *mm_for_maps(struct task_struct *task)


^ permalink raw reply	[flat|nested] 5+ messages in thread