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.starovoi...@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" <gust...@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 <gust...@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