[PATCH v2 0/5] arm64: kernel: Add support for User Access Override

On Tue, Mar 8, 2016 at 10:22 AM, Catalin Marinas
[off-list ref] wrote:

On Tue, Mar 08, 2016 at 09:39:27AM -0800, Kees Cook wrote:

quoted

On Tue, Mar 8, 2016 at 9:19 AM, Catalin Marinas [off-list ref] wrote:

quoted

On Mon, Mar 07, 2016 at 12:54:33PM -0800, Kees Cook wrote:

quoted

On Mon, Mar 7, 2016 at 9:38 AM, Catalin Marinas [off-list ref] wrote:

quoted

(cc'ing Kees)

On Mon, Mar 07, 2016 at 04:43:19PM +0000, James Morse wrote:

quoted

I've just spotted UAO causes the test_user_copy module (CONFIG_TEST_USER_COPY)
to fail. Who to blame is up for discussion. The test is passing a user pointer
as the 'to' field of copy_from_user(), which it expects to fail gracefully:

lib/test_user_copy.c:75

quoted

    /* Invalid usage: none of these should succeed. */

[ ... ]

quoted

    ret |= test(!copy_from_user(bad_usermem, (char __user *)kmem,
                                PAGE_SIZE),
                "illegal reversed copy_from_user passed");

access_ok() catches the "(char __user *)kmem", causing copy_from_user() to pass
bad_usermem to memset():

arch/arm64/include/asm/uaccess.h:279

quoted

    if (access_ok(VERIFY_READ, from, n))
            n = __copy_from_user(to, from, n);
    else /* security hole - plug it */
            memset(to, 0, n);

This (correctly) trips UAO's "Accessing user space memory outside uaccess.h
routines" message, which is a little confusing to debug, and stops the rest of
the module's tests from being run.

I suggest we don't do anything on arch/arm64 (or arch/arm), I just
consider the test to be broken. The semantics of copy_from_user() don't
say anything about the safety checks on the destination pointer, this is
supposed to be a valid kernel address. The test assumes that if the
source pointer is invalid, the copy_from_user() routine should not touch
the destination.

Hmmm, I'd prefer that copy_*_user was checking both src and
destination. That's what these tests are checking for.

If the kernel pointer (source or destination, depending on the function)
is not valid, I would rather panic the kernel (maybe as a result of a
data abort) than silently returning a non-zero value from
copy_from_user() which doesn't even tell whether the source or
destination pointer is wrong.

The copy_*_user functions are indeed meant to check the validity of user
pointer arguments and safely handle aborts (extable annotations) since
these values are usually provided by user space. But the kernel pointer
arguments are under the control of the kernel, so IMO passing a corrupt
value is a serious kernel/driver bug.

I'm totally fine with that, though I suspect Linus would not like this
(perhaps just Oops instead of panic).

That's what we get on arm64 with PAN enabled (oops).

quoted

Regardless, if that was changed, we could move this entire test into
lkdtm (where system panics are considered a "success").

The only downside is that not all architectures behave in the same way
w.r.t. the copy_*_user() routines, so the test may have some surprises
with the oops not being triggered.

Right. I think if the tests were moved to lkdtm, we could do the
checks in a few stages, where lkdtm would generate the Oops if
copy_*_user correctly errored/ignored the bad combo without Oopsing on
its own.

Anyway, I think the test should also check the invalid
source/destination independently (possibly in combination with
set_fs(KERNEL_DS)).

I'm open to whatever you like. :)

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security