Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support

[Avi Kivity <avi@scylladb.com>]

On 10/08/2015 11:32 AM, Michael S. Tsirkin wrote:
> On Thu, Oct 08, 2015 at 08:33:45AM +0300, Avi Kivity wrote:
>> On 08/10/15 00:05, Michael S. Tsirkin wrote:
>>> On Wed, Oct 07, 2015 at 07:39:16PM +0300, Avi Kivity wrote:
>>>> That's what I thought as well, but apparently adding msix support to the
>>>> already insecure uio drivers is even worse.
>>> I'm glad you finally agree what these drivers are doing is insecure.
>>>
>>> And basically kernel cares about security, no one wants to maintain insecure stuff.
>>>
>>> So you guys should think harder whether this code makes any sense upstream.
>> You simply ignore everything I write, cherry-picking the word "insecure" as
>> if it makes your point.  That is very frustrating.
> And I'm sorry about the frustration.  I didn't intend to twist your
> words. It's just that I had to spend literally hours trying to explain
> that security matters in kernel, and all I was getting back was a
> summary "there's no security issue because there are other way to
> corrupt memory".

The word security has several meanings.  The primary meaning is "defense 
against a malicious attacker".  In that sense, there is no added value 
at all, because the attacker is already root, and can already access all 
of kernel and user memory.  Even if the attacker is not root, and just 
has access to a non-iommu-protected device, they can still DMA to and 
from any memory they like.

This sense of the word however is irrelevant for this conversation; the 
user already gave up on it when they chose to use uio_pci_generic 
(either because they have no iommu, or because they need the extra 
performance).

Do we agree that security, in the sense of defense against a malicious 
attacker, is irrelevant for this conversation?

A secondary meaning is protection against inadvertent bugs.  Yes, a 
faulty memory write that happens to land in the msix page, can cause a 
random memory word to be overwritten.  But so can a faulty memory write 
into the rings, or the data structures that support virtual->physical 
translation, the data structures that describe the packets before 
translation, the memory allocator or pool.  The patch extends the 
vulnerable surface, but by a negligible amount.

>
> So I was glad when it looked like there's finally an agreement that yes,
> there's value in validating userspace input and yes, it's insecure
> not to do this.



>
>> It is good practice to defend against root oopsing the kernel, but in some
>> cases it cannot be achieved.
> I originally included ways to fix issues that I pointed out, ranging
> from harder to implement with more overhead but more secure to easier to
> implement with less overhead but less secure.  There didn't seem to be
> an understanding that the issues are there at all, so I stopped doing
> that - seemed like a waste of time.
>
> For example, will it kill your performance to reset devices cleanly, on
> open and close,

I don't recall this being mentioned at all.  It seems completely 
unrelated to a patch adding msix support to uio_pci_generic.

>   protect them from writes into MSI config, BAR registers
> and related capablities etc etc?

Obviously the userspace driver has to write to the BAR area.

If you're talking about the BAR setup registers, yes there is some 
(tiny) value in that, but how is it related to this patch?

Protecting the MSI area in the BARs _is_ related to the patch.  I agree 
it adds value, if small.

>    And if not, why are you people wasting
> time arguing about that?

I you want to use your position as maintainer of uio_pci_generic to get 
people to overhaul the driver for you with unrelated changes, they will 
object.  I can understand a maintainer pointing out the right way to do 
something rather than the wrong way.  But piling on a list of unrelated 
features as prerequisites is, in my opinion, abuse.

Let me repeat that pci_uio_generic is already used for userspace 
drivers, with all the issues that you point out, for a long while now. 
These issues are not exposed by the requirement to use msix. You are not 
protecting the kernel in any way by blocking the patch, you are only 
protecting people with iommu-less configurations from using their hardware.

>    The only thing I heard is that it's a hassle.
> That's true (though if you follow my advice and try to share code with
> vfio/pci you get a lot of this logic for free).

My thinking was that vfio was for secure (in the "defense against 
malicious attackers" sense) while uio_pci_generic was, de-facto at 
least, for use by trusted users.

We are in the strange situation that the Alex is open to adding an 
insecure mode to vfio, while you object to a patch which does not change 
the security of uio_pci_generic in any way; it only makes it more usable 
at the cost of a tiny increase in the bug surface.

>    So it's an
> understandable argument if you just need something that works, quickly.
> But if it's such a stopgap hack, there's no need to insist on it
> upstream.

It is not more or less a hack than uio_pci_generic allowing DMA, or 
/dev/mem, or the module loading interface, or nommu kernels. Security is 
just one aspect of the kernel, not the only one.

It's perfectly reasonable to taint the kernel when insecure DMA is 
enabled, and to allow the administrator to disable the interface 
completely.  What I don't understand is why, given that the user allows 
DMA, we should prevent them from using MSIX in addition.