[PATCH] kernel: sys: fix potential Spectre v1

[PATCH] kernel: sys: fix potential Spectre v1

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

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

[1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2

Cc: stable@vger.kernel.org
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 kernel/sys.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/sys.c b/kernel/sys.c
index 63ef036..78646e6 100644
--- 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];
 	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);
-- 
2.7.4

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

[Andrew Morton]

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

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

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

[Thomas Gleixner]

On Tue, 15 May 2018, Andrew Morton wrote:
> On Mon, 14 May 2018 22:00:38 -0500 "Gustavo A. R. Silva" <gustavo@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().

Thanks,

	tglx

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

[Dan Williams]

On Tue, May 15, 2018 at 3:29 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Tue, 15 May 2018, Andrew Morton wrote:
>> On Mon, 14 May 2018 22:00:38 -0500 "Gustavo A. R. Silva" <gustavo@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.

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

[Gustavo A. R. Silva]

On 05/15/2018 05:57 PM, Dan Williams wrote:
> On Tue, May 15, 2018 at 3:29 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> On Tue, 15 May 2018, Andrew Morton wrote:
>>> On Mon, 14 May 2018 22:00:38 -0500 "Gustavo A. R. Silva" <gustavo@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 */


And here is an example of its use:

diff --git a/drivers/media/i2c/tvp7002.c b/drivers/media/i2c/tvp7002.c
index 4599b7e..27b39c0 100644
--- a/drivers/media/i2c/tvp7002.c
+++ b/drivers/media/i2c/tvp7002.c
@@ -35,6 +35,8 @@
  #include <media/v4l2-ctrls.h>
  #include <media/v4l2-fwnode.h>

+#include <linux/nospec.h>
+
  #include "tvp7002_reg.h"

  MODULE_DESCRIPTION("TI TVP7002 Video and Graphics Digitizer driver");
@@ -784,7 +786,7 @@ static int tvp7002_enum_dv_timings(struct 
v4l2_subdev *sd,
                 return -EINVAL;

         /* Check requested format index is within range */
-       if (timings->index >= NUM_TIMINGS)
+       if (!sanitize_index_nospec(timings->index, NUM_TIMINGS))
                 return -EINVAL;

         timings->timings = tvp7002_timings[timings->index].timings;

This patter is very common. So, it may be a good idea to unify both 
bounds checking and the serialization of instructions into a single 
function.

What do you think?

Thanks
--
Gustavo

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 <tglx@linutronix.de> 
>> wrote:
>>> On Tue, 15 May 2018, Andrew Morton wrote:
>>>> On Mon, 14 May 2018 22:00:38 -0500 "Gustavo A. R. Silva" 
>>>> <gustavo@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

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

[Dan Williams]

On Fri, May 18, 2018 at 12:21 PM, Gustavo A. R. Silva
<gustavo@embeddedor.com> 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@linutronix.de>
>>> wrote:
>>>>
>>>> On Tue, 15 May 2018, Andrew Morton wrote:
>>>>>
>>>>> On Mon, 14 May 2018 22:00:38 -0500 "Gustavo A. R. Silva"
>>>>> <gustavo@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

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?

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

[Gustavo A. R. Silva]

On 05/18/2018 03:38 PM, Dan Williams wrote:
> On Fri, May 18, 2018 at 12:21 PM, Gustavo A. R. Silva
> <gustavo@embeddedor.com> 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@linutronix.de>
>>>> wrote:
>>>>>
>>>>> On Tue, 15 May 2018, Andrew Morton wrote:
>>>>>>
>>>>>> On Mon, 14 May 2018 22:00:38 -0500 "Gustavo A. R. Silva"
>>>>>> <gustavo@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
> 
> 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

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

[Gustavo A. R. Silva]

On 05/18/2018 03:44 PM, Gustavo A. R. Silva wrote:
>>>>
>>>
>>> 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.

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.

Thanks
--
Gustavo

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

[Dan Williams]

On Fri, May 18, 2018 at 2:27 PM, Gustavo A. R. Silva
<gustavo@embeddedor.com> wrote:
>
>
> On 05/18/2018 03:44 PM, Gustavo A. R. Silva wrote:
>>>>>
>>>>>
>>>>
>>>> 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.
>
>
> 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.

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.

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

[Gustavo A. R. Silva]

On 05/18/2018 04:45 PM, Dan Williams wrote:
> On Fri, May 18, 2018 at 2:27 PM, Gustavo A. R. Silva
> <gustavo@embeddedor.com> wrote:
>>
>>
>> On 05/18/2018 03:44 PM, Gustavo A. R. Silva wrote:
>>>>>>
>>>>>>
>>>>>
>>>>> 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.
>>
>>
>> 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.
> 
> 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];

	[...]
}

Thanks
--
Gustavo

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

[Dan Williams]

On Fri, May 18, 2018 at 3:01 PM, Gustavo A. R. Silva
<gustavo@embeddedor.com> wrote:
>
>
> On 05/18/2018 04:45 PM, Dan Williams wrote:
>>
>> On Fri, May 18, 2018 at 2:27 PM, Gustavo A. R. Silva
>> <gustavo@embeddedor.com> wrote:
>>>
>>>
>>>
>>> On 05/18/2018 03:44 PM, Gustavo A. R. Silva wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> 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.
>>>
>>>
>>>
>>> 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.
>>
>>
>> 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?:

Correct, broken:

>
> bool foo(int x)
> {
>         if(x >= MAX)
>                 return false;

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

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

>         return true;
> }
>
> int vulnerable(int x)
> {
>         if(!foo(x))
>                 return -1;

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

>
>         temp = array[x];

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

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

[Gustavo A. R. Silva]

On 05/18/2018 05:08 PM, Dan Williams wrote:
>>
>> 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:
> 
>>
>> bool foo(int x)
>> {
>>          if(x >= MAX)
>>                  return false;
> 
> Under speculation we may not return here when x is greater than max.
> 
>>          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.
> 
>>          return true;
>> }
>>
>> int vulnerable(int x)
>> {
>>          if(!foo(x))
>>                  return -1;
> 
> cpu might speculate that this branch is not taken...
> 
>>
>>          temp = array[x];
> 
> ...so x had better be bounded here, otherwise Spectre.
> 

I got it.

I appreciate the feedback.

Thanks, Dan.
--
Gustavo

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

[Gustavo A. R. Silva]

On 05/18/2018 03:44 PM, Gustavo A. R. Silva wrote:
>>>
>>> #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.
> 

Dan,

What do you think about this first draft:

diff --git a/include/linux/nospec.h b/include/linux/nospec.h
index e791ebc..6154183 100644
--- a/include/linux/nospec.h
+++ b/include/linux/nospec.h
@@ -55,4 +55,16 @@ static inline unsigned long 
array_index_mask_nospec(unsigned long index,
                                                                        \
         (typeof(_i)) (_i & _mask);                                     \
  })
+
+#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);           \
+})
  #endif /* _LINUX_NOSPEC_H */

Thanks
--
Gustavo

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

[Gustavo A. R. Silva]

On 05/20/2018 07:50 PM, Gustavo A. R. Silva wrote:
> 
> 
> On 05/18/2018 03:44 PM, Gustavo A. R. Silva wrote:
>>>>
>>>> #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.
>>
> 
> Dan,
> 
> What do you think about this first draft:
> 
> diff --git a/include/linux/nospec.h b/include/linux/nospec.h
> index e791ebc..6154183 100644
> --- a/include/linux/nospec.h
> +++ b/include/linux/nospec.h
> @@ -55,4 +55,16 @@ static inline unsigned long 
> array_index_mask_nospec(unsigned long index,
>                                                                         \
>          (typeof(_i)) (_i & _mask);                                     \
>   })
> +
> +#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);

> +})
>   #endif /* _LINUX_NOSPEC_H */
> 
> Thanks
> -- 
> Gustavo

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

[Dan Williams]

On Sun, May 20, 2018 at 7:00 PM, Gustavo A. R. Silva
<gustavo@embeddedor.com> wrote:
>
>
> On 05/20/2018 07:50 PM, Gustavo A. R. Silva wrote:
>>
>>
>>
>> On 05/18/2018 03:44 PM, Gustavo A. R. Silva wrote:
>>>>>
>>>>>
>>>>> #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.
>>>
>>
>> Dan,
>>
>> What do you think about this first draft:
>>
>> diff --git a/include/linux/nospec.h b/include/linux/nospec.h
>> index e791ebc..6154183 100644
>> --- a/include/linux/nospec.h
>> +++ b/include/linux/nospec.h
>> @@ -55,4 +55,16 @@ static inline unsigned long
>> array_index_mask_nospec(unsigned long index,
>>                                                                         \
>>          (typeof(_i)) (_i & _mask);                                     \
>>   })
>> +
>> +#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);
>

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

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

[Gustavo A. R. Silva]

On 05/22/2018 03:50 PM, Dan Williams wrote:
>>>
>>> Dan,
>>>
>>> What do you think about this first draft:
>>>
>>> diff --git a/include/linux/nospec.h b/include/linux/nospec.h
>>> index e791ebc..6154183 100644
>>> --- a/include/linux/nospec.h
>>> +++ b/include/linux/nospec.h
>>> @@ -55,4 +55,16 @@ static inline unsigned long
>>> array_index_mask_nospec(unsigned long index,
>>>                                                                          \
>>>           (typeof(_i)) (_i & _mask);                                     \
>>>    })
>>> +
>>> +#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);
>>
> 
> Let's not use ternary conditionals at all to make this more readable.
> 

OK. How about this:

diff --git a/include/linux/nospec.h b/include/linux/nospec.h
index e791ebc..498995b 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 = 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)                                              \
+               ret = false;                                           \
+                                                                      \
+       *ptr = array_index_nospec(*ptr, _s);                           \
+                                                                      \
+       ret;                                                           \
+})
  #endif /* _LINUX_NOSPEC_H */

Thanks
--
Gustavo

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

[Dan Williams]

On Tue, May 22, 2018 at 10:03 PM, Gustavo A. R. Silva
<gustavo@embeddedor.com> wrote:
>
>
> On 05/22/2018 03:50 PM, Dan Williams wrote:
>>>>
>>>>
>>>> Dan,
>>>>
>>>> What do you think about this first draft:
>>>>
>>>> diff --git a/include/linux/nospec.h b/include/linux/nospec.h
>>>> index e791ebc..6154183 100644
>>>> --- a/include/linux/nospec.h
>>>> +++ b/include/linux/nospec.h
>>>> @@ -55,4 +55,16 @@ static inline unsigned long
>>>> array_index_mask_nospec(unsigned long index,
>>>>
>>>> \
>>>>           (typeof(_i)) (_i & _mask);
>>>> \
>>>>    })
>>>> +
>>>> +#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);
>>>
>>
>> Let's not use ternary conditionals at all to make this more readable.
>>
>
> OK. How about this:
>
> diff --git a/include/linux/nospec.h b/include/linux/nospec.h
> index e791ebc..498995b 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 = 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)                                              \
> +               ret = false;                                           \
> +                                                                      \
> +       *ptr = array_index_nospec(*ptr, _s);                           \
> +                                                                      \
> +       ret;                                                           \
>
> +})
>  #endif /* _LINUX_NOSPEC_H */

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

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

[Gustavo A. R. Silva]

On 05/23/2018 12:15 AM, Dan Williams wrote:
>>
>> OK. How about this:
>>
>> diff --git a/include/linux/nospec.h b/include/linux/nospec.h
>> index e791ebc..498995b 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 = 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)                                              \

I'll change the line above by this one:

if (*ptr >= _s)

>> +               ret = false;                                           \
>> +                                                                      \
>> +       *ptr = array_index_nospec(*ptr, _s);                           \
>> +                                                                      \
>> +       ret;                                                           \
>>
>> +})
>>   #endif /* _LINUX_NOSPEC_H */
> 
> Assuming the assembly generation is comparable with the open coded
> version, this looks ok to me.
> 

OK. I'll send a proper patch tomorrow morning.

Thanks for the feedback, Dan.
--
Gustavo

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

[Peter Zijlstra]

Sorry for being late to the party..

On Wed, May 23, 2018 at 12:03:57AM -0500, Gustavo A. R. Silva wrote:

> +#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));                       \
> +                                                                      \
> +       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?

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

[Dan Williams]

On Wed, May 23, 2018 at 2:08 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>
> Sorry for being late to the party..
>
> On Wed, May 23, 2018 at 12:03:57AM -0500, Gustavo A. R. Silva wrote:
>
>> +#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));                       \
>> +                                                                      \
>> +       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?

Yes, that looks even better to me.

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

[Mark Rutland]

On Wed, May 23, 2018 at 11:08:40AM +0200, Peter Zijlstra wrote:
> 
> Sorry for being late to the party..

Likewise!

> On Wed, May 23, 2018 at 12:03:57AM -0500, Gustavo A. R. Silva wrote:
> > +#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));                       \
> > +                                                                      \
> > +       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?

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

Thanks,
Mark.

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

[Dan Williams]

On Wed, May 23, 2018 at 8:07 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, May 23, 2018 at 11:08:40AM +0200, Peter Zijlstra wrote:
>>
>> Sorry for being late to the party..
>
> Likewise!
>
>> On Wed, May 23, 2018 at 12:03:57AM -0500, Gustavo A. R. Silva wrote:
>> > +#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));                       \
>> > +                                                                      \
>> > +       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?
>
> 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. :/

This would be interesting, because my operating assumption is that the
compiler will not play these games over inline asm, i.e. the index
will always be modified before use in all cases.

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

[Peter Zijlstra]

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 was assuming the compiler would not do that, that's pretty stupid
code-gen. But you're right in calling that out, because I think it's
entirely in it's right to do that :/

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

*groan*...

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

[Mark Rutland]

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.

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

[Gustavo A. R. Silva]

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