Date:	Thu, 8 Oct 2015 16:20:12 +0300
From:	Avi Kivity <avi@...lladb.com>
To:	"Michael S. Tsirkin" <mst@...hat.com>
Cc:	Alex Williamson <alex.williamson@...hat.com>,
	Vlad Zolotarov <vladz@...udius-systems.com>,
	Greg KH <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org, hjk@...sjkoch.de, corbet@....net,
	bruce.richardson@...el.com, avi@...udius-systems.com,
	gleb@...udius-systems.com, stephen@...workplumber.org,
	alexander.duyck@...il.com
Subject: Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support

On 10/08/2015 01:26 PM, Michael S. Tsirkin wrote:
> On Thu, Oct 08, 2015 at 12:19:20PM +0300, Avi Kivity wrote:
>>
>> 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?
> No. uio_pci_generic currently can be used in a secure way in
> a sense that it's protected againt malicious attacker,
> assuming you bind it to a device that does not do DMA.

The context of the conversation is dpdk, which only supports DMA.

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


>
>> 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.
> http://mid.gmane.org/20151006005527-mutt-send-email-mst@redhat.com

Down at the moment for me.

> But really, this is just off the top of my head.
> These are all issues VFIO developers encountered
> and fixed over the years. Go into that code, read it,
> and you will discover the issues and the solutions.

vfio is solving a different problem, the problem of security against a 
malicious attacker, one that I'm hoping to agree here that we aren't 
attempting to solve.

People have been happily using uio_pci_generic despite all those 
issues.  All they were missing was msix support.  You can't use that to 
force them to overhaul that driver, or to add a new subsystem to vfio.

>
>>   It seems completely unrelated
>> to a patch adding msix support to uio_pci_generic.
> It isn't unrelated. It's because with MSIX patch you are enabling bus
> mastering in kernel.  So if you start device in a bad state it will
> corrupt kernel memory.

You are right, this patch can regress secure users.

I'd be surprised if there are msix-capable pci devices that do not rely 
on DMA, though.

>
>>>   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?
> If you don't, moving BARs will move the MSI-X region and
> protecting it won't help.

Won't it just become invisible if you do?

If userspace starts playing with BARs, you lost already, whether msix is 
enabled or not doesn't matter.  It can shadow other BARs, for example.

This is a general weakness of uio_pci_generic, not something exposed by 
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.
> I don't see them as unrelated.  Basically you want to turn
> uio_pci_generic into vfio/pci except without an IOMMU.

That is not what we want.  Simply adding msix support is sufficient for 
us.  Everything else was piled on afterwards.

> You will need a
> lot of VFIO code then.  That will need a lot of work.  You seem to blame
> me for this but IMHO that's because patch author has chosen a wrong
> approach.
>
>> 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.
> I answered this already. I don't agree with this.

With the first sentence or the second?  I'm trying really hard to 
understand the problem.

>
>> 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.
> Because it's either this patch or nothing at all? I don't believe that.
> Someone come along and write a better one.

 From your description, I can't imagine what the better patch looks 
like, except as a complete overhaul of uio_pci_generic.

Our requirement is to enable msix, not to pretend that it is secure 
while it allows DMA to any piece of memory in the system.

>
>>>    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.
> And some are using it in very broken ways. Yes. But now you want
> to fix this in stone by tying a kernel/userspace interface
> to their broken ways. I think that would be a mistake.

At heart, the brokenness here is that you allow insecure DMA. No amount 
of changes will fix this.

We need an interface for users that are prepared to give up kernel/user 
protection, either because they have no other choice, or because they 
have performance requirements that mandate it. What extra value does 
protecting the BARs against movement add? Nothing.

>
>> We are in the strange situation that the Alex is open to adding an insecure
>> mode to vfio,
> I don't find this strange. It seems to make sense. VFIO is
> already used with DMA capable devices.

It's strange to me because it's charter was for iommu-protected device 
assignment, while uio_pci_generic is for generic pci userspace.


>
>> 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.
> I don't agree with this either. This depends on the device.
>
>>>    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,
> It doesn't. sysfs does.
>
>> 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.
> There's no need to prevent MSIX with or without DMA.
>
> But UIO uses sysfs for device access. So if we program MSIX we need to
> extend sysfs to protect a ton of registers that are MSIX related from
> the user, and do a bunch of setup and cleanup otherwise kernel will be
> very confused.
>
> It might be surprising to you how many registers are MSIX related,
> but it's true.
>

Userspace can already do all of these things, confusing the kernel. It 
simply doesn't, which is why everything works.  It need only continue 
not to do so for everything to continue to work.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists