linux-kernel - Re: [PATCH] kernel: sys: fix potential Spectre v1

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] [day] [month] [year] [list]

Message-ID: <5684a277-464d-dbad-8e3a-d766e66626ec@embeddedor.com>
Date:   Fri, 25 May 2018 13:11:54 -0500
From:   "Gustavo A. R. Silva" <gustavo@...eddedor.com>
To:     Mark Rutland <mark.rutland@....com>,
        Peter Zijlstra <peterz@...radead.org>
Cc:     Dan Williams <dan.j.williams@...el.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>
Subject: Re: [PATCH] kernel: sys: fix potential Spectre v1



On 05/23/2018 11:31 AM, Mark Rutland wrote:
> On Wed, May 23, 2018 at 04:07:37PM +0100, Mark Rutland wrote:
>> I think that either way, we have a potential problem if the compiler
>> generates a branch dependent on the result of validate_index_nospec().
>>
>> In that case, we could end up with codegen approximating:
>>
>> 	bool safe = false;
>>
>> 	if (idx < bound) {
>> 		idx = array_index_nospec(idx, bound);
>> 		safe = true;
>> 	}
>>
>> 	// this branch can be mispredicted
>> 	if (safe) {
>> 		foo = array[idx];
>> 	}
>>
>> ... and thus we lose the nospec protection.
> 
> I see GCC do this at -O0, but so far I haven't tricked it into doing
> this at -O1 or above.
> 
> Regardless, I worry this is fragile -- GCC *can* generate code as per
> the above, even if it's unlikely to.
> 
>> I also suspect that compiler transformations mean that this might
>> already be the case for patterns like:
>>
>> 	if (idx < bound)  {
>> 		safe_idx = array_index_nospec(idx, bound)];
>> 		...
>> 		foo = array[safe_idx];
>> 	}
>>
>> ... if the compiler can transform that to something like:
>>
>> 	if (idx < bound) {
>> 		idx = array_index_nospec(idx, bound);
>> 	}
>>
>> 	// can be mispredicted
>> 	if (idx < bound) {
>> 		foo = array[idx];
>> 	}
>>
>> ... which I think a compiler might be capable of, depending on the rest
>> of the function body (e.g. if there's a common portion shared with the
>> else case).
>>
>> I'll see if I can trigger that in a test case. :/
> 
> No luck so far, but I'll keeep fighting...
> 
> GCC will happily pull a common suffix after the branch, e.g.
> 
> 	if (cond) {
> 		foo();
> 		bar();
> 	} else {
> 		bar();
> 	}
> 
> .. goes to:
> 
> 	if (cond)
> 		foo()
> 
> 	bar();
> 
> ... but I can't convince it to pull a common prefix before the branch.
> 
> Mark.
> 
I will send the following patch once Dan's [1] has been applied upstream.

diff --git a/include/linux/nospec.h b/include/linux/nospec.h
index e791ebc..2a1ab2e 100644
--- a/include/linux/nospec.h
+++ b/include/linux/nospec.h
@@ -55,4 +55,21 @@ static inline unsigned long 
array_index_mask_nospec(unsigned long index,
                                                                        \
         (typeof(_i)) (_i & _mask);                                     \
  })
+
+#define validate_index_nospec(index, size)                            \
+({                                                                    \
+       bool ret = false;                                              \
+       typeof(index) *ptr = &(index);                                 \
+       typeof(size) _s = (size);                                      \
+                                                                      \
+       BUILD_BUG_ON(sizeof(*ptr) > sizeof(long));                     \
+       BUILD_BUG_ON(sizeof(_s) > sizeof(long));                       \
+                                                                      \
+       if (*ptr < _s) {                                               \
+               *ptr = array_index_nospec(*ptr, _s);                   \
+               ret = true;                                            \
+       }                                                              \
+                                                                      \
+       ret;                                                           \
+})

[1] https://marc.info/?l=linux-kernel&m=152726947109104&w=2

Thank you, Dan, Peter and Mark for your feedback.
--
Gustavo