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

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

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). Regardless, if that was changed,
we could move this entire test into lkdtm (where system panics are
considered a "success").

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security