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 12:01:04 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Dan Williams <dan.j.williams@...el.com>
Cc:     Josh Poimboeuf <jpoimboe@...hat.com>,
        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>,
        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 11:26 AM, Dan Williams <dan.j.williams@...el.com> wrote:
>
> 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.

I'm not actually convinced that's right in the original patches,
exactly because of the issue that Josh pointed out: even if there is a
comparison inside access_ok() that will be properly serialized, then
that comparison can (and sometimes does) just cause a truth value to
be generated, and then there  might be *another* comparison of that
return value after the lfence. And while the return value is table,
the conditional branch on that comparison isn't.

The new model of just doing it together with the STAC should be fine, though.

I do think that it would be a good idea to very expressly document the
fact that it's not that the user access itself is unsafe. I do agree
that things like "get_user()" want to be protected, but not because of
any direct bugs or problems with get_user() and friends, but simply
because get_user() is an excellent source of a pointer that is
obviously controlled from a potentially attacking user space. So it's
a prime candidate for then finding _subsequent_ accesses that can then
be used to perturb the cache.

                 Linus