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

[Catalin Marinas]

On Mon, Mar 07, 2016 at 12:54:33PM -0800, Kees Cook wrote:
> On Mon, Mar 7, 2016 at 9:38 AM, Catalin Marinas <catalin.marinas at arm.com> wrote:
> > (cc'ing Kees)
> >
> > On Mon, Mar 07, 2016 at 04:43:19PM +0000, James Morse wrote:
> >> 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
> >> >     /* Invalid usage: none of these should succeed. */
> >> [ ... ]
> >> >     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
> >> >     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.

-- 
Catalin