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@xxxxxxxxx>
> > > > 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@xxxxxxxxxxxxxx>
> > > > > > 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@xxxxxxxxxxxxxx>
> > > > > >
> > > > > > 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