lkml.org 
[lkml]   [2018]   [May]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH] kernel: sys: fix potential Spectre v1
From
Date


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

\
 
 \ /
  Last update: 2018-05-25 20:12    [W:0.057 / U:1.372 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site