Re: + procfs-mem-permission-cleanup.patch added to -mm tree

[Alexey Dobriyan]

On Mon, Mar 31, 2008 at 01:55:21PM -0700, akpm@xxxxxxxxxxxxxxxxxxxx wrote:
> Subject: procfs: mem permission cleanup
> From: Roland McGrath <roland@xxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/