linux-kernel - Re: [PATCH 11/12] proc: make check_mem_permission() return an mm

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20110314000859.GF21770@outflux.net>
Date:	Sun, 13 Mar 2011 17:08:59 -0700
From:	Kees Cook <kees.cook@...onical.com>
To:	Stephen Wilson <wilsons@...rt.ca>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	Michel Lespinasse <walken@...gle.com>,
	Andi Kleen <ak@...ux.intel.com>,
	Rik van Riel <riel@...hat.com>,
	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>,
	Matt Mackall <mpm@...enic.com>,
	David Rientjes <rientjes@...gle.com>,
	Nick Piggin <npiggin@...nel.dk>,
	Andrea Arcangeli <aarcange@...hat.com>,
	Mel Gorman <mel@....ul.ie>, Hugh Dickins <hughd@...gle.com>,
	x86@...nel.org, linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 11/12] proc: make check_mem_permission() return an
 mm_struct on success

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@...rt.ca>
> ---
>  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 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/