Re: [PATCH] net: core: Fix Spectre v1 vulnerability
On 12/23/18 5:58 PM, David Miller wrote: From: Alexei Starovoitov 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
Re: [PATCH] net: core: Fix Spectre v1 vulnerability
From: Alexei Starovoitov 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
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
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
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
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 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" 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 = [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=152449131114778=2 Signed-off-by: Gustavo A. R. Silva 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
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 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" 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 = [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=152449131114778=2 Signed-off-by: Gustavo A. R. Silva 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 = [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
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 > > 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" > > > > 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 = [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=152449131114778=2 > > > > > > > > > > Signed-off-by: Gustavo A. R. Silva > > > > > > > > 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
Hi, On 12/22/18 8:40 PM, David Miller wrote: From: Alexei Starovoitov 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" 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 = [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=152449131114778=2 Signed-off-by: Gustavo A. R. Silva 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
From: Alexei Starovoitov 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" >> 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 = [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=152449131114778=2 >> > >> > Signed-off-by: Gustavo A. R. Silva >> >> 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
On Sat, Dec 22, 2018 at 03:07:22PM -0800, David Miller wrote: > From: "Gustavo A. R. Silva" > 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 = [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=152449131114778=2 > > > > Signed-off-by: Gustavo A. R. Silva > > 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
From: "Gustavo A. R. Silva" 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 = [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=152449131114778=2 > > Signed-off-by: Gustavo A. R. Silva BPF folks, I'll take this directly. Applied and queued up for -stable, thanks.
[PATCH] 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 = [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=152449131114778=2 Signed-off-by: Gustavo A. R. Silva --- 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 #include #include +#include /** * 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 = [pc]; -- 2.20.1