[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 3 Nov 2016 07:44:35 +0530
From: Vaishali Thakkar <vaishali.thakkar@...cle.com>
To: Kees Cook <keescook@...omium.org>,
"kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com>
Cc: Mark Rutland <mark.rutland@....com>, Al Viro <viro@...iv.linux.org.uk>,
David Windsor <dwindsor@...il.com>
Subject: Re: [RFC PATCH] lib: Harden
csum_partial_copy_from_user
On Thursday 03 November 2016 03:29 AM, Kees Cook wrote:
> On Wed, Nov 2, 2016 at 2:44 PM, Mark Rutland <mark.rutland@....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
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
