Q: proc: disable mem_write after exec

[Oleg Nesterov <oleg@redhat.com>]

Hello.

The last question, I promise ;)

Another change we probably need to backport,

    commit 26947f8c8f9598209001cdcd31bb2162a2e54691
    proc: disable mem_write after exec

    This change makes mem_write() observe the same constraints as mem_read().

mem_read() looks equally confusing to me, may be Linus can explain.
This self_exec_id check was added in 29f279c7 (history tree)
"v2.4.5.5 -> v2.4.5.6", the changelog can't help.

    This
    is particularly important for mem_write as an accidental leak of the fd across
    an exec could result in arbitrary modification of the target process' memory.

    ...

	@@ -850,6 +850,10 @@ static ssize_t mem_write(struct file * file, const char __user *buf,
		if (check_mem_permission(task))
			goto out;
	 
	+	copied = -EIO;
	+	if (file->private_data != (void *)((long)current->self_exec_id))
	+		goto out;
	+
		copied = -ENOMEM;
		page = (char *)__get_free_page(GFP_TEMPORARY);
		if (!page)


Could you explain this? Why this is wrong from the security pov? We are
the tracer, this was checked by check_mem_permission(). We can modify
this memory even without /proc/pid/mem.

And in any case, why do we check current's self_exec_id? I'd understand
if mem_open/mem_read/mem_writed used task->self_exec_id, see the trivial
patch below. With this patch this check means: this task has changed its
->mm after /proc/pid/mem was opened, abort. And perhaps this was the
actual intent. May be makes sense.

But the real question is, why (from the security pov) we can't simply kill
these self_exec_id checks?

Not to mention, it would be nice to remove self_exec_id/parent_exec_id too.
Ignoring mem_open(), afaics the _only_ reason we need these id's is that
linux allows clone(CLONE_PARENT | SIGKILL).

Thanks,

Oleg.


--- x/fs/proc/base.c
+++ x/fs/proc/base.c
@@ -816,7 +816,17 @@ static const struct file_operations proc
 
 static int mem_open(struct inode* inode, struct file* file)
 {
-	file->private_data = (void*)((long)current->self_exec_id);
+	struct task_struct *task;
+
+	rcu_read_lock();
+	task = pid_task(proc_pid(file->f_path.dentry->d_inode), PIDTYPE_PID);
+	if (task)
+		file->private_data = (void*)((long)task->self_exec_id);
+	rcu_read_unlock();
+
+	if (!task)
+		return -EINVAL;
+
 	/* OK to pass negative loff_t, we can catch out-of-range */
 	file->f_mode |= FMODE_UNSIGNED_OFFSET;
 	return 0;
@@ -846,7 +856,7 @@ static ssize_t mem_read(struct file * fi
 
 	ret = -EIO;
  
-	if (file->private_data != (void*)((long)current->self_exec_id))
+	if (file->private_data != (void*)((long)task->self_exec_id))
 		goto out_put;
 
 	ret = 0;
@@ -908,7 +918,7 @@ static ssize_t mem_write(struct file * f
 		goto out_free;
 
 	copied = -EIO;
-	if (file->private_data != (void *)((long)current->self_exec_id))
+	if (file->private_data != (void *)((long)task->self_exec_id))
 		goto out_mm;
 
 	copied = 0;