Hi Stephen,
On Sun, Mar 13, 2011 at 03:49:23PM -0400, Stephen Wilson wrote:
> This change allows us to take advantage of access_remote_vm(), which in turn
> eliminates a security issue with the mem_write() implementation.
>
> The previous implementation of mem_write() was insecure since the target task
> could exec a setuid-root binary between the permission check and the actual
> write. Holding a reference to the target mm_struct eliminates this
> vulnerability.
>
> Signed-off-by: Stephen Wilson <wilsons@xxxxxxxx>
> ---
> fs/proc/base.c | 58 ++++++++++++++++++++++++++++++++-----------------------
> 1 files changed, 34 insertions(+), 24 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index f6b644f..2af83bd 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -858,22 +863,25 @@ static ssize_t mem_write(struct file * file, const char __user *buf,
> char *page;
> struct task_struct *task = get_proc_task(file->f_path.dentry->d_inode);
> unsigned long dst = *ppos;
> + struct mm_struct *mm;
>
> copied = -ESRCH;
> if (!task)
> goto out_no_task;
>
> - if (check_mem_permission(task))
> - goto out;
> + mm = check_mem_permission(task);
> + copied = PTR_ERR(mm);
> + if (IS_ERR(mm))
> + goto out_task;
>
> copied = -EIO;
> if (file->private_data != (void *)((long)current->self_exec_id))
> - goto out;
> + goto out_mm;
The file->private_data test seems wrong to me. Is there a case were the mm
returned from check_mem_permission(task) can refer to something that is no
longer attached to task?
For example:
- pid 100 ptraces pid 200
- pid 100 opens /proc/200/mem
- pid 200 execs into something else
A read of that mem fd could, IIUC, read from the new pid 200 mm, but
only after passing check_mem_permission(task) again. This is stopped
by the private_data test. But should it, since check_mem_permission()
passed?
Even if it does mean to block it, it's insufficient since pid 200
could just exec u32 many times and align with the original private_data
value. What is that test trying to do? And I'm curious for both mem_write
as well as the existing mem_read use of the test, since I'd like to see
a general solution to the "invalidate /proc fds across exec" so we can
close CVE-2011-1020 for everything[1].
Associated with this, the drop of check_mem_permission(task) during the
mem_read loop implies that the mm is locked during that loop and seems to
reflect what you're saying ("Holding a reference to the target mm_struct
eliminates this vulnerability."), meaning there's no reason to recheck
permissions. Is that accurate?
Thanks,
-Kees
[1] https://lkml.org/lkml/2011/2/7/368
--
Kees Cook
Ubuntu Security Team
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxxx For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>