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

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

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.