Q: proc: hold cred_guard_mutex in check_mem_permission()

Oleg Nesterov
2011-09-28T20:23:58Z

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)