Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <cover.1620186182.git.jpoimboe@redhat.com> <5ba93cdbf35ab40264a9265fc24575a9b2f813b3.1620186182.git.jpoimboe@redhat.com>
In-Reply-To: <5ba93cdbf35ab40264a9265fc24575a9b2f813b3.1620186182.git.jpoimboe@redhat.com>
From:   Andy Lutomirski <luto@kernel.org>
Date:   Wed, 5 May 2021 09:55:11 -0700
Message-ID: <CALCETrWQrMkeP+=pkNVNvSs9q3ZhhLq_ceHJ-N72Urp2KBrUfQ@mail.gmail.com>
Subject: Re: [PATCH v4 3/4] x86/uaccess: Use pointer masking to limit uaccess speculation
To:     Josh Poimboeuf <jpoimboe@redhat.com>
Cc:     Al Viro <viro@zeniv.linux.org.uk>, X86 ML <x86@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Will Deacon <will@kernel.org>,
        Dan Williams <dan.j.williams@intel.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Waiman Long <longman@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Andrew Cooper <andrew.cooper3@citrix.com>,
        Andy Lutomirski <luto@kernel.org>,
        Christoph Hellwig <hch@lst.de>,
        David Laight <David.Laight@aculab.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Borislav Petkov <bp@alien8.de>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, May 4, 2021 at 8:55 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> +/*
> + * Sanitize a user pointer such that it becomes NULL if it's not a valid user
> + * pointer.  This prevents speculatively dereferencing a user-controlled
> + * pointer to kernel space if access_ok() speculatively returns true.  This
> + * should be done *after* access_ok(), to avoid affecting error handling
> + * behavior.
> + */
> +#define mask_user_ptr(ptr)                                             \
> +({                                                                     \
> +       unsigned long _ptr = (__force unsigned long)ptr;                \
> +       unsigned long mask;                                             \
> +                                                                       \
> +       asm volatile("cmp %[max], %[_ptr]\n\t"                          \
> +                    "sbb %[mask], %[mask]\n\t"                         \
> +                    : [mask] "=r" (mask)                               \
> +                    : [_ptr] "r" (_ptr),                               \
> +                      [max] "r" (TASK_SIZE_MAX)                        \
> +                    : "cc");                                           \
> +                                                                       \
> +       mask &= _ptr;                                                   \
> +       ((typeof(ptr)) mask);                                           \
> +})

Is there an equally efficient sequence that squishes the pointer value
to something noncanonical or something like -1 instead of 0?  I'm not
sure this matters, but it opens up the possibility of combining the
access_ok check with the masking without any branches at all.

Also, why are you doing mask &= _ptr; mask instead of just
((typeof(ptr)) (_ptr & mask))?  or _ptr &= mask, for that matter?