[PATCH] net: core: Fix Spectre v1 vulnerability

[PATCH] net: core: Fix Spectre v1 vulnerability

[Gustavo A. R. Silva]

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>
---
 net/core/filter.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/core/filter.c b/net/core/filter.c
index 447dd1bad31f..8ec4337256ed 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -73,6 +73,7 @@
 #include <linux/seg6_local.h>
 #include <net/seg6.h>
 #include <net/seg6_local.h>
+#include <linux/nospec.h>
 
 /**
  *	sk_filter_trim_cap - run a packet through a socket filter
@@ -1035,6 +1036,7 @@ static int bpf_check_classic(const struct sock_filter *filter,
 	bool anc_found;
 	int pc;
 
+	flen = array_index_nospec(flen, BPF_MAXINSNS + 1);
 	/* Check the filter code now */
 	for (pc = 0; pc < flen; pc++) {
 		const struct sock_filter *ftest = &filter[pc];
-- 
2.20.1

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

[David Miller]

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.

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

[Alexei Starovoitov]

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 change doesn't harm, but I don't think it's a good idea
to sprinkle kernel with array_index_nospec() just because some tool
produced a warning.

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

[David Miller]

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 change doesn't harm, but I don't think it's a good idea
> to sprinkle kernel with array_index_nospec() just because some tool
> produced a warning.

Ok, that makes sense, I can revert.

Just let me know.

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

[Gustavo A. R. Silva]

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

--
Gustavo

>> The change doesn't harm, but I don't think it's a good idea
>> to sprinkle kernel with array_index_nospec() just because some tool
>> produced a warning.
> 
> Ok, that makes sense, I can revert.
> 
> Just let me know.
> 

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

[Gustavo A. R. Silva]

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

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

[Alexei Starovoitov]

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?

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.

What tool did you use to analyze this?
Did you analyze all warnings case by case or just trusted the tool
and generated these patches?

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

[Gustavo A. R. Silva]

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

We might be interpreting the information available about Spectre a bit 
different, but when the Spectre paper talks about memory vs cpu speed it 
seems to me that it's just an example to illustrate how the microcode 
can come into the equation and speculate. So I'm genuinely curious about 
your last statement: "Slow load is a necessary attribute..." Where did 
you get that info from?

Can't we have the case in which the code can be "trained" to read
perfectly valid values for prog->len for quite a while, making the
microcode come into place and speculate about:

1013         if (flen == 0 || flen > BPF_MAXINSNS)
1014                 return false;

and then make flen to be greater than BPF_MAXINSNS?

> What tool did you use to analyze this?
> Did you analyze all warnings case by case or just trusted the tool
> and generated these patches?
> 

I read every case, but I sometimes might be wrong of course.

Thanks
--
Gustavo

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

[Gustavo A. R. Silva]

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

We might be interpreting the information available about Spectre a bit 
different, but when the Spectre paper talks about memory vs cpu speed it 
seems to me that it's just an example to illustrate how the microcode 
can come into the equation and speculate. So I'm genuinely curious about 
your last statement: "Slow load is a necessary attribute..." Where did 
you get that info from?

Can't we have the case in which the code can be "trained" to read
perfectly valid values for prog->len for quite a while, making the
microcode come into place and speculate about:

1013         if (flen == 0 || flen > BPF_MAXINSNS)
1014                 return false;

and then make flen to be greater than BPF_MAXINSNS?

> What tool did you use to analyze this?
> Did you analyze all warnings case by case or just trusted the tool
> and generated these patches?
> 

I read every case, but I sometimes might be wrong of course.

Thanks

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

[Gustavo A. R. Silva]

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

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

[Alexei Starovoitov]

On Sat, Dec 22, 2018 at 09:37:02PM -0600, Gustavo A. R. Silva wrote:
> 
> Can't we have the case in which the code can be "trained" to read
> perfectly valid values for prog->len for quite a while, making the
> microcode come into place and speculate about:
> 
> 1013         if (flen == 0 || flen > BPF_MAXINSNS)
> 1014                 return false;
> 
> and then make flen to be greater than BPF_MAXINSNS?

Yes. The user space can train line 1013 to mispredict by passing
smaller flen N times and then passing large flen.
Why do you think it's exploitable?

Without the patch in the mispredicted path the cpu will do
if (0 < flen) condition and since flen is hot in the cache
it will happily execute the filter[0] load...
and about 12-20 u-ops later (depending on u-arch of cpu) when
branch predictor realizes that it's a miss, the cpu will ignore
the values computed in the shadow cpu registers used by speculative execution
and go back to the 'return false' execution path.
The side effect of bringing filter[0] value in L1 cache is still there.
The cpu is incapable to undo that cache load. That's what spectre1 is about.
Do you see how filter[0] value in cpu L1 cache is exploitable?

I took another look at the following patches:
"net: core: Fix Spectre v1 vulnerability"
"nfc: af_nfc: Fix Spectre v1 vulnerability"
"can: af_can: Fix Spectre v1 vulnerability"
and I have to say that none of them are necessary.
I'm not sure whether there were other patches that pretend to fix spectre1.

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

[Gustavo A. R. Silva]

Alexei,

On 12/22/18 10:12 PM, Alexei Starovoitov wrote:
> On Sat, Dec 22, 2018 at 09:37:02PM -0600, Gustavo A. R. Silva wrote:
>>
>> Can't we have the case in which the code can be "trained" to read
>> perfectly valid values for prog->len for quite a while, making the
>> microcode come into place and speculate about:
>>
>> 1013         if (flen == 0 || flen > BPF_MAXINSNS)
>> 1014                 return false;
>>
>> and then make flen to be greater than BPF_MAXINSNS?
> 
> Yes. The user space can train line 1013 to mispredict by passing
> smaller flen N times and then passing large flen.
> Why do you think it's exploitable?
> 
> Without the patch in the mispredicted path the cpu will do
> if (0 < flen) condition and since flen is hot in the cache
> it will happily execute the filter[0] load...
> and about 12-20 u-ops later (depending on u-arch of cpu) when
> branch predictor realizes that it's a miss, the cpu will ignore
> the values computed in the shadow cpu registers used by speculative execution
> and go back to the 'return false' execution path.
> The side effect of bringing filter[0] value in L1 cache is still there.
> The cpu is incapable to undo that cache load. That's what spectre1 is about.
> Do you see how filter[0] value in cpu L1 cache is exploitable?
> 

In this regard, the policy has been to kill the speculation on that
first load (as I mentioned in my last email. It is also mentioned in
the commit log for every patch).

> I took another look at the following patches:
> "net: core: Fix Spectre v1 vulnerability"
> "nfc: af_nfc: Fix Spectre v1 vulnerability"
> "can: af_can: Fix Spectre v1 vulnerability"
> and I have to say that none of them are necessary.
> I'm not sure whether there were other patches that pretend to fix spectre1.
> 

It's not about pretending to fix it. It's about trying to prevent the
conditions that can actually trigger the exploitation of a potential
vulnerability.  The code is not always the same, it changes, it evolves,
and we are currently trying to catch that first load (that's what smatch
does in all these cases) that could eventually lead to an actual vuln.

Thanks
--
Gustavo

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

[Alexei Starovoitov]

On Sat, Dec 22, 2018 at 11:03:31PM -0600, Gustavo A. R. Silva wrote:
> Alexei,
> 
> On 12/22/18 10:12 PM, Alexei Starovoitov wrote:
> > On Sat, Dec 22, 2018 at 09:37:02PM -0600, Gustavo A. R. Silva wrote:
> > > 
> > > Can't we have the case in which the code can be "trained" to read
> > > perfectly valid values for prog->len for quite a while, making the
> > > microcode come into place and speculate about:
> > > 
> > > 1013         if (flen == 0 || flen > BPF_MAXINSNS)
> > > 1014                 return false;
> > > 
> > > and then make flen to be greater than BPF_MAXINSNS?
> > 
> > Yes. The user space can train line 1013 to mispredict by passing
> > smaller flen N times and then passing large flen.
> > Why do you think it's exploitable?
> > 
> > Without the patch in the mispredicted path the cpu will do
> > if (0 < flen) condition and since flen is hot in the cache
> > it will happily execute the filter[0] load...
> > and about 12-20 u-ops later (depending on u-arch of cpu) when
> > branch predictor realizes that it's a miss, the cpu will ignore
> > the values computed in the shadow cpu registers used by speculative execution
> > and go back to the 'return false' execution path.
> > The side effect of bringing filter[0] value in L1 cache is still there.
> > The cpu is incapable to undo that cache load. That's what spectre1 is about.
> > Do you see how filter[0] value in cpu L1 cache is exploitable?
> > 
> 
> In this regard, the policy has been to kill the speculation on that
> first load (as I mentioned in my last email. It is also mentioned in
> the commit log for every patch).
> 
> > I took another look at the following patches:
> > "net: core: Fix Spectre v1 vulnerability"
> > "nfc: af_nfc: Fix Spectre v1 vulnerability"
> > "can: af_can: Fix Spectre v1 vulnerability"
> > and I have to say that none of them are necessary.
> > I'm not sure whether there were other patches that pretend to fix spectre1.
> > 
> 
> It's not about pretending to fix it. It's about trying to prevent the
> conditions that can actually trigger the exploitation of a potential
> vulnerability.  The code is not always the same, it changes, it evolves,
> and we are currently trying to catch that first load (that's what smatch
> does in all these cases) that could eventually lead to an actual vuln.

in other words there is no bug and there is no vulnerability,
but there is a 'policy' set by ... ?
So hence Nack to the policy and Nack to the patches.

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

[David Miller]

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Date: Sat, 22 Dec 2018 22:00:00 -0800

> On Sat, Dec 22, 2018 at 11:03:31PM -0600, Gustavo A. R. Silva wrote:
>> > I took another look at the following patches:
>> > "net: core: Fix Spectre v1 vulnerability"
>> > "nfc: af_nfc: Fix Spectre v1 vulnerability"
>> > "can: af_can: Fix Spectre v1 vulnerability"
>> > and I have to say that none of them are necessary.
>> > I'm not sure whether there were other patches that pretend to fix spectre1.
 ...
> in other words there is no bug and there is no vulnerability,
> but there is a 'policy' set by ... ?
> So hence Nack to the policy and Nack to the patches.

I have to agree with Alexei after looking at all of this stuff one more
time.

I'm reverting all of these changes.

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

[Gustavo A. R. Silva]

On 12/23/18 5:58 PM, David Miller wrote:
> From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Date: Sat, 22 Dec 2018 22:00:00 -0800
> 
>> On Sat, Dec 22, 2018 at 11:03:31PM -0600, Gustavo A. R. Silva wrote:
>>>> I took another look at the following patches:
>>>> "net: core: Fix Spectre v1 vulnerability"
>>>> "nfc: af_nfc: Fix Spectre v1 vulnerability"
>>>> "can: af_can: Fix Spectre v1 vulnerability"
>>>> and I have to say that none of them are necessary.
>>>> I'm not sure whether there were other patches that pretend to fix spectre1.
>   ...
>> in other words there is no bug and there is no vulnerability,
>> but there is a 'policy' set by ... ?
>> So hence Nack to the policy and Nack to the patches.
> 
> I have to agree with Alexei after looking at all of this stuff one more
> time.
> 
> I'm reverting all of these changes.
> 

Yeah. That's fine with me.

Thanks
--
Gustavo