On Mon, Mar 31, 2008 at 01:55:21PM -0700, akpm@linux-foundation.org wrote:
> Subject: procfs: mem permission cleanup
> From: Roland McGrath
>
> This cleans up the permission checks done for /proc/PID/mem i/o calls. It
> puts all the logic in a new function, check_mem_permission().
>
> The old code repeated the (!MAY_PTRACE(task) || !ptrace_may_attach(task))
> magical expression multiple times. The new function does all that work in one
> place, with clear comments.
>
> The old code called security_ptrace() twice on successful checks, once in
> MAY_PTRACE() and once in __ptrace_may_attach(). Now it's only called once,
> and only if all other checks have succeeded.


> --- a/fs/proc/base.c~procfs-mem-permission-cleanup
> +++ a/fs/proc/base.c
> @@ -195,12 +195,32 @@ static int proc_root_link(struct inode *
> return result;
> }
>
> -#define MAY_PTRACE(task) \
> - (task == current || \
> - (task->parent == current && \
> - (task->ptrace & PT_PTRACED) && \
> - (task_is_stopped_or_traced(task)) && \
> - security_ptrace(current,task) == 0))
> +/*
> + * Return zero if current may access user memory in @task, -error if not.
> + */
> +static int check_mem_permission(struct task_struct *task)
> +{
> + /*
> + * A task can always look at itself, in case it chooses
> + * to use system calls instead of load instructions.
> + */
> + if (task == current)
> + return 0;


OK.

> + /*
> + * If current is actively ptrace'ing, and would also be
> + * permitted to freshly attach with ptrace now, permit it.

because it could attach and read task's memory memory.

Something like this.

> + */
> + if (task->parent == current && (task->ptrace & PT_PTRACED) &&
> + task_is_stopped_or_traced(task) &&
> + ptrace_may_attach(task))
> + return 0;
> +
> + /*
> + * Noone else is allowed.
> + */
> + return -EPERM;


This one is obviously from obvious comments category. :-)

> @@ -722,7 +742,7 @@ static ssize_t mem_read(struct file * fi
> if (!task)
> goto out_no_task;
>
> - if (!MAY_PTRACE(task) || !ptrace_may_attach(task))
> + if (check_mem_permission(task))
> goto out;


I think, name should be proc_mem_permission() (a la vfs_permission()),
but don't have strong opinion on that.

> @@ -748,7 +768,7 @@ static ssize_t mem_read(struct file * fi
>
> this_len = (count > PAGE_SIZE) ? PAGE_SIZE : count;
> retval = access_process_vm(task, src, page, this_len, 0);
> - if (!retval || !MAY_PTRACE(task) || !ptrace_may_attach(task)) {
> + if (!retval || check_mem_permission(task)) {
> if (!ret)
> ret = -EIO;
> break;
> @@ -792,7 +812,7 @@ static ssize_t mem_write(struct file * f
> if (!task)
> goto out_no_task;
>
> - if (!MAY_PTRACE(task) || !ptrace_may_attach(task))
> + if (check_mem_permission(task))
> goto out;


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