Re: [PATCH 06/18] x86, barrier: stop speculation for failed access_ok

[Alexei Starovoitov]

On Sat, Jan 06, 2018 at 06:38:59PM +0000, Alan Cox wrote:
> On Sat, 6 Jan 2018 10:13:33 -0800
> Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote:
> 
> > On Sat, Jan 06, 2018 at 12:32:42PM +0000, Alan Cox wrote:
> > > On Fri, 5 Jan 2018 18:52:07 -0800
> > > Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> > >   
> > > > On Fri, Jan 5, 2018 at 5:10 PM, Dan Williams <dan.j.williams@xxxxxxxxx> wrote:  
> > > > > From: Andi Kleen <ak@xxxxxxxxxxxxxxx>
> > > > >
> > > > > When access_ok fails we should always stop speculating.
> > > > > Add the required barriers to the x86 access_ok macro.    
> > > > 
> > > > Honestly, this seems completely bogus.  
> > > 
> > > Also for x86-64 if we are trusting that an AND with a constant won't get
> > > speculated into something else surely we can just and the address with ~(1
> > > << 63) before copying from/to user space ? The user will then just
> > > speculatively steal their own memory.  
> > 
> > +1
> > 
> > Any type of straight line code can address variant 1.
> > Like changing:
> >   array[index]
> > into
> >   array[index & mask]
> > works even when 'mask' is a variable.
> 
> That statement is unfortunately not one that we currently believe is
> true for all architectures, platforms and implementations. It may be true
> for some architectures but processors can speculate on more than just
> execution paths. For some architecutres it may be the right way to
> implement Linus array_* methods.

please name one where cpu will ignore above '&' and speculate
load from array based on 'index' only.
cpus execute what they see. speculative execution does the same
except results are not committed to visible registers and stay
in renanmed/shadow set. There is no 'undo' of the speculative execution.
The whole issue is that cache and branch predictor don't have
a shadow unlike registers.

> > I think "lets sprinkle lfence everywhere" approach is going to
> > cause serious performance degradation. Yet people pushing for lfence
> > didn't present any numbers.
> 
> Normally people who propose security fixes don't have to argue about the
> fact they added 30 clocks to avoid your box being 0wned.
> 
> If you look at the patches very very few are in remotely hot paths, which
> is good news. The ones in hot paths definitely need careful scrutiny but
> the priority ought to be fixing and then optimizing unless its obvious
> how to tackle it.

fdtable, access_ok and networking are clearly hot, no?