Re: [PATCH] arm64: uaccess: consistently check object sizes

[Kees Cook]

On Tue, Feb 7, 2017 at 10:52 AM, Mark Rutland <mark.rutland@xxxxxxx> wrote:
> On Tue, Feb 07, 2017 at 10:27:52AM -0800, Kees Cook wrote:
>> On Tue, Feb 7, 2017 at 4:33 AM, Mark Rutland <mark.rutland@xxxxxxx> wrote:
>> > Currently in arm64's copy_{to,from}_user, we only check the
>> > source/destination object size if access_ok() tells us the user access
>> > is permissible.
>> >
>> > However, in copy_from_user() we'll subsequently zero any remainder on
>> > the destination object. If we failed the access_ok() check, that applies
>> > to the whole object size, which we didn't check.
>> >
>> > To ensure that we catch that case, this patch hoists check_object_size()
>> > to the start of copy_from_user(), matching __copy_from_user() and
>> > __copy_to_user(). To make all of our uaccess copy primitives consistent,
>> > the same is done to copy_to_user().
>> >
>> > Signed-off-by: Mark Rutland <mark.rutland@xxxxxxx>
>> > Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
>> > Cc: Kees Cook <keescook@xxxxxxxxxxxx>
>> > Cc: Will Deacon <will.deacon@xxxxxxx>
>> > ---
>> >  arch/arm64/include/asm/uaccess.h | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > Kees, Was there any rationale for not handling the !access_ok() case?
>>
>> So, when I pulled the similar code for other architectures out of PaX,
>> I retained this pattern. When I reworked x86 and added arm64, it
>> seemed sensible to optimize the check to follow access_ok(), since if
>> that failed, why do the checking work... but yes, in copy_from_user(),
>> we'll wipe the destination without having done the check. Ewww.
>> Excellent catch.
>
> Can I take that as an Acked-by?

Sure!

Acked-by: Kees Cook <keescook@xxxxxxxxxxxx>

>
> FWIW, longer term I'd love to fold that and the KASAN checks into the
> core uaccess headers, so that we consistently apply them everywhere. If
> Al is done reworking the uaccess code, I can have another look.

I *think* he's done, yes. I haven't seen anything recently coming in
from him there.

Yeah, if we can refactor the uaccess stuff a bunch, hopefully we can
get an API where we can do the slab-whitelist exceptions (i.e. skip
slab checks in certain conditions).

>> > I note that other architectures follow the same pattern, and may need a similar
>> > fixup.
>>
>> I would agree. It will need some fiddling, though. If you look at ARM,
>> it's implicitly after the access_ok() check because
>> check_object_size() is only run in __copy_*_user().
>>
>> (I still think the whole memset(to, 0, n) thing is a bit dangerous...
>> it's kind of a "write 0 anywhere" primitive if an attacker can control
>> the kernel address at all...)
>
> If the user can control the kernel address, it's an arbitrary write if
> access_ok() succeeds, so we've lost regardless.

True, but then what's the point of running the check before the
access_ok()? :) But yes, let's do the check before access_ok() in the
copy_* case.

> The zeroing of the buffer is itself is attempting to minimise the impact
> of buffers not being fully initialised by a copy_from_user, so removing
> it would open another class of issue.

Yeah, it's the lesser of two evils. :)

-Kees

-- 
Kees Cook
Pixel Security