linux-kernel - Re: [PATCH v2 07/19] x86: introduce __uaccess_begin_nospec and ASM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 12 Jan 2018 11:26:30 -0800
From:   Dan Williams <dan.j.williams@...el.com>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-arch@...r.kernel.org, Andi Kleen <ak@...ux.intel.com>,
        Kees Cook <keescook@...omium.org>,
        kernel-hardening@...ts.openwall.com, X86 ML <x86@...nel.org>,
        Ingo Molnar <mingo@...hat.com>,
        Al Viro <viro@...iv.linux.org.uk>,
        "H. Peter Anvin" <hpa@...or.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Alan Cox <alan@...ux.intel.com>
Subject: Re: [PATCH v2 07/19] x86: introduce __uaccess_begin_nospec and ASM_IFENCE

On Fri, Jan 12, 2018 at 10:58 AM, Josh Poimboeuf <jpoimboe@...hat.com> wrote:
> On Fri, Jan 12, 2018 at 10:21:43AM -0800, Dan Williams wrote:
>> > That just sounds wrong.  What if the speculation starts *after* the
>> > access_ok() check?  Then the barrier has no purpose.
>> >
>> > Most access_ok/get_user/copy_from_user calls are like this:
>> >
>> >   if (copy_from_user(...uptr..))  /* or access_ok() or get_user() */
>> >         return -EFAULT;
>> >
>> > So in other words, the usercopy function is called *before* the branch.
>> >
>> > But to halt speculation, the lfence needs to come *after* the branch.
>> > So putting lfences *before* the branch doesn't solve anything.
>> >
>> > So what am I missing?
>>
>> We're trying to prevent a pointer under user control from being
>> de-referenced inside the kernel, before we know it has been limited to
>> something safe. In the following sequence the branch we are worried
>> about speculating is the privilege check:
>>
>> if (access_ok(uptr))  /* <--- Privelege Check */
>>     if (copy_from_user_(uptr))
>>
>> The cpu can speculatively skip that access_ok() check and cause a read
>> of kernel memory.
>
> Converting your example code to assembly:
>
>         call    access_ok # no speculation which started before this point is allowed to continue past this point
>         test    %rax, %rax
>         jne     error_path
> dereference_uptr:
>         (do nefarious things with the user pointer)
>
> error_path:
>         mov -EINVAL, %rax
>         ret
>
> So the CPU is still free to speculately execute the dereference_uptr
> block because the lfence was before the 'jne error_path' branch.

By the time we get to de-reference uptr we know it is not pointing at
kernel memory, because access_ok would have failed and the cpu would
have waited for that failure result before doing anything else.

Now the vulnerability that still exists after this fence is if we do
something with the value returned from de-referencing that pointer.
I.e. if we do:

get_user(val, uptr);
if (val < array_max)
    array[val];

...then we're back to having a user controlled pointer in the kernel.
This is the point where we need array_ptr() to sanitize things.