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

Date:	Mon, 14 Mar 2011 08:13:20 -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

On Sun, Mar 13, 2011 at 08:59:48PM -0400, Stephen Wilson wrote:
> On Sun, Mar 13, 2011 at 05:08:59PM -0700, Kees Cook wrote:
> > On Sun, Mar 13, 2011 at 03:49:23PM -0400, Stephen Wilson wrote:
> > >  	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
> 
> If the _target_ task (pid 200) execs then we are OK -- we hold a
> reference to the *old* mm and it is that to which we read and write via
> access_remote_vm().

Right, the old mm is held during read_mem(). But isn't the mm fetched
from check_mem_permission(task) each time pid 100 reads from the
/proc/200/mem fd? (And if so, that's still okay, it still passes through
check_mem_permission() so the access will be validated.)

> In the case of the file->private_data test we are looking at the
> *ptracer* (pid 100).  Here we are guarding against the case where the
> tracer exec's and accidentally leaks the fd (hence the test wrt
> current).  IOW, /proc/pid/mem is implicitly close on exec.  This is just
> a minor feature to protect against buggy user space reading/writing
> mistakenly into the targets address space.

Ah! Right, thanks, that clears that up.

> > 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].
> 
> These patches certainly do not add to the problem -- but they do not try
> to address the general issue either.

The use of check_mem_permission() already protects /proc/pid/mem, but
that test is much stricter than the may_ptrace() checks of things like
/proc/pid/maps. Regardless, yeah, there's no problem here that I can see.

> > 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?
> 
> Yes, precisely.  Once we have a reference to the mm we do not need to
> worry about things changing underneath our feet, so the second check in
> mem_read() is redundant and can be dropped.

Excellent. :)

-Kees

-- 
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/