[kernel-hardening] [RFC PATCH] lib: Harden csum_partial_copy_from_user

[kernel-hardening] [RFC PATCH] lib: Harden csum_partial_copy_from_user

[Vaishali Thakkar]

The routine csum_partial_copy_from_user is same as csum_partial_copy
but it copies from user space for the checksumming. In other respects
it is identical, and can be used to copy an arbitrarily large buffer
from userspace into the kernel. Conceptually this exposes a similar
attack surface like copy_from_user. So, to validate the given address
we should call check_object_size here.

Note that in the absence of hardened usercopy this will have no impact.

Signed-off-by: Vaishali Thakkar <vaishali.thakkar@oracle.com>
---
 lib/checksum.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/checksum.c b/lib/checksum.c
index d3ec93f..2e0fec8 100644
--- a/lib/checksum.c
+++ b/lib/checksum.c
@@ -33,6 +33,7 @@
  kills, so most of the assembly has to go. */
 
 #include <linux/export.h>
+#include <linux/thread_info.h>
 #include <net/checksum.h>
 
 #include <asm/byteorder.h>
@@ -158,6 +159,7 @@ csum_partial_copy_from_user(const void __user *src, void *dst, int len,
 {
 	int missing;
 
+	check_object_size(dst, len, false);
 	missing = __copy_from_user(dst, src, len);
 	if (missing) {
 		memset(dst + len - missing, 0, missing);
-- 
2.1.4

Re: [kernel-hardening] [RFC PATCH] lib: Harden csum_partial_copy_from_user

[Mark Rutland]

Hi,

On Wed, Nov 02, 2016 at 10:32:49PM +0530, Vaishali Thakkar wrote:
> The routine csum_partial_copy_from_user is same as csum_partial_copy
> but it copies from user space for the checksumming. In other respects
> it is identical, and can be used to copy an arbitrarily large buffer
> from userspace into the kernel. Conceptually this exposes a similar
> attack surface like copy_from_user. So, to validate the given address
> we should call check_object_size here.

Thanks for looking at this! I agree that we should be trying lock down these
homebrew/specialised copy_{to,from}_user routines.

However...

> @@ -158,6 +159,7 @@ csum_partial_copy_from_user(const void __user *src, void *dst, int len,
>  {
>  	int missing;
>  
> +	check_object_size(dst, len, false);
>  	missing = __copy_from_user(dst, src, len);

... here we're just calling into the architecture-specific __copy_from_user(),
and I know that both arm64 and x86 have a check_object_size() call in their
__copy_from_user() implementations.

Is that missing on some architectures?

I think we need to figure out where check_object_size() and other checks (e.g.
kasan_check_size) are expected to live in the hierarchy of uaccess copy
primitives (and/or if they should also live in {get,put)_user()).

I had a plan to try to refactor the generic uaccess code so that we could put
those checks in one place, but I put that on hold as Al Viro was doing some
overlapping refactoring of all the uaccess primitives (and I got busy with some
other work).

Thanks,
Mark.

Re: [kernel-hardening] [RFC PATCH] lib: Harden csum_partial_copy_from_user

[Kees Cook]

On Wed, Nov 2, 2016 at 2:44 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi,
>
> On Wed, Nov 02, 2016 at 10:32:49PM +0530, Vaishali Thakkar wrote:
>> The routine csum_partial_copy_from_user is same as csum_partial_copy
>> but it copies from user space for the checksumming. In other respects
>> it is identical, and can be used to copy an arbitrarily large buffer
>> from userspace into the kernel. Conceptually this exposes a similar
>> attack surface like copy_from_user. So, to validate the given address
>> we should call check_object_size here.
>
> Thanks for looking at this! I agree that we should be trying lock down these
> homebrew/specialised copy_{to,from}_user routines.

Yes, I'm glad to have more eyes on this. I did notice that these are
almost exclusively used in networking. I'd be curious to see how
noticeable this is on performance (though see my inlining comment
below...)

>
> However...
>
>> @@ -158,6 +159,7 @@ csum_partial_copy_from_user(const void __user *src, void *dst, int len,
>>  {
>>       int missing;
>>
>> +     check_object_size(dst, len, false);
>>       missing = __copy_from_user(dst, src, len);
>
> ... here we're just calling into the architecture-specific __copy_from_user(),
> and I know that both arm64 and x86 have a check_object_size() call in their
> __copy_from_user() implementations.

Another issue here is that csum_partial_copy_from_user() isn't an
inline function, so we lose the performance benefits of ignoring
builtin_cost uses.

> Is that missing on some architectures?

Every architecture is _slightly_ different. Most put the check in
__copy_from_user() so it's correctly caught. (x86 puts them in both
since copy*() calls _copy*(), not __copy*() ... >_<)

> I think we need to figure out where check_object_size() and other checks (e.g.
> kasan_check_size) are expected to live in the hierarchy of uaccess copy
> primitives (and/or if they should also live in {get,put)_user()).

Yes, totally agreed. This is going to be needed for the next step of
usercopy hardening, too. We need to have an arch-specific
__actually_do_the_copy() function that has none of the instrumentation
and is only visible to the usercopy functions. Then we can separate
check logic from the action itself. (Right now we can't pass any
additional information to the check (e.g. whether to make exceptions
to slab whitelisting, etc) because the check is deep in __copy*user().

> I had a plan to try to refactor the generic uaccess code so that we could put
> those checks in one place, but I put that on hold as Al Viro was doing some
> overlapping refactoring of all the uaccess primitives (and I got busy with some
> other work).

Al, is your pass at improving uaccess currently finished, or do you
have more changes coming?

-Kees

-- 
Kees Cook
Nexus Security

Re: [kernel-hardening] [RFC PATCH] lib: Harden csum_partial_copy_from_user

[Vaishali Thakkar]

On Thursday 03 November 2016 03:29 AM, Kees Cook wrote:
> On Wed, Nov 2, 2016 at 2:44 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> Hi,
>>
>> On Wed, Nov 02, 2016 at 10:32:49PM +0530, Vaishali Thakkar wrote:
>>> The routine csum_partial_copy_from_user is same as csum_partial_copy
>>> but it copies from user space for the checksumming. In other respects
>>> it is identical, and can be used to copy an arbitrarily large buffer
>>> from userspace into the kernel. Conceptually this exposes a similar
>>> attack surface like copy_from_user. So, to validate the given address
>>> we should call check_object_size here.
>>
>> Thanks for looking at this! I agree that we should be trying lock down these
>> homebrew/specialised copy_{to,from}_user routines.
> 
> Yes, I'm glad to have more eyes on this. I did notice that these are
> almost exclusively used in networking. I'd be curious to see how
> noticeable this is on performance (though see my inlining comment
> below...)
> 
>>
>> However...
>>
>>> @@ -158,6 +159,7 @@ csum_partial_copy_from_user(const void __user *src, void *dst, int len,
>>>  {
>>>       int missing;
>>>
>>> +     check_object_size(dst, len, false);
>>>       missing = __copy_from_user(dst, src, len);
>>
>> ... here we're just calling into the architecture-specific __copy_from_user(),
>> and I know that both arm64 and x86 have a check_object_size() call in their
>> __copy_from_user() implementations.
> 
> Another issue here is that csum_partial_copy_from_user() isn't an
> inline function, so we lose the performance benefits of ignoring
> builtin_cost uses.
> 
>> Is that missing on some architectures?
> 
> Every architecture is _slightly_ different. Most put the check in
> __copy_from_user() so it's correctly caught. (x86 puts them in both
> since copy*() calls _copy*(), not __copy*() ... >_<)

I think still there are some architectures which didn't put the check
in __copy_from_user() [eg. tile].

 
>> I think we need to figure out where check_object_size() and other checks (e.g.
>> kasan_check_size) are expected to live in the hierarchy of uaccess copy
>> primitives (and/or if they should also live in {get,put)_user()).
> 
> Yes, totally agreed. This is going to be needed for the next step of
> usercopy hardening, too. We need to have an arch-specific
> __actually_do_the_copy() function that has none of the instrumentation
> and is only visible to the usercopy functions. Then we can separate
> check logic from the action itself. (Right now we can't pass any
> additional information to the check (e.g. whether to make exceptions
> to slab whitelisting, etc) because the check is deep in __copy*user().

I was actually wondering if there are any cases where we need any
architecture specific extra check(s)?

>> I had a plan to try to refactor the generic uaccess code so that we could put
>> those checks in one place, but I put that on hold as Al Viro was doing some
>> overlapping refactoring of all the uaccess primitives (and I got busy with some
>> other work).
> 
> Al, is your pass at improving uaccess currently finished, or do you
> have more changes coming?
> 
> -Kees
> 

-- 
Vaishali

Re: [kernel-hardening] [RFC PATCH] lib: Harden csum_partial_copy_from_user

[Mark Rutland]

Hi Vaishali,

On Thu, Nov 03, 2016 at 07:44:35AM +0530, Vaishali Thakkar wrote:
> On Thursday 03 November 2016 03:29 AM, Kees Cook wrote:
> > On Wed, Nov 2, 2016 at 2:44 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> >> I know that both arm64 and x86 have a check_object_size() call in their
> >> __copy_from_user() implementations.

> >> Is that missing on some architectures?
> > 
> > Every architecture is _slightly_ different. Most put the check in
> > __copy_from_user() so it's correctly caught. (x86 puts them in both
> > since copy*() calls _copy*(), not __copy*() ... >_<)
> 
> I think still there are some architectures which didn't put the check
> in __copy_from_user() [eg. tile].

I see. :(

Looking again, a grep shows many (even those with MMUs) don't do anything at
all in v4.9-rc2:

[mark@remoulade:~/src/linux]% for ARCH in arch/*; do
printf "%d %s\n" $(git grep check_object_size -- "${ARCH}" | wc -l) ${ARCH};
done | sort -n
0 arch/alpha
0 arch/arc
0 arch/avr32
0 arch/blackfin
0 arch/c6x
0 arch/cris
0 arch/frv
0 arch/h8300
0 arch/hexagon
0 arch/Kconfig
0 arch/m32r
0 arch/m68k
0 arch/metag
0 arch/microblaze
0 arch/mn10300
0 arch/nios2
0 arch/openrisc
0 arch/score
0 arch/sh
0 arch/tile
0 arch/um
0 arch/unicore32
0 arch/xtensa
2 arch/parisc
2 arch/s390
3 arch/arm
4 arch/arm64
4 arch/ia64
4 arch/powerpc
5 arch/sparc
6 arch/mips
6 arch/x86

> I was actually wondering if there are any cases where we need any
> architecture specific extra check(s)?

Generally, I'd expect that to be orthogonal to the hardened usercopy work, and
that check would still be present in the low-level architecture-specific code
even if we made the check_object_size() checks common.

Do you have an example of the kind of thing you're worried about?

Thanks,
Mark.

Re: [kernel-hardening] [RFC PATCH] lib: Harden csum_partial_copy_from_user

[Vaishali Thakkar]

On Thursday 03 November 2016 09:53 AM, Mark Rutland wrote:
> Hi Vaishali,

Hi Mark,
 
> On Thu, Nov 03, 2016 at 07:44:35AM +0530, Vaishali Thakkar wrote:
>> On Thursday 03 November 2016 03:29 AM, Kees Cook wrote:
>>> On Wed, Nov 2, 2016 at 2:44 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>>>> I know that both arm64 and x86 have a check_object_size() call in their
>>>> __copy_from_user() implementations.
> 
>>>> Is that missing on some architectures?
>>>
>>> Every architecture is _slightly_ different. Most put the check in
>>> __copy_from_user() so it's correctly caught. (x86 puts them in both
>>> since copy*() calls _copy*(), not __copy*() ... >_<)
>>
>> I think still there are some architectures which didn't put the check
>> in __copy_from_user() [eg. tile].
> 
> I see. :(
> 
> Looking again, a grep shows many (even those with MMUs) don't do anything at
> all in v4.9-rc2:
> 
> [mark@remoulade:~/src/linux]% for ARCH in arch/*; do
> printf "%d %s\n" $(git grep check_object_size -- "${ARCH}" | wc -l) ${ARCH};
> done | sort -n
> 0 arch/alpha
> 0 arch/arc
> 0 arch/avr32
> 0 arch/blackfin
> 0 arch/c6x
> 0 arch/cris
> 0 arch/frv
> 0 arch/h8300
> 0 arch/hexagon
> 0 arch/Kconfig
> 0 arch/m32r
> 0 arch/m68k
> 0 arch/metag
> 0 arch/microblaze
> 0 arch/mn10300
> 0 arch/nios2
> 0 arch/openrisc
> 0 arch/score
> 0 arch/sh
> 0 arch/tile
> 0 arch/um
> 0 arch/unicore32
> 0 arch/xtensa
> 2 arch/parisc
> 2 arch/s390
> 3 arch/arm
> 4 arch/arm64
> 4 arch/ia64
> 4 arch/powerpc
> 5 arch/sparc
> 6 arch/mips
> 6 arch/x86

Hmm, should we go for sending patches for them? [atleast for the ones with MMUs
and then may be maintainers/developers can check the change]

Also, I think same goes for the the kasan_check. We have only arm64 and x86  
with these checks.

>> I was actually wondering if there are any cases where we need any
>> architecture specific extra check(s)?
> 
> Generally, I'd expect that to be orthogonal to the hardened usercopy work, and
> that check would still be present in the low-level architecture-specific code
> even if we made the check_object_size() checks common.
> 
> Do you have an example of the kind of thing you're worried about?

Ahmm, no. Not at the moment. But I was actually thinking that this might be possible
based on how architecture copies data from the userspace. Probably this can be figure
out later on by architecture maintainers once they have this check ...
 
> Thanks,
> Mark.
> 

-- 
Vaishali

Re: [kernel-hardening] [RFC PATCH] lib: Harden csum_partial_copy_from_user

[Mark Rutland]

On Thu, Nov 03, 2016 at 10:26:41AM +0530, Vaishali Thakkar wrote:
> On Thursday 03 November 2016 09:53 AM, Mark Rutland wrote:
> > On Thu, Nov 03, 2016 at 07:44:35AM +0530, Vaishali Thakkar wrote:
> >> On Thursday 03 November 2016 03:29 AM, Kees Cook wrote:
> >>> On Wed, Nov 2, 2016 at 2:44 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> >>>> I know that both arm64 and x86 have a check_object_size() call in their
> >>>> __copy_from_user() implementations.
> > 
> >>>> Is that missing on some architectures?

> > Looking again, a grep shows many (even those with MMUs) don't do anything at
> > all in v4.9-rc2:
> > 
> > [mark@remoulade:~/src/linux]% for ARCH in arch/*; do
> > printf "%d %s\n" $(git grep check_object_size -- "${ARCH}" | wc -l) ${ARCH};
> > done | sort -n

> Hmm, should we go for sending patches for them? [atleast for the ones with MMUs
> and then may be maintainers/developers can check the change]

If Al's uaccess unification work is arriving shortly, sending patches for those
in parallel is just going to make matters more painful.

So it really depends on when that's likely to appear.

> Also, I think same goes for the the kasan_check. We have only arm64 and x86  
> with these checks.

Yes. I'd hoped to collect all of those behind a common helper, something like:

static inline void check_uaccess_read(void *kaddr, const void __user *uaddr, unsigned long n)
{
	kasan_check_write(kaddr, n);
	check_object_size(kaddr, n, false);
	any_uaddr_sanity_check(uaddr, n);
}

Thanks,
Mark.

Re: [kernel-hardening] [RFC PATCH] lib: Harden csum_partial_copy_from_user

[Vaishali Thakkar]

On Thursday 03 November 2016 11:35 PM, Mark Rutland wrote:
> On Thu, Nov 03, 2016 at 10:26:41AM +0530, Vaishali Thakkar wrote:
>> On Thursday 03 November 2016 09:53 AM, Mark Rutland wrote:
>>> On Thu, Nov 03, 2016 at 07:44:35AM +0530, Vaishali Thakkar wrote:
>>>> On Thursday 03 November 2016 03:29 AM, Kees Cook wrote:
>>>>> On Wed, Nov 2, 2016 at 2:44 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>>>>>> I know that both arm64 and x86 have a check_object_size() call in their
>>>>>> __copy_from_user() implementations.
>>>
>>>>>> Is that missing on some architectures?
> 
>>> Looking again, a grep shows many (even those with MMUs) don't do anything at
>>> all in v4.9-rc2:
>>>
>>> [mark@remoulade:~/src/linux]% for ARCH in arch/*; do
>>> printf "%d %s\n" $(git grep check_object_size -- "${ARCH}" | wc -l) ${ARCH};
>>> done | sort -n
> 
>> Hmm, should we go for sending patches for them? [atleast for the ones with MMUs
>> and then may be maintainers/developers can check the change]
> 
> If Al's uaccess unification work is arriving shortly, sending patches for those
> in parallel is just going to make matters more painful.
> 
> So it really depends on when that's likely to appear.

Makes sense.
 
>> Also, I think same goes for the the kasan_check. We have only arm64 and x86  
>> with these checks.
> 
> Yes. I'd hoped to collect all of those behind a common helper, something like:
> 
> static inline void check_uaccess_read(void *kaddr, const void __user *uaddr, unsigned long n)
> {
> 	kasan_check_write(kaddr, n);
> 	check_object_size(kaddr, n, false);
> 	any_uaddr_sanity_check(uaddr, n);
> }

Ok, sounds reasonable. 

Thanks.
 
> Thanks,
> Mark.
> 

-- 
Vaishali

Re: [kernel-hardening] [RFC PATCH] lib: Harden csum_partial_copy_from_user

[Al Viro]

On Wed, Nov 02, 2016 at 03:59:25PM -0600, Kees Cook wrote:
> On Wed, Nov 2, 2016 at 2:44 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > Hi,
> >
> > On Wed, Nov 02, 2016 at 10:32:49PM +0530, Vaishali Thakkar wrote:
> >> The routine csum_partial_copy_from_user is same as csum_partial_copy
> >> but it copies from user space for the checksumming. In other respects
> >> it is identical, and can be used to copy an arbitrarily large buffer
> >> from userspace into the kernel. Conceptually this exposes a similar
> >> attack surface like copy_from_user. So, to validate the given address
> >> we should call check_object_size here.
> >
> > Thanks for looking at this! I agree that we should be trying lock down these
> > homebrew/specialised copy_{to,from}_user routines.
> 
> Yes, I'm glad to have more eyes on this. I did notice that these are
> almost exclusively used in networking. I'd be curious to see how
> noticeable this is on performance (though see my inlining comment
> below...)

No check_object_size() in that place.  Really.  Look through the call chains.

> Al, is your pass at improving uaccess currently finished, or do you
> have more changes coming?

Yes, and quite a few.  There's going to be a lot of merging between the
architectures in that area.

As for csum stuff...  csum_partial_copy_from_user() is *NOT* an exposed
API.  Really.  It's a helper often used by implementations of
csum_partial_copy_nocheck() and csum_and_copy_from_user().  Those two
are parts of API.  With very limited use.  csum_and_copy_from_user()
has literally only one user - csum_and_copy_from_iter().

csum_partial_copy_nocheck() has slightly wider use.  It has nothing to do
with userland memory, though, and should never be used for such.  First of all,
it's used in implementation of csum_and_copy_{to,from}_iter().  Then
there are
	* skb_copy_and_csum_bits()
	* getfrag callbacks of ip_append_data() - icmp_push_reply(),
ip_reply_glue_bits(), raw_getfrag(), raw6_getfrag()
	* tcp_fragment()
	* really weird shit in drivers/net/ethernet/3com/typhoon.c - the
hardware apparently uses IP-style csum as a signature for the firmware.
Yes, really.  Comments regarding the value of such 'protection' (whatever
it was they were trying to protect) are best directed to 3COM people.

	What would you check the sizes of?  The most common users are
csum_and_copy_from_iter() and skb_copy_and_csum_bits().  And serious
overhead added to either of those will be _very_ painful.