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.