net: core: Fix Spectre v1 vulnerability

PATCH In review Gustavo A. R. Silva 3 participants 13 messages Dec 21, 2018 – Dec 24, 2018

Subsystems (4)

Networking Network drivers BPF BPF networking

lore

Share & tools

lore

Gustavo A. R. Silva

author

2018-12-21 20:49 UTC #0 lore raw

PATCH 1/1 net: core: Fix Spectre v1 vulnerability

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 <[email protected]>

---
 net/core/filter.c | 2 ++
 1 file changed, 2 insertions(+)

net/core/filter.c +2 -0

index 447dd1bad31f..8ec4337256ed 100644
@@ -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];

Signature · 1 line

2.20.1

David Miller

maintainer

2018-12-22 23:07 UTC #1 lore raw

From: "Gustavo A. R. Silva" <[email protected]>
Date: Fri, 21 Dec 2018 14:49:01 -0600

Quoted 15 lines

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

Quoted 7 lines

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 <[email protected]>

BPF folks, I'll take this directly.

Applied and queued up for -stable, thanks.

Alexei Starovoitov

2018-12-22 23:59 UTC #2 lore raw

Quoted 22 lines · David Miller

From: "Gustavo A. R. Silva" <[email protected]>
Date: Fri, 21 Dec 2018 14:49:01 -0600
Quoted 19 lines
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].

Quoted 7 lines

Quoted 3 lines
[1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2
Signed-off-by Gustavo A. R. Silva <[email protected]>
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.

David Miller

maintainer

2018-12-23 02:40 UTC #3 lore raw

From: Alexei Starovoitov <[email protected]>
Date: Sat, 22 Dec 2018 15:59:54 -0800

Quoted 31 lines · David Miller

Quoted 22 lines
From: "Gustavo A. R. Silva" <[email protected]>
Date: Fri, 21 Dec 2018 14:49:01 -0600
Quoted 19 lines
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].
Quoted 7 lines
Quoted 3 lines
[1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2
Signed-off-by Gustavo A. R. Silva <[email protected]>
BPF folks, I'll take this directly.
Applied and queued up for -stable, thanks.

Quoted 9 lines

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.

Gustavo A. R. Silva

author

2018-12-23 02:53 UTC #4 lore raw

Hi,

Quoted 40 lines · David Miller

From: Alexei Starovoitov <[email protected]>
Date: Sat, 22 Dec 2018 15:59:54 -0800
Quoted 37 lines · David Miller
Quoted 22 lines
From: "Gustavo A. R. Silva" <[email protected]>
Date: Fri, 21 Dec 2018 14:49:01 -0600
Quoted 19 lines
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].
Quoted 7 lines
Quoted 3 lines
[1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2
Signed-off-by Gustavo A. R. Silva <[email protected]>
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[].

Alexei Starovoitov

2018-12-23 03:00 UTC #5 lore raw

Quoted 46 lines · Gustavo A. R. Silva

Hi,
Quoted 40 lines · David Miller
From: Alexei Starovoitov <[email protected]>
Date: Sat, 22 Dec 2018 15:59:54 -0800
Quoted 37 lines · David Miller
Quoted 22 lines
From: "Gustavo A. R. Silva" <[email protected]>
Date: Fri, 21 Dec 2018 14:49:01 -0600
Quoted 19 lines
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].
Quoted 7 lines
Quoted 3 lines
[1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2
Signed-off-by Gustavo A. R. Silva <[email protected]>
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:

Quoted 6 lines

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?

Gustavo A. R. Silva

author

2018-12-23 03:50 UTC #7 lore raw

In reply to Gustavo A. R. Silva

Alexei,

Quoted 57 lines · Gustavo A. R. Silva

Quoted 54 lines · Alexei Starovoitov
Quoted 52 lines · Gustavo A. R. Silva
Hi,
Quoted 40 lines · David Miller
From: Alexei Starovoitov <[email protected]>
Date: Sat, 22 Dec 2018 15:59:54 -0800
Quoted 37 lines · David Miller
Quoted 22 lines
From: "Gustavo A. R. Silva" <[email protected]>
Date: Fri, 21 Dec 2018 14:49:01 -0600
Quoted 19 lines
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].
Quoted 7 lines
Quoted 3 lines
[1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2
Signed-off-by Gustavo A. R. Silva <[email protected]>
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[].

Quoted 16 lines

Quoted 2 lines
Why do you think it can be mispredicted?
Beause fprog->len comes from user space:
bpf_prog_create_from_user() -> bpf_check_basics_ok()
Quoted 9 lines
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) {

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

Gustavo A. R. Silva

author

2018-12-23 03:57 UTC #6 lore raw

In reply to Alexei Starovoitov

Quoted 47 lines · Alexei Starovoitov

Quoted 46 lines · Gustavo A. R. Silva
Hi,
Quoted 40 lines · David Miller
From: Alexei Starovoitov <[email protected]>
Date: Sat, 22 Dec 2018 15:59:54 -0800
Quoted 37 lines · David Miller
Quoted 22 lines
From: "Gustavo A. R. Silva" <[email protected]>
Date: Fri, 21 Dec 2018 14:49:01 -0600
Quoted 19 lines
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].
Quoted 7 lines
Quoted 3 lines
[1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2
Signed-off-by Gustavo A. R. Silva <[email protected]>
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:

Quoted 9 lines

Quoted 6 lines
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()

Quoted 9 lines

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?

Quoted 4 lines

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

Alexei Starovoitov

2018-12-23 04:12 UTC #8 lore raw

Quoted 9 lines · Gustavo A. R. Silva

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.

Gustavo A. R. Silva

author

2018-12-23 05:03 UTC #9 lore raw

Alexei,

Quoted 15 lines · Alexei Starovoitov

Quoted 9 lines · Gustavo A. R. Silva
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?

Quoted 11 lines

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

Quoted 7 lines

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

Alexei Starovoitov

2018-12-23 06:00 UTC #10 lore raw

Quoted 42 lines · Gustavo A. R. Silva

Alexei,
Quoted 15 lines · Alexei Starovoitov
Quoted 9 lines · Gustavo A. R. Silva
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?
Quoted 11 lines
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).
Quoted 7 lines
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.

Quoted 5 lines

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.

David Miller

maintainer

2018-12-23 23:58 UTC #11 lore raw

From: Alexei Starovoitov <[email protected]>
Date: Sat, 22 Dec 2018 22:00:00 -0800

Quoted 7 lines · Gustavo A. R. Silva

Quoted 6 lines
Quoted 6 lines
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.

...

Quoted 3 lines

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.

Gustavo A. R. Silva

author

2018-12-24 00:01 UTC #12 lore raw

Quoted 11 lines · David Miller

From: Alexei Starovoitov <[email protected]>
Date: Sat, 22 Dec 2018 22:00:00 -0800
Quoted 7 lines · Gustavo A. R. Silva
Quoted 6 lines
Quoted 6 lines
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.
...

Quoted 9 lines

Quoted 3 lines
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