kernel: sys: fix potential Spectre v1

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].

kernel/sys.c +3 -0

@@ -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

kernel/sys.c +1 -0

 	task_unlock(current->group_leader);

@@ -1470,6 +1474,7 @@ COMPAT_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);
 	r = current->signal->rlim[resource];
 	task_unlock(current->group_leader);

Quoted 10 lines · On Mon, 14 May 2018 22:00:38 -0500 "Gustavo A. R. Silva"…

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)

Quoted 6 lines

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 ;)

Quoted 16 lines

kernel/sys.c +3 -0

@@ -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;
 

kernel/sys.c SYSCALL_DEFINE2(old_getrlimit, unsigned int, resource, · line 1459

Quoted 3 lines

kernel/sys.c +1 -0

+	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?

Quoted 37 lines · Andrew Morton

Quoted 10 lines · On Mon, 14 May 2018 22:00:38 -0500 "Gustavo A. R. Silva"…

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)

Quoted 6 lines

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 ;)

Quoted 16 lines

kernel/sys.c +3 -0

@@ -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;
 

Quoted 6 lines

kernel/sys.c SYSCALL_DEFINE2(old_getrlimit, unsigned int, resource, · line 1459

Quoted 3 lines

kernel/sys.c +1 -0

+   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().

Quoted 45 lines · Thomas Gleixner

Quoted 43 lines · Andrew Morton

Quoted 10 lines · On Mon, 14 May 2018 22:00:38 -0500 "Gustavo A. R. Silva"…

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)

Quoted 6 lines

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 ;)

Quoted 13 lines

kernel/sys.c +3 -0

@@ -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

kernel/sys.c SYSCALL_DEFINE2(old_getrlimit, unsigned int, resource, · line 1459

Quoted 6 lines

kernel/sys.c +1 -0

@@ -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?

Quoted 3 lines

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.

Quoted 47 lines · Dan Williams

Quoted 46 lines · Thomas Gleixner

Quoted 44 lines · Andrew Morton

Quoted 10 lines · On Mon, 14 May 2018 22:00:38 -0500 "Gustavo A. R. Silva"…

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)

Quoted 6 lines

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 ;)

Quoted 13 lines

kernel/sys.c +3 -0

@@ -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

kernel/sys.c SYSCALL_DEFINE2(old_getrlimit, unsigned int, · line 1460

Quoted 7 lines

kernel/sys.c +1 -0

@@ -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?

Quoted 8 lines

Quoted 3 lines

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:

include/linux/nospec.h +13 -0

index e791ebc..81e9a77 100644
@@ -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 */

Quoted 97 lines · Gustavo A. R. Silva

Quoted 62 lines · Dan Williams

Quoted 53 lines · Thomas Gleixner

Quoted 42 lines · Andrew Morton

Quoted 11 lines · On Mon, 14 May 2018 22:00:38 -0500 "Gustavo A. R. Silva"…

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)

Quoted 6 lines

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 ;)

Quoted 17 lines

kernel/sys.c +3 -0

@@ -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;
 

Quoted 7 lines

kernel/sys.c SYSCALL_DEFINE2(old_getrlimit, unsigned int, · line 1460

Quoted 3 lines

kernel/sys.c +1 -0

+   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?

Quoted 4 lines

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().

Quoted 4 lines

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:

include/linux/nospec.h +13 -0

index e791ebc..81e9a77 100644
@@ -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

Quoted 2 lines

  #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

Quoted 113 lines · Gustavo A. R. Silva

Quoted 97 lines · Gustavo A. R. Silva

Quoted 62 lines · Dan Williams

Quoted 53 lines · Thomas Gleixner

Quoted 42 lines · Andrew Morton

Quoted 11 lines · On Mon, 14 May 2018 22:00:38 -0500 "Gustavo A. R. Silva"…

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)

Quoted 6 lines

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 ;)

Quoted 17 lines

kernel/sys.c +3 -0

@@ -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;
 

Quoted 7 lines

kernel/sys.c SYSCALL_DEFINE2(old_getrlimit, unsigned int, · line 1460

Quoted 3 lines

kernel/sys.c +1 -0

+   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?

Quoted 4 lines

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().

Quoted 4 lines

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:

include/linux/nospec.h +13 -0

index e791ebc..81e9a77 100644
@@ -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

Quoted 2 lines

   #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);

Quoted 3 lines

         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.

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?

Quoted 12 lines

Quoted 12 lines

Quoted 1 line

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);

Quoted 9 lines

Quoted 3 lines

         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.

Quoted 5 lines

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.

Quoted 33 lines · Gustavo A. R. Silva

Quoted 19 lines

Quoted 9 lines

Quoted 2 lines

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)

Quoted 8 lines

{
         if (*index >= size)
                 return false;
         *index = array_index_nospec(*index, size);

         return true;
}

#endif

Quoted 5 lines

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.

Quoted 5 lines

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?

Quoted 4 lines

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.

BTW, I'm analyzing other cases, like the following:

bool foo(int x)

{
         if(!validate_index_nospec(&x))
                 return false;

         [...]

         return true;
}

int vulnerable(int x)

{
         if (!foo(x))
                 return -1;

         temp = array[x];

         [...]

}

Basically my doubt is how deep this barrier can be placed into the call chain in order to continue working.

Quoted 63 lines · Gustavo A. R. Silva

Quoted 33 lines · Gustavo A. R. Silva

Quoted 19 lines

Quoted 9 lines

Quoted 2 lines

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)

Quoted 8 lines

{
          if (*index >= size)
                  return false;
          *index = array_index_nospec(*index, size);

          return true;
}

#endif

Quoted 5 lines

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.

Quoted 5 lines

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?

Quoted 4 lines

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.

BTW, I'm analyzing other cases, like the following:

bool foo(int x)

{
          if(!validate_index_nospec(&x))
                  return false;

          [...]

          return true;
}

int vulnerable(int x)

{
          if (!foo(x))
                  return -1;

          temp = array[x];

          [...]

Quoted 4 lines

};

Basically my doubt is how deep this barrier can be placed into the call chain in order to continue working.

This is broken you would need to pass the address of x into foo() otherwise there may be speculation on the return value of foo.

Quoted 76 lines · Dan Williams

Quoted 71 lines · Gustavo A. R. Silva

Quoted 35 lines · Gustavo A. R. Silva

Quoted 21 lines

Quoted 10 lines

Quoted 3 lines

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)

Quoted 8 lines

{
          if (*index >= size)
                  return false;
          *index = array_index_nospec(*index, size);

          return true;
}

#endif

Quoted 5 lines

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.

Quoted 5 lines

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?

Quoted 4 lines

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.

BTW, I'm analyzing other cases, like the following:

bool foo(int x)

{
          if(!validate_index_nospec(&x))
                  return false;

          [...]

          return true;
}

int vulnerable(int x)

{
          if (!foo(x))
                  return -1;

          temp = array[x];

          [...]

};

Basically my doubt is how deep this barrier can be placed into the call chain in order to continue working.

Quoted 3 lines

This is broken you would need to pass the address of x into foo() otherwise there may be speculation on the return value of foo.

Oh I see now. Just to double check, then something like the following would be broken too, because is basically the same as the code above, and well, it doesn't make much sense to store the value returned by macro array_index_nospec into x, correct?:

bool foo(int x)

{
        if(x >= MAX)
                return false;

        x = array_index_nospec(x, MAX);

        return true;
}

int vulnerable(int x)

{
        if(!foo(x))
                return -1;

        temp = array[x];

Quoted 5 lines

Oh I see now. Just to double check, then something like the following would be broken too, because is basically the same as the code above, and well, it doesn't make much sense to store the value returned by macro array_index_nospec into x, correct?:

Correct, broken:

Quoted 5 lines

bool foo(int x)

{
         if(x >= MAX)
                 return false;

Under speculation we may not return here when x is greater than max.

Quoted 1 line

         x = array_index_nospec(x, MAX);

x is now sanitized under speculation to zero, but the compiler would likely just throw this away because nothing consumes it.

Quoted 7 lines

         return true;
}

int vulnerable(int x)

{
         if(!foo(x))
                 return -1;

cpu might speculate that this branch is not taken...

Quoted 2 lines

         temp = array[x];

...so x had better be bounded here, otherwise Spectre.

Quoted 9 lines

Quoted 9 lines

#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);

Quoted 9 lines

Quoted 3 lines

         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.

Quoted 5 lines

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.

Quoted 21 lines · Gustavo A. R. Silva

Quoted 9 lines

Quoted 9 lines

#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);

Quoted 9 lines

Quoted 3 lines

         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.

Quoted 8 lines

Quoted 5 lines

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.

Dan,

What do you think about this first draft:

include/linux/nospec.h +1 -0

index e791ebc..6154183 100644
@@ -55,4 +55,16 @@ static inline unsigned long 
 array_index_mask_nospec(unsigned long index,
                                                                        \
         (typeof(_i)) (_i & _mask);                                     \
  })
+

include/linux/nospec.h +1 -0

+})

 #endif /* _LINUX_NOSPEC_H */

Thanks

Signature · 1 line

Gustavo

Quoted 51 lines · Gustavo A. R. Silva

Quoted 23 lines · Gustavo A. R. Silva

Quoted 15 lines

Quoted 5 lines

#ifndef sanitize_index_nospec
inline bool sanitize_index_nospec(unsigned long *index,
unsigned long size)

Quoted 8 lines

{
         if (*index >= size)
                 return false;
         *index = array_index_nospec(*index, size);

         return true;
}

#endif

Quoted 5 lines

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.

Quoted 9 lines

Quoted 5 lines

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.

Dan,

What do you think about this first draft:

include/linux/nospec.h +1 -0

index e791ebc..6154183 100644
@@ -55,4 +55,16 @@ static inline unsigned long
 array_index_mask_nospec(unsigned long index,
                                                                        \
         (typeof(_i)) (_i & _mask);                                     \
  })
+

include/linux/nospec.h static inline unsigned long · line 69

Quoted 10 lines

include/linux/nospec.h +10 -0

+#define validate_index_nospec(index, size)                            \
+({                                                                    \
+       typeof(index) *ptr = &(index);                                 \
+       typeof(size) _s = (size);                                      \
+                                                                      \
+       BUILD_BUG_ON(sizeof(*ptr) > sizeof(long));                     \
+       BUILD_BUG_ON(sizeof(_s) > sizeof(long));                       \
+                                                                      \
+       *ptr >= _s ? false :                                           \
+       (*ptr = array_index_nospec(*ptr, _s) ? true : true);           \

This actually should be:

((*ptr = array_index_nospec(*ptr, _s)) ? true : true);

Quoted 29 lines

Quoted 15 lines

Dan,

What do you think about this first draft:

include/linux/nospec.h +1 -0

index e791ebc..6154183 100644
@@ -55,4 +55,16 @@ static inline unsigned long
 array_index_mask_nospec(unsigned long index,
                                                                         \
          (typeof(_i)) (_i & _mask);                                     \
   })
+

include/linux/nospec.h static inline unsigned long · line 69

Quoted 10 lines

include/linux/nospec.h +10 -0

+#define validate_index_nospec(index, size)                            \
+({                                                                    \
+       typeof(index) *ptr = &(index);                                 \
+       typeof(size) _s = (size);                                      \
+                                                                      \
+       BUILD_BUG_ON(sizeof(*ptr) > sizeof(long));                     \
+       BUILD_BUG_ON(sizeof(_s) > sizeof(long));                       \
+                                                                      \
+       *ptr >= _s ? false :                                           \
+       (*ptr = array_index_nospec(*ptr, _s) ? true : true);           \

This actually should be:

Quoted 2 lines

((*ptr = array_index_nospec(*ptr, _s)) ? true : true);

Let's not use ternary conditionals at all to make this more readable.

Quoted 33 lines · Dan Williams

Quoted 33 lines

Quoted 18 lines

Dan,

What do you think about this first draft:

include/linux/nospec.h +0 -0

index e791ebc..6154183 100644
@@ -55,4 +55,16 @@ static inline unsigned long
 array_index_mask_nospec(unsigned long index,
 
 \
          (typeof(_i)) (_i & _mask);

\
   })

include/linux/nospec.h +1 -0

+

include/linux/nospec.h static inline unsigned long array_index_mask_nospec(unsigned long index,

Quoted 10 lines

include/linux/nospec.h +10 -0

+#define validate_index_nospec(index, size)                            \
+({                                                                    \
+       typeof(index) *ptr = &(index);                                 \
+       typeof(size) _s = (size);                                      \
+                                                                      \
+       BUILD_BUG_ON(sizeof(*ptr) > sizeof(long));                     \
+       BUILD_BUG_ON(sizeof(_s) > sizeof(long));                       \
+                                                                      \
+       *ptr >= _s ? false :                                           \
+       (*ptr = array_index_nospec(*ptr, _s) ? true : true);           \

This actually should be:

Quoted 5 lines

Quoted 2 lines

((*ptr = array_index_nospec(*ptr, _s)) ? true : true);

Let's not use ternary conditionals at all to make this more readable.

OK. How about this:

include/linux/nospec.h +9 -0

index e791ebc..498995b 100644
@@ -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 = true;                                               \
+       typeof(index) *ptr = &(index);                                 \
+       typeof(size) _s = (size);                                      \
+                                                                      \
+       BUILD_BUG_ON(sizeof(*ptr) > sizeof(long));                     \
+       BUILD_BUG_ON(sizeof(_s) > sizeof(long));                       \

Quoted 15 lines

OK. How about this:

include/linux/nospec.h +3 -0

index e791ebc..498995b 100644
@@ -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)                            \
+({                                                                    \

include/linux/nospec.h static inline unsigned long array_index_mask_nospec(unsigned long index, · line 68

Quoted 8 lines

include/linux/nospec.h +8 -0

+       bool ret = true;                                               \
+       typeof(index) *ptr = &(index);                                 \
+       typeof(size) _s = (size);                                      \
+                                                                      \
+       BUILD_BUG_ON(sizeof(*ptr) > sizeof(long));                     \
+       BUILD_BUG_ON(sizeof(_s) > sizeof(long));                       \
+                                                                      \
+       if (*ptr >= size)                                              \

Quoted 6 lines

include/linux/nospec.h +5 -0

+               ret = false;                                           \
+                                                                      \
+       *ptr = array_index_nospec(*ptr, _s);                           \
+                                                                      \
+       ret;                                                           \

Quoted 2 lines

include/linux/nospec.h +1 -0

+})

  #endif /* _LINUX_NOSPEC_H */

Assuming the assembly generation is comparable with the open coded version, this looks ok to me.

include/linux/nospec.h +6 -0

+#define validate_index_nospec(index, size)                            \
+({                                                                    \
+       bool ret = true;                                               \
+       typeof(index) *ptr = &(index);                                 \
+       typeof(size) _s = (size);                                      \
+                                                                      \

Sorry for being late to the party..

Quoted 6 lines · Gustavo A. R. Silva

include/linux/nospec.h +6 -0

+#define validate_index_nospec(index, size)                            \
+({                                                                    \
+       bool ret = true;                                               \
+       typeof(index) *ptr = &(index);                                 \
+       typeof(size) _s = (size);                                      \
+                                                                      \

include/linux/nospec.h static inline unsigned long array_index_mask_nospec(unsigned long index, · line 74

Quoted 10 lines

include/linux/nospec.h +10 -0

+       BUILD_BUG_ON(sizeof(*ptr) > sizeof(long));                     \
+       BUILD_BUG_ON(sizeof(_s) > sizeof(long));                       \
+                                                                      \
+       if (*ptr >= size)                                              \
+               ret = false;                                           \
+                                                                      \
+       *ptr = array_index_nospec(*ptr, _s);                           \
+                                                                      \
+       ret;                                                           \
+})

Would not something like:

        bool ret = false;

        ....

        if (*ptr < _s) {
                *ptr = array_index_nospec(*ptr, _s);
                ret = true;
        }

        ret;

be more obvious?

Sorry for being late to the party..

Quoted 6 lines

include/linux/nospec.h +6 -0

+#define validate_index_nospec(index, size)                            \
+({                                                                    \
+       bool ret = true;                                               \
+       typeof(index) *ptr = &(index);                                 \
+       typeof(size) _s = (size);                                      \
+                                                                      \

include/linux/nospec.h static inline unsigned long array_index_mask_nospec(unsigned long index, · line 74

Quoted 10 lines

include/linux/nospec.h +10 -0

+       BUILD_BUG_ON(sizeof(*ptr) > sizeof(long));                     \
+       BUILD_BUG_ON(sizeof(_s) > sizeof(long));                       \
+                                                                      \
+       if (*ptr >= size)                                              \
+               ret = false;                                           \
+                                                                      \
+       *ptr = array_index_nospec(*ptr, _s);                           \
+                                                                      \
+       ret;                                                           \
+})

Would not something like:

	bool ret = false;

	....

	if (*ptr < _s) {
		*ptr = array_index_nospec(*ptr, _s);
		ret = true;
	}

	ret;

be more obvious?

Quoted 2 lines · Peter Zijlstra

Sorry for being late to the party..

Likewise!

Quoted 31 lines · Gustavo A. R. Silva

Quoted 6 lines

include/linux/nospec.h +6 -0

+#define validate_index_nospec(index, size)                            \
+({                                                                    \
+       bool ret = true;                                               \
+       typeof(index) *ptr = &(index);                                 \
+       typeof(size) _s = (size);                                      \
+                                                                      \

include/linux/nospec.h static inline unsigned long array_index_mask_nospec(unsigned long index, · line 74

Quoted 10 lines

include/linux/nospec.h +10 -0

+       BUILD_BUG_ON(sizeof(*ptr) > sizeof(long));                     \
+       BUILD_BUG_ON(sizeof(_s) > sizeof(long));                       \
+                                                                      \
+       if (*ptr >= size)                                              \
+               ret = false;                                           \
+                                                                      \
+       *ptr = array_index_nospec(*ptr, _s);                           \
+                                                                      \
+       ret;                                                           \
+})

Would not something like:

      bool ret = false;

      ....

      if (*ptr < _s) {
              *ptr = array_index_nospec(*ptr, _s);
              ret = true;
      }

      ret;

Quoted 1 line

be more obvious?

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 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. :/

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 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. :/

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 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. :/

Quoted 17 lines · Mark Rutland

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];
	}

Quoted 1 line

... 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.

Quoted 20 lines

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];
	}

Quoted 5 lines

... 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.