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

[Gustavo A. R. Silva]

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 <t...@linutronix.de> wrote:

On Tue, 15 May 2018, Andrew Morton wrote:

On Mon, 14 May 2018 22:00:38 -0500 "Gustavo A. R. Silva" <gust...@embeddedor.com> 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

Thanks
--
Gustavo