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

On Mon, Feb 25, 2019 at 09:11:54PM +0100, speck for Greg KH wrote:
> On Mon, Feb 25, 2019 at 10:10:23AM -0800, speck for Andi Kleen wrote:
> > > You obviously do not care about all bus data, only some.  So which
> > > "some"?  As we have class drivers, why not focus on the type of data you
> > 
> > For most drivers anything that would end up being copy_*_user()ed.
> > 
> > (modulo some corner cases like a network address. While I would
> > like to protect those too, I don't see a practical way to do so.
> > And they're already visible on the network)
> > 
> > > feel is an issue?  That way you have a chance to keep on top of this
> > > thing.
> > 
> > Ok so the problem is the terminology? 
> > 
> > We should call it sensitive data?
> > 
> > I'll not be able to list all the cases explicitely in the document
> > (this would require understanding all drivers and all subsystems
> > which I certainly don't), there has to be some discretion.
> > 
> > > If you only focus on a bus, then you will miss those types of things
> > > that do not use a bus.  If you focus on the type of data, then you have
> > > a chance to do this right.
> > 
> > I wasn't really looking for busses, or anything like that, mainly just going
> > through the code and looking for copies or direct data access.
> 
> To make this clearer to me, is this a better definition of the problem:
> 
>   - Whenever the kernel touches data that userspace will eventually
>     see, but does not want any other userspace process to see, the CPU
>     buffers need to be flushed before returning from the kernel to
>     userspace.
> 
> Is that correct?  If not, how would you rephrase this in a single
> sentence?

Ok, I just read the "Deep Dive: Intel Analysis of MSBDS" pdf that Intel
gave me last weekend, and I think the above sentence is correct.  But it
would be great for someone to verify it.

> If this statement is true, do you care about multiple copies of that
> data, or only the "last" one as you just need to flush things when
> returning to userspace?

And to answer myself, I think that all is needed is to flush when
returning from userspace, which lazy_clear_cpu_interrupt() _should_ be
marking to have happen.

So my original point remains for the usbmon patch, it's pointless :)

> I ask as your patchset, for the USB data flow cases, calls
> lazy_clear_cpu_interrupt() twice on the same irq, each time after the
> buffer was copied off, which is very odd to me.
> 
> So do we care about the first copy, every copy, the last copy, or do we
> just need to call lazy_clear_cpu_interrupt() at least once per data
> flow?
> 
> As an example, here's the data coming in from a USB keyboard through to
> userspace:
> 	- USB irq
> 	- USB data potentially copied from USB controller buffer ->
> 	  usbmon buffer
> 	- USB data copied from USB controller buffer -> HID driver buffer
> 	- HID driver parsing and copying -> input data buffer
> 	- input data buffer copied -> input event buffer(s)
> 	- irq return
> 
> So we have USB layer copying once, maybe twice, HID layer copying, and
> input layer copying (at least once, maybe multiple times as you can
> multiplex input streams together).  Where do we care about calling
> lazy_clear_cpu_interrupt() on this call chain?  Everywhere, or just at
> least once?
> 
> If only once, why not just care about the input layer, as that is where
> things get sent to userspace and that is the _type_ of data you are
> concerned about.
> 
> [Note, I'm not sure about the input layer, it's just an example, things
> could be better than it used to be, but you get the idea here.]
> 
> Same goes for USB storage data.  Sometimes that uses dma bounce buffers,
> do we care about those?  If not, do we care later if the SCSI layer
> strips things off and reconstructs data paths and feeds that to the
> block layer?  If so, shouldn't we only care about the block layer and
> not scsi or USB?
> 
> I'm asking you to think of the data either at the physical layer (USB,
> PCI, DMA, MIPI, I2C, etc.) or at the logical layer (tty, bluetooth,
> mouse, sound input, block, etc.) or both.  Right now it's not obvious
> from your patchset what the correct thing is at all.

After reading the paper, I think you need to focus on the logical layer,
when possible, and for busses that copy data in an irq, care about them
_IFF_ they are carrying data for a logical type you care about.

greg k-h