Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1240258 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 1/3] uio: add ioctl support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-06 10:40 +0200
    Re: [PATCH v3 1/3] uio: add ioctl support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-06 16:20 +0200
      Re: [PATCH v3 1/3] uio: add ioctl support Gleb Natapov <gleb@scylladb.com> - 2015-10-06 16:40 +0200
        Re: [PATCH v3 1/3] uio: add ioctl support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-06 17:30 +0200
          Re: [PATCH v3 1/3] uio: add ioctl support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-06 17:40 +0200
          Re: [PATCH v3 1/3] uio: add ioctl support Gleb Natapov <gleb@scylladb.com> - 2015-10-06 18:00 +0200

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

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

>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1240462

On Tue, Oct 06, 2015 at 11:33:56AM +0300, Vlad Zolotarov wrote:
> the solution u propose should be a matter of a separate patch and is
> obviously orthogonal to this series.

Doesn't work this way, sorry. You want a patch enabling MSI merged,
you need to secure the MSI configuration.

And it's going to be a lot of work, duplicating a bunch of code from
VFIO.

-- 
MST
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1240489

On Tue, Oct 06, 2015 at 05:19:22PM +0300, Michael S. Tsirkin wrote:
> On Tue, Oct 06, 2015 at 11:33:56AM +0300, Vlad Zolotarov wrote:
> > the solution u propose should be a matter of a separate patch and is
> > obviously orthogonal to this series.
> 
> Doesn't work this way, sorry. You want a patch enabling MSI merged,
> you need to secure the MSI configuration.
> 
MSI can be enabled right now without the patch by writing directly into
PCI bar. The only thing this patch adds is forwarding the interrupt to
an eventfd.
 
--
			Gleb.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1240560

On Tue, Oct 06, 2015 at 05:30:31PM +0300, Gleb Natapov wrote:
> On Tue, Oct 06, 2015 at 05:19:22PM +0300, Michael S. Tsirkin wrote:
> > On Tue, Oct 06, 2015 at 11:33:56AM +0300, Vlad Zolotarov wrote:
> > > the solution u propose should be a matter of a separate patch and is
> > > obviously orthogonal to this series.
> > 
> > Doesn't work this way, sorry. You want a patch enabling MSI merged,
> > you need to secure the MSI configuration.
> > 
> MSI can be enabled right now without the patch by writing directly into
> PCI bar.

By poking at config registers in sysfs? We can block this, or we
can log this, pretty easily. We don't ATM but it's not hard to do.

> The only thing this patch adds is forwarding the interrupt to
> an eventfd.

This one just adds a bunch of ioctls. The next ones do
more than you describe.

> --
> 			Gleb.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1240582

On 10/06/15 18:19, Michael S. Tsirkin wrote:
> On Tue, Oct 06, 2015 at 05:30:31PM +0300, Gleb Natapov wrote:
>> On Tue, Oct 06, 2015 at 05:19:22PM +0300, Michael S. Tsirkin wrote:
>>> On Tue, Oct 06, 2015 at 11:33:56AM +0300, Vlad Zolotarov wrote:
>>>> the solution u propose should be a matter of a separate patch and is
>>>> obviously orthogonal to this series.
>>> Doesn't work this way, sorry. You want a patch enabling MSI merged,
>>> you need to secure the MSI configuration.
>>>
>> MSI can be enabled right now without the patch by writing directly into
>> PCI bar.
> By poking at config registers in sysfs? We can block this, or we
> can log this, pretty easily. We don't ATM but it's not hard to do.
>
>> The only thing this patch adds is forwarding the interrupt to
>> an eventfd.
> This one just adds a bunch of ioctls. The next ones do
> more than you describe.

This one adds zero ioctls and the next one does exactly what Gleb 
describes.

>
>> --
>> 			Gleb.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1240616

On Tue, Oct 06, 2015 at 06:19:34PM +0300, Michael S. Tsirkin wrote:
> On Tue, Oct 06, 2015 at 05:30:31PM +0300, Gleb Natapov wrote:
> > On Tue, Oct 06, 2015 at 05:19:22PM +0300, Michael S. Tsirkin wrote:
> > > On Tue, Oct 06, 2015 at 11:33:56AM +0300, Vlad Zolotarov wrote:
> > > > the solution u propose should be a matter of a separate patch and is
> > > > obviously orthogonal to this series.
> > > 
> > > Doesn't work this way, sorry. You want a patch enabling MSI merged,
> > > you need to secure the MSI configuration.
> > > 
> > MSI can be enabled right now without the patch by writing directly into
> > PCI bar.
> 
> By poking at config registers in sysfs? We can block this, or we
> can log this, pretty easily. We don't ATM but it's not hard to do.
> 
Blocking this will break userspace API. As a maintainer you should know
that we do not break userspace APIs. Logging this is fine, but how
exactly it helps you with "security"? The patch in question already
taints the kernel which is much stronger than logging.

> > The only thing this patch adds is forwarding the interrupt to
> > an eventfd.
> 
> This one just adds a bunch of ioctls. The next ones do
> more than you describe.
> 
Yes, it adds bunch of ioctls to do exactly what I wrote above. What
point have you tried to make by this statement? It eluded me.

--
			Gleb.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web