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

[Catalin Marinas]

On Tue, Mar 08, 2016 at 09:39:27AM -0800, Kees Cook wrote:
> On Tue, Mar 8, 2016 at 9:19 AM, Catalin Marinas <catalin.marinas at arm.com> wrote:
> > 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.
> 
> 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).

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

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

-- 
Catalin