Re: [PATCH 0/7] x86: uaccess hardening, easy part

[Kees Cook]

On Tue, May 24, 2016 at 8:55 PM, Brian Gerst <brgerst@xxxxxxxxx> wrote:
> On Tue, May 24, 2016 at 6:48 PM, Andy Lutomirski <luto@xxxxxxxxxx> wrote:
>> This series hardens x86's uaccess code a bit.  It adds warnings for
>> some screwups, adds an OOPS for a major exploitable screwup, and it
>> improves debuggability a bit by indicating non-default fs in oopses.
>>
>> It shouldn't cause any new OOPSes except in the particularly
>> dangerous case where the kernel faults on a kernel address under
>> USER_DS, which indicates that an access_ok is missing and is likely
>> to be easily exploitable -- OOPSing will make it harder to exploit.
>>
>> I have some draft patches to force OOPSes on user address accesses
>> under KERNEL_DS (which is a big no-no), but I'd rather make those
>> warn instead of OOPSing, and I don't have a good implementation of
>> that yet.  Those patches aren't part of this series.
>>
>> Andy Lutomirski (7):
>>   x86/xen: Simplify set_aliased_prot
>>   x86/extable: Pass error_code and an extra unsigned long to exhandlers
>>   x86/uaccess: Give uaccess faults their own handler
>>   x86/dumpstack: If addr_limit is non-default, display it
>>   x86/uaccess: Warn on uaccess faults other than #PF
>>   x86/uaccess: Don't fix up USER_DS uaccess faults to kernel addresses
>>   x86/uaccess: OOPS or warn on a fault with KERNEL_DS and
>>     !pagefault_disabled()
>>
>>  arch/x86/include/asm/uaccess.h   |  19 ++++---
>>  arch/x86/kernel/cpu/mcheck/mce.c |   2 +-
>>  arch/x86/kernel/dumpstack_32.c   |   4 ++
>>  arch/x86/kernel/dumpstack_64.c   |   5 ++
>>  arch/x86/kernel/kprobes/core.c   |   6 +-
>>  arch/x86/kernel/traps.c          |   6 +-
>>  arch/x86/lib/getuser.S           |  12 ++--
>>  arch/x86/lib/putuser.S           |  10 ++--
>>  arch/x86/mm/extable.c            | 120 ++++++++++++++++++++++++++++++++++-----
>>  arch/x86/mm/fault.c              |   2 +-
>>  arch/x86/xen/enlighten.c         |   4 +-
>>  11 files changed, 145 insertions(+), 45 deletions(-)
>
> I'd also like to see the use of set_fs() (which has been grossly
> misnamed since ancient versions of Linux) significantly reduced.  Many
> of these uses are in compat syscalls, which do:
> - read the user memory
> - convert it to the native format
> - call set_fs(KERNEL_DS)
> - pass it to the native syscall which does another user copy.
>
> By separating the core syscall code from the userspace accesses so
> that it only touches kernel memory, you can eliminate the set_fs() and
> the extra copy from the compat case.
>
> I had started work on this a while back but never finished it.  I'll
> look at bringing it up to date.

Yes, please! This kind of compat handling is very wrong. :( We've had
nothing but problems from having such a large piece of code running
under KERNEL_DS.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security