Re: [PATCH] net: core: Fix Spectre v1 vulnerability

["Gustavo A. R. Silva" <gustavo@embeddedor.com>]

Alexei,

On 12/22/18 9:37 PM, Gustavo A. R. Silva wrote:
> 
> 
> On 12/22/18 9:00 PM, Alexei Starovoitov wrote:
>> On Sat, Dec 22, 2018 at 08:53:40PM -0600, Gustavo A. R. Silva wrote:
>>> Hi,
>>>
>>> On 12/22/18 8:40 PM, David Miller wrote:
>>>> From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
>>>> Date: Sat, 22 Dec 2018 15:59:54 -0800
>>>>
>>>>> On Sat, Dec 22, 2018 at 03:07:22PM -0800, David Miller wrote:
>>>>>> From: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
>>>>>> Date: Fri, 21 Dec 2018 14:49:01 -0600
>>>>>>
>>>>>>> flen is indirectly 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:
>>>>>>>
>>>>>>> net/core/filter.c:1101 bpf_check_classic() warn: potential 
>>>>>>> spectre issue 'filter' [w]
>>>>>>>
>>>>>>> Fix this by sanitizing flen before using it to index filter at 
>>>>>>> line 1101:
>>>>>>>
>>>>>>>     switch (filter[flen - 1].code) {
>>>>>>>
>>>>>>> and through pc at line 1040:
>>>>>>>
>>>>>>>     const struct sock_filter *ftest = &filter[pc];
>>>>>>>
>>>>>>> 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
>>>>>>>
>>>>>>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>>>>>>
>>>>>> BPF folks, I'll take this directly.
>>>>>>
>>>>>> Applied and queued up for -stable, thanks.
>>>>>
>>>>> hmm. what was the rush?
>>>>> I think it is unnecessary change.
>>>>> Though fp is passed initially from user space
>>>>> it's copied into kernel struct first.
>>>>> There is no way user space can force kernel to mispredict
>>>>> branch in for (pc = 0; pc < flen; pc++) loop.
>>> The following piece of code is the one that can be mispredicted, not 
>>> the for
>>> loop:
>>>
>>> 1013         if (flen == 0 || flen > BPF_MAXINSNS)
>>> 1014                 return false;
>>>
>>> Instead of calling array_index_nospec() inside bpf_check_basics_ok(), I
>>> decided to place the call close to the code that could be 
>>> compromised. This
>>> is when accessing filter[].
>>
>> Why do you think it can be mispredicted?
>>
> 
> Beause fprog->len comes from user space:
> 
> bpf_prog_create_from_user() -> bpf_check_basics_ok()
> 
>> I've looked at your other patch for nfc_sock_create() where you're 
>> adding:
>> + proto = array_index_nospec(proto, NFC_SOCKPROTO_MAX);
>>
>> 'proto' is the value passed in _register_ into system call.
>> There is no need to wrap it with array_index_nospec().
>> It's not a load from memory and user space cannot make it slow.
>> Slow load is a necessary attribute to trigger speculative execution
>> into mispredicted branch.
>>

I think I know where the confusion is coming from. The load you talk 
about is the firs load needed in the following code:

if (x < array1_size) {
   v = array2[array1[x]*256]
}

This is array[x]

In this case, that first load needed would be:

1101: switch (filter[flen - 1].code) {

or

1040: const struct sock_filter *ftest = &filter[pc];

The policy has been to kill the speculation on that first load and not 
worry if it can be completed with a dependent load/store. As mentioned 
in the commit log.

Thanks
--
Gustavo