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

From: Gustavo A. R. Silva
Date: Fri May 25 2018 - 14:12:03 EST




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