Messages in this thread Patch in this message | | | Date | Thu, 29 Sep 2011 16:17:06 +0200 | From | Oleg Nesterov <> | Subject | Q: proc: disable mem_write after exec |
| |
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;
| |