Re: [PATCH v3 1/3] uio: add ioctl support

[Vlad Zolotarov <vladz@cloudius-systems.com>]

On 10/06/15 01:29, Michael S. Tsirkin wrote:
> On Tue, Oct 06, 2015 at 12:43:45AM +0300, Vladislav Zolotarov wrote:
>> So, like it has already been asked in a different thread I'm going to
>> ask a rhetorical question: what adding an MSI and MSI-X interrupts support to
>> uio_pci_generic has to do with security?
> memory protection is a better term than security.
>
> It's very simple: you enable bus mastering and you ask userspace to map
> all device BARs. One of these BARs holds the address to which device
> writes to trigger MSI-X interrupt.
>
> This is how MSI-X works, internally: from the point of view of
> PCI it's a memory write. It just so happens that the destination
> address is in the interrupt controller, that triggers an interrupt.
>
> But a bug in this userspace application can corrupt the MSI-X table,
> which in turn can easily corrupt kernel memory, or unrelated processes's
> memory.  This is in my opinion unacceptable.
>
> So you need to be very careful
> - probably need to reset device before you even enable bus master
> - prevent userspace from touching msi config
> - prevent userspace from moving BARs since msi-x config is within a BAR
> - detect reset and prevent linux from touching device while it's under
>    reset
>
> The list goes on and on.
>
> This is pretty much what VFIO spent the last 3 years doing, except VFIO
> also can do IOMMU groups.
>
>> What "security threat" does it add
>> that u don't already have today?
> Yes, userspace can create this today if it tweaks PCI config space to
> enable MSI-X, then corrupts the MSI-X table.  It's unfortunate that we
> don't yet prevent this, but at least you need two things to go wrong for
> this to trigger.
>
> The reason, as I tried to point out, is simply that I didn't think
> uio_pci_generic will be used for these configurations.
> But there's nothing fundamental here that makes them secure
> and that therefore makes your patches secure as well.
>
> Fixing this to make uio_pci_generic write-protect MSI/MSI-X enable
> registers sounds kind of reasonable, this shouldn't be too hard.

Sure. But like u've just pointed out yourself - this is a general issue 
and it has nothing to do with the ability to get notifications per 
MSI-X/MSI interrupts, which this series adds (bus mastering may and is 
easily enabled from the user space - look for pci_uio_set_bus_master() 
function in the DPDK).

So, while I absolutely agree with u in regard to the fact that we have a 
security/memory corruption threat in the current in-tree uio_pci_generic 
- the solution u propose should be a matter of a separate patch and is 
obviously orthogonal to this series.

thanks,
vlad

>