Re: [PATCH] kernel: sys: fix potential Spectre v1
From: Gustavo A. R. Silva
Date: Fri May 18 2018 - 16:09:23 EST
On 05/18/2018 03:38 PM, Dan Williams wrote:
On Fri, May 18, 2018 at 12:21 PM, Gustavo A. R. Silva
<gustavo@xxxxxxxxxxxxxx> wrote:
On 05/18/2018 02:04 PM, Gustavo A. R. Silva wrote:
On 05/15/2018 05:57 PM, Dan Williams wrote:
On Tue, May 15, 2018 at 3:29 PM, Thomas Gleixner <tglx@xxxxxxxxxxxxx>
wrote:
On Tue, 15 May 2018, Andrew Morton wrote:
On Mon, 14 May 2018 22:00:38 -0500 "Gustavo A. R. Silva"
<gustavo@xxxxxxxxxxxxxx> wrote:
resource can be controlled by user-space, hence leading to a
potential exploitation of the Spectre variant 1 vulnerability.
This issue was detected with the help of Smatch:
kernel/sys.c:1474 __do_compat_sys_old_getrlimit() warn: potential
spectre issue 'get_current()->signal->rlim' (local cap)
kernel/sys.c:1455 __do_sys_old_getrlimit() warn: potential spectre
issue
'get_current()->signal->rlim' (local cap)
Fix this by sanitizing *resource* before using it to index
current->signal->rlim
Notice that given that speculation windows are large, the policy is
to kill the speculation on the first load and not worry if it can be
completed with a dependent load/store [1].
hm. Not my area, but I'm always willing to learn ;)
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -69,6 +69,9 @@
#include <asm/io.h>
#include <asm/unistd.h>
+/* Hardening for Spectre-v1 */
+#include <linux/nospec.h>
+
#include "uid16.h"
#ifndef SET_UNALIGN_CTL
@@ -1451,6 +1454,7 @@ SYSCALL_DEFINE2(old_getrlimit, unsigned int,
resource,
if (resource >= RLIM_NLIMITS)
return -EINVAL;
+ resource = array_index_nospec(resource, RLIM_NLIMITS);
task_lock(current->group_leader);
x = current->signal->rlim[resource];
Can the speculation proceed past the task_lock()? Or is the policy to
ignore such happy happenstances even if they are available?
Locks are not in the way of speculation. Speculation has almost no
limits
except serializing instructions. At least they respect the magic AND
limitation in array_index_nospec().
I'd say it another way, because they don't respect the magic AND, we
just make the result in the speculation path safe. So, it's controlled
speculation.
Dan,
What do you think about adding the following function to the nospec API:
diff --git a/include/linux/nospec.h b/include/linux/nospec.h
index e791ebc..81e9a77 100644
--- a/include/linux/nospec.h
+++ b/include/linux/nospec.h
@@ -55,4 +55,17 @@ static inline unsigned long
array_index_mask_nospec(unsigned long index,
\
(typeof(_i)) (_i & _mask); \
})
+
+
+#ifndef sanitize_index_nospec
+inline bool sanitize_index_nospec(unsigned long index,
+ unsigned long size)
+{
+ if (index >= size)
+ return false;
+ index = array_index_nospec(index, size);
+
+ return true;
+}
+#endif
#endif /* _LINUX_NOSPEC_H */
Oops, it seems I sent the wrong patch. The function would look like this:
#ifndef sanitize_index_nospec
inline bool sanitize_index_nospec(unsigned long *index,
unsigned long size)
{
if (*index >= size)
return false;
*index = array_index_nospec(*index, size);
return true;
}
#endif
I think this is fine in concept, we already do something similar in
mpls_label_ok(). Perhaps call it validate_index_nospec() since
validation is something that can fail, but sanitization in theory is
something that can always succeed.
OK. I got it.
However, the problem is the data type of the index. I expect you would
need to do this in a macro and use typeof() if you wanted this to be
generally useful, and also watch out for multiple usage of a macro
argument. Is it still worth it at that point?
Yeah. I think it is worth it. I'll work on this during the weekend and
send a proper patch for review.
Thanks for the feedback.
--
Gustavo