[MODERATED] Re: [PATCH v6 31/43] MDSv6

On Mon, Feb 25, 2019 at 04:49:35PM +0100, speck for Greg KH wrote:
> On Mon, Feb 25, 2019 at 07:34:11AM -0800, speck for Andi Kleen wrote:
> > On Mon, Feb 25, 2019 at 04:19:35PM +0100, speck for Greg KH wrote:
> > > On Sun, Feb 24, 2019 at 07:07:37AM -0800, speck for Andi Kleen wrote:
> > > > From: Andi Kleen <[email protected]>
> > > > Subject:  mds sweep: Clear cpu for usbmon intercepts
> > > > 
> > > > usbmon touches user data in interrupts that otherwise don't
> > > > touch user data. Automatically schedule a clear cpu if
> > > > usbmon is called from an interrupt.
> > > 
> > > I have written a long and very satisfying rant about this patch, that I
> > > than deleted, as it made me feel very better, but probably would not
> > > have helped anyone else out.
> > > 
> > > In turn, I need you to properly justify this patch as these two tiny
> > > sentences, and this small patch make no sense to me at all.  Please
> > > explain _WHY_ this is needed in this specific location.  Before
> > > responding, I would strongly recommend reading up on exactly what usbmon
> > > is and who is allowed to use it.  If after doing that, you still feel
> > 
> > Right it's root only.
> > 
> > But this is not about leaking data to the root monitoring user
> > (who can see the data anyways), but to unrelated processes
> > which are not root, but happen to be interrupted by the USB
> > interrupt.
> 
> Then why are you messing around with the usbmon callback?  It has
> nothing to do with anything here.  By hooking it here, you now have 2
> calls to this function on the USB urb callback path.
> 
> The fact that a root process happens to be watching the USB data flowing
> through the system, or not, should have no affect on anything here, as
> the data flow is still the same (with the exception an extra copy in the
> irq could happen).  Does multiple copys matter or not?  I can't find
> anything in the documentation we have about this, am I missing it?
> 
> > > this patch is needed (and it might be, I still can not tell for sure),
> > 
> > Anything that touches user data in an interrupt needs to be marked
> > with the lazy approach.
> 
> As I asked with the hcd change, what is "user data"?
FWIW when I started my review of the SCSI drivers (which I'm not super
confident about) I was looking for anthing that copied non-meta data that could
land in a cache line on all possible code paths while in interrupt context.

My understanding of MDS (which might not be as correct as others) is that the
internal uArch buffers (fill, load, store) used when populating the cache lines
can leak stale data to a attacking process doing a "sampling" looking for
interesting data patterns.  Such as secret key's or something.  The attacker
will need to be lucky and gather data over time but, over time some data could
be useful.


> > I can write more on this instance.
> 
> I nicely asked for that in the past but was ignored twice.  Do I need to
> ask for it again in a non-nice manner?
> 
> Without that information, this patchset is pretty impossible to review.
> 
> > However I will probably not be able to write a detailed
> > description for each of the interrupt handlers changed because
> > there are just too many.
> 
> Then how do you expect each subsystem / driver author to know if this is
> an acceptable change or not?  How do you expect to educate driver
> authors to have them determine if they need to do this on their new
> drivers or not?  Are you going to hand-audit each new driver that gets
> added to the kernel for forever?

Athough Andi's patch does include some Documentation changes that help explain
what a driver author needs to look out for I think this approach will be hard
to sustain (and backport).

--mark

> Without this type of information, this seems like a futile exercise.
> 
> greg k-h