lkml.org 
[lkml]   [2018]   [Dec]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] net: core: Fix Spectre v1 vulnerability
From
Date
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

\
 
 \ /
  Last update: 2022-09-17 16:05    [W:0.078 / U:1.792 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site