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
