Messages in this thread |  | | Date | Sat, 5 Apr 2008 16:48:01 +0400 | From | Alexey Dobriyan <> | Subject | Re: + procfs-mem-permission-cleanup.patch added to -mm tree |
| |
On Mon, Mar 31, 2008 at 01:55:21PM -0700, akpm@linux-foundation.org wrote: > Subject: procfs: mem permission cleanup > From: Roland McGrath <roland@redhat.com> > > 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;
|  |