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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <20080405124801.GA4849@martell.zuzino.mipt.ru>
Date:	Sat, 5 Apr 2008 16:48:01 +0400
From:	Alexey Dobriyan <adobriyan@...il.com>
To:	akpm@...ux-foundation.org
Cc:	roland@...hat.com, oleg@...sign.ru, linux-kernel@...r.kernel.org
Subject: Re: + procfs-mem-permission-cleanup.patch added to -mm tree

On Mon, Mar 31, 2008 at 01:55:21PM -0700, akpm@...ux-foundation.org wrote:
> Subject: procfs: mem permission cleanup
> From: Roland McGrath <roland@...hat.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;

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