[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 22 Jan 2018 10:52:54 -0800
From: Dan Williams <dan.j.williams@...el.com>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Russell King - ARM Linux <linux@...linux.org.uk>, Thomas Gleixner <tglx@...utronix.de>,
linux-arch <linux-arch@...r.kernel.org>,
Kernel Hardening <kernel-hardening@...ts.openwall.com>, Peter Zijlstra <peterz@...radead.org>,
Catalin Marinas <catalin.marinas@....com>, X86 ML <x86@...nel.org>,
Will Deacon <will.deacon@....com>, Ingo Molnar <mingo@...hat.com>,
Greg KH <gregkh@...uxfoundation.org>, "H. Peter Anvin" <hpa@...or.com>,
Alan Cox <alan@...ux.intel.com>
Subject: Re: [PATCH v4.1 02/10] asm/nospec, array_ptr: sanitize speculative
array de-references
On Mon, Jan 22, 2018 at 10:37 AM, Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
> On Mon, Jan 22, 2018 at 10:07 AM, Dan Williams <dan.j.williams@...el.com> wrote:
>> On Sun, Jan 21, 2018 at 2:40 AM, Russell King - ARM Linux
>> <linux@...linux.org.uk> wrote:
>>>
>>> I'd suggest changing the description to something like:
>>>
>>> * If 0 <= idx < size, return a mask of ~0UL, otherwise return zero.
>
> Yes. HOWEVER.
>
> We should make it clear that the signedness of the comparisons is
> undefined, and that 'size' should always be positive for the above to
> be well-specified.
>
> Why?
>
> Because one valid implementation of the above is:
>
> idx U< size ? ~0UL : 0
>
> as a single unsigned comparison (I'm using "U<" as a "unsigned less than").
>
> But an equally valid implementation of it might be
>
> idx S>= 0 && idx S< size ? ~0UL : 0
>
> which does two signed comparisons instead.
>
> And they are not exactly the same. They are the same _IFF_ 'size' is
> positive in 'long'. But if size is an unsigned value so big as to be
> negative in 'long', they give different results for 'idx'.
>
> In fact, the ALU-only version actually did a third version of the
> above that only uses "signed comparison against zero":
>
> idx S> 0 && (size - idx - 1) S>= 0 ? ~0UL : 0
>
> which again is equivalent only when 'size' is positive in 'long'.
>
> Note that 'idx' itself can have any range (that's kind of the _point_,
> after all: checking that idx is in some range). But the logic only
> works for a positive array size.
>
> Which honestly is not really a limitation, but it's worth spelling out, I think.
>
> In particular, if you have a user pointer, and a 3G:1G split in
> user:kernel address space, you camn *not* do something like
>
> uptr = array_ptr(NULL, userptr, TASK_SIZE);
>
> to get a sanitized user pointer, because TASK_SIZE is not positive in 'long'.
>
> Hmm?
We could at least have a BUILD_BUG_ON when 'size' is a
builtin_contstant and greater than LONG_MAX. Alternatively we could
require archs to provide the equivalent of the x86 array_ptr_mask()
that does not have the LONG_MAX limitation?
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.
