[PATCH] arm64: uaccess: consistently check object sizes

[PATCH] arm64: uaccess: consistently check object sizes

[Mark Rutland]

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@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Will Deacon <will.deacon@arm.com>
---
 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?

I note that other architectures follow the same pattern, and may need a similar
fixup.

Thanks,
Mark.

diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index 46da3ea..5308d69 100644
--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -379,9 +379,9 @@ static inline unsigned long __must_check copy_from_user(void *to, const void __u
 {
 	unsigned long res = n;
 	kasan_check_write(to, n);
+	check_object_size(to, n, false);
 
 	if (access_ok(VERIFY_READ, from, n)) {
-		check_object_size(to, n, false);
 		res = __arch_copy_from_user(to, from, n);
 	}
 	if (unlikely(res))
@@ -392,9 +392,9 @@ static inline unsigned long __must_check copy_from_user(void *to, const void __u
 static inline unsigned long __must_check copy_to_user(void __user *to, const void *from, unsigned long n)
 {
 	kasan_check_read(from, n);
+	check_object_size(from, n, true);
 
 	if (access_ok(VERIFY_WRITE, to, n)) {
-		check_object_size(from, n, true);
 		n = __arch_copy_to_user(to, from, n);
 	}
 	return n;
-- 
1.9.1

[PATCH] arm64: uaccess: consistently check object sizes

[Kees Cook]

On Tue, Feb 7, 2017 at 4:33 AM, Mark Rutland <mark.rutland@arm.com> 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@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Will Deacon <will.deacon@arm.com>
> ---
>  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.

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

-Kees

>
> Thanks,
> Mark.
>
> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> index 46da3ea..5308d69 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -379,9 +379,9 @@ static inline unsigned long __must_check copy_from_user(void *to, const void __u
>  {
>         unsigned long res = n;
>         kasan_check_write(to, n);
> +       check_object_size(to, n, false);
>
>         if (access_ok(VERIFY_READ, from, n)) {
> -               check_object_size(to, n, false);
>                 res = __arch_copy_from_user(to, from, n);
>         }
>         if (unlikely(res))
> @@ -392,9 +392,9 @@ static inline unsigned long __must_check copy_from_user(void *to, const void __u
>  static inline unsigned long __must_check copy_to_user(void __user *to, const void *from, unsigned long n)
>  {
>         kasan_check_read(from, n);
> +       check_object_size(from, n, true);
>
>         if (access_ok(VERIFY_WRITE, to, n)) {
> -               check_object_size(from, n, true);
>                 n = __arch_copy_to_user(to, from, n);
>         }
>         return n;
> --
> 1.9.1
>



-- 
Kees Cook
Pixel Security

[PATCH] arm64: uaccess: consistently check object sizes

[Mark Rutland]

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@arm.com> 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@arm.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Will Deacon <will.deacon@arm.com>
> > ---
> >  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?

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

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.

Thanks,
Mark.

[PATCH] arm64: uaccess: consistently check object sizes

[Kees Cook]

On Tue, Feb 7, 2017 at 10:52 AM, Mark Rutland <mark.rutland@arm.com> 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@arm.com> 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@arm.com>
>> > Cc: Catalin Marinas <catalin.marinas@arm.com>
>> > Cc: Kees Cook <keescook@chromium.org>
>> > Cc: Will Deacon <will.deacon@arm.com>
>> > ---
>> >  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@chromium.org>

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