linux-kernel - Q: proc: hold cred_guard_mutex in check_mem

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 28 Sep 2011 22:20:20 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Stephen Wilson <wilsons@...rt.ca>,
	Al Viro <viro@...iv.linux.org.uk>
Cc:	Johannes Weiner <jweiner@...hat.com>, linux-kernel@...r.kernel.org
Subject: Q: proc: hold cred_guard_mutex in check_mem_permission()

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)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/