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:

quoted

On Sun, Mar 13, 2011 at 03:49:23PM -0400, Stephen Wilson wrote:

quoted

 	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.

quoted

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.

quoted

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, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>