| Message ID | 20171024200426.62811-1-jeffrey.t.kirsher@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Tue, 24 Oct 2017 13:04:26 -0700 Jeff Kirsher <jeffrey.t.kirsher@intel.com> wrote: > From: Liang-Min Wang <liang-min.wang@intel.com> > > When a SR-IOV supported device is bound with vfio-pci, the driver > could not create SR-IOV instance through /sys/bus/pci/devices/... > /sriov_numvfs. This patch re-activates this capability for a PCIe > device that supports SR-IOV and is bound with vfio-pci.ko. > > Signed-off-by: Liang-Min Wang <liang-min.wang@intel.com> > --- > drivers/vfio/pci/vfio_pci.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) Why? The PF bound to vfio-pci can be assigned to a user. PFs often have backdoors into the VF. Therefore this enables creation of a VF in the host that may be snooped or manipulated by a user. This clearly seems like a security issue. Thanks, Alex > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c > index f041b1a6cf66..8fbd362607e1 100644 > --- a/drivers/vfio/pci/vfio_pci.c > +++ b/drivers/vfio/pci/vfio_pci.c > @@ -1256,6 +1256,7 @@ static void vfio_pci_remove(struct pci_dev *pdev) > if (!vdev) > return; > > + pci_disable_sriov(pdev); > vfio_iommu_group_put(pdev->dev.iommu_group, &pdev->dev); > kfree(vdev->region); > kfree(vdev); > @@ -1303,12 +1304,23 @@ static const struct pci_error_handlers vfio_err_handlers = { > .error_detected = vfio_pci_aer_err_detected, > }; > > +static int vfio_sriov_configure(struct pci_dev *pdev, int num_vfs) > +{ > + if (!num_vfs) { > + pci_disable_sriov(pdev); > + return 0; > + } > + > + return pci_enable_sriov(pdev, num_vfs); > +} > + > static struct pci_driver vfio_pci_driver = { > .name = "vfio-pci", > .id_table = NULL, /* only dynamic ids */ > .probe = vfio_pci_probe, > .remove = vfio_pci_remove, > .err_handler = &vfio_err_handlers, > + .sriov_configure = vfio_sriov_configure, > }; > > struct vfio_devices {
Just like any PCIe devices that supports SR-IOV. There are restrictions set for VF. Also, there is a concept of trust VF now available for PF to manage certain features that only selected VF could exercise. Are you saying all the devices supporting SR-IOV all have security issue? Larry > -----Original Message----- > From: Alex Williamson [mailto:alex.williamson@redhat.com] > Sent: Tuesday, October 24, 2017 5:44 PM > To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com> > Cc: Wang, Liang-min <liang-min.wang@intel.com>; kvm@vger.kernel.org; > linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; > bhelgaas@google.com; Duyck, Alexander H <alexander.h.duyck@intel.com> > Subject: Re: [PATCH] Enable SR-IOV instantiation through /sys file > > On Tue, 24 Oct 2017 13:04:26 -0700 > Jeff Kirsher <jeffrey.t.kirsher@intel.com> wrote: > > > From: Liang-Min Wang <liang-min.wang@intel.com> > > > > When a SR-IOV supported device is bound with vfio-pci, the driver > > could not create SR-IOV instance through /sys/bus/pci/devices/... > > /sriov_numvfs. This patch re-activates this capability for a PCIe > > device that supports SR-IOV and is bound with vfio-pci.ko. > > > > Signed-off-by: Liang-Min Wang <liang-min.wang@intel.com> > > --- > > drivers/vfio/pci/vfio_pci.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > Why? The PF bound to vfio-pci can be assigned to a user. PFs often > have backdoors into the VF. Therefore this enables creation of a VF in > the host that may be snooped or manipulated by a user. This clearly > seems like a security issue. Thanks, > > Alex > > > > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c > > index f041b1a6cf66..8fbd362607e1 100644 > > --- a/drivers/vfio/pci/vfio_pci.c > > +++ b/drivers/vfio/pci/vfio_pci.c > > @@ -1256,6 +1256,7 @@ static void vfio_pci_remove(struct pci_dev *pdev) > > if (!vdev) > > return; > > > > + pci_disable_sriov(pdev); > > vfio_iommu_group_put(pdev->dev.iommu_group, &pdev->dev); > > kfree(vdev->region); > > kfree(vdev); > > @@ -1303,12 +1304,23 @@ static const struct pci_error_handlers > vfio_err_handlers = { > > .error_detected = vfio_pci_aer_err_detected, > > }; > > > > +static int vfio_sriov_configure(struct pci_dev *pdev, int num_vfs) > > +{ > > + if (!num_vfs) { > > + pci_disable_sriov(pdev); > > + return 0; > > + } > > + > > + return pci_enable_sriov(pdev, num_vfs); > > +} > > + > > static struct pci_driver vfio_pci_driver = { > > .name = "vfio-pci", > > .id_table = NULL, /* only dynamic ids */ > > .probe = vfio_pci_probe, > > .remove = vfio_pci_remove, > > .err_handler = &vfio_err_handlers, > > + .sriov_configure = vfio_sriov_configure, > > }; > > > > struct vfio_devices {
On Tue, 24 Oct 2017 21:49:15 +0000
"Wang, Liang-min" <liang-min.wang@intel.com> wrote:
> Just like any PCIe devices that supports SR-IOV. There are restrictions set for VF. Also, there is a concept of trust VF now available for PF to manage certain features that only selected VF could exercise. Are you saying all the devices supporting SR-IOV all have security issue?
Here's a simple example, most SR-IOV capable NICs, including those from
Intel, require the PF interface to be up in order to route traffic from
the VF. If the user controls the PF interface and VFs are used
elsewhere in the host, the PF driver in userspace can induce a denial
of service on the VFs. That doesn't even take into account that VFs
might be in separate IOMMU groups from the PF and therefore not
isolated from the host like the PF and that the PF driver can
potentially manipulate the VF, possibly performing DMA on behalf of the
PF. VFs are only considered secure today because the PF is managed by
a driver in the host kernel. Allowing simple enablement of VFs for a
user owned PF seems inherently insecure to me. Thanks,
Alex
> -----Original Message----- > From: Alex Williamson [mailto:alex.williamson@redhat.com] > Sent: Tuesday, October 24, 2017 6:07 PM > To: Wang, Liang-min <liang-min.wang@intel.com> > Cc: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>; kvm@vger.kernel.org; > linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; > bhelgaas@google.com; Duyck, Alexander H <alexander.h.duyck@intel.com> > Subject: Re: [PATCH] Enable SR-IOV instantiation through /sys file > > On Tue, 24 Oct 2017 21:49:15 +0000 > "Wang, Liang-min" <liang-min.wang@intel.com> wrote: > > > Just like any PCIe devices that supports SR-IOV. There are restrictions set for > VF. Also, there is a concept of trust VF now available for PF to manage certain > features that only selected VF could exercise. Are you saying all the devices > supporting SR-IOV all have security issue? > > Here's a simple example, most SR-IOV capable NICs, including those from > Intel, require the PF interface to be up in order to route traffic from > the VF. If the user controls the PF interface and VFs are used > elsewhere in the host, the PF driver in userspace can induce a denial > of service on the VFs. That doesn't even take into account that VFs > might be in separate IOMMU groups from the PF and therefore not > isolated from the host like the PF and that the PF driver can > potentially manipulate the VF, possibly performing DMA on behalf of the > PF. VFs are only considered secure today because the PF is managed by > a driver in the host kernel. Allowing simple enablement of VFs for a > user owned PF seems inherently insecure to me. Thanks, > > Alex So, I assume over PF+SR-IOV usage model, you would agree that PF is trusted, and not VF. So, the "potential" insecure issue occurs on both native device kernel driver and vfio-pci. The interface that is used to create SR-IOV is also considered trusted, either it's a script run by a network manager or manually done by network manager. So, it's up to the trusted network manager to give privileges to each individual VF according to respective policy. BTW, there is a separate effort on a similar support (https://lkml.org/lkml/2017/9/27/348). Do you have the same concern for uio_pci_generic? Liang-Min
On Tue, 24 Oct 2017 22:29:00 +0000 "Wang, Liang-min" <liang-min.wang@intel.com> wrote: > > -----Original Message----- > > From: Alex Williamson [mailto:alex.williamson@redhat.com] > > Sent: Tuesday, October 24, 2017 6:07 PM > > To: Wang, Liang-min <liang-min.wang@intel.com> > > Cc: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>; kvm@vger.kernel.org; > > linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; > > bhelgaas@google.com; Duyck, Alexander H <alexander.h.duyck@intel.com> > > Subject: Re: [PATCH] Enable SR-IOV instantiation through /sys file > > > > On Tue, 24 Oct 2017 21:49:15 +0000 > > "Wang, Liang-min" <liang-min.wang@intel.com> wrote: > > > > > Just like any PCIe devices that supports SR-IOV. There are restrictions set for > > VF. Also, there is a concept of trust VF now available for PF to manage certain > > features that only selected VF could exercise. Are you saying all the devices > > supporting SR-IOV all have security issue? > > > > Here's a simple example, most SR-IOV capable NICs, including those from > > Intel, require the PF interface to be up in order to route traffic from > > the VF. If the user controls the PF interface and VFs are used > > elsewhere in the host, the PF driver in userspace can induce a denial > > of service on the VFs. That doesn't even take into account that VFs > > might be in separate IOMMU groups from the PF and therefore not > > isolated from the host like the PF and that the PF driver can > > potentially manipulate the VF, possibly performing DMA on behalf of the > > PF. VFs are only considered secure today because the PF is managed by > > a driver in the host kernel. Allowing simple enablement of VFs for a > > user owned PF seems inherently insecure to me. Thanks, > > > > Alex > > So, I assume over PF+SR-IOV usage model, you would agree that PF is trusted, and not VF. So, the "potential" insecure issue occurs on both native device kernel driver and vfio-pci. The interface that is used to create SR-IOV is also considered trusted, either it's a script run by a network manager or manually done by network manager. So, it's up to the trusted network manager to give privileges to each individual VF according to respective policy. BTW, there is a separate effort on a similar support (https://lkml.org/lkml/2017/9/27/348). Do you have the same concern for uio_pci_generic? That thread doesn't seem to support this as a safe thing to do. Do we expect existing userspace PF drivers through vfio to recognize that SR-IOV is enabled? How would we coordinate enabling SR-IOV on a PF while the PF is up and running in a userspace driver? Plus the security concerns of a VF under the influence of a user owned PF. I would think that UIO would have all of these same concerns, but in reality UIO is severely abused and mostly used in insecure ways already, so security is already compromised through UIO. Host kernel drivers and core code is necessarily trusted, there's no memory protection between such code. A VF driver, in kernel or userspace, must trust the PF driver is operating benevolently, we have no other choice. We cannot make such assumptions about a userspace PF driver. VFs managed by a user owned PF would need to be quarantined in some way by default, IMO. Thanks, Alex
> -----Original Message----- > From: Alex Williamson [mailto:alex.williamson@redhat.com] > Sent: Tuesday, October 24, 2017 6:07 PM > To: Wang, Liang-min <liang-min.wang@intel.com> > Cc: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>; kvm@vger.kernel.org; > linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; > bhelgaas@google.com; Duyck, Alexander H <alexander.h.duyck@intel.com> > Subject: Re: [PATCH] Enable SR-IOV instantiation through /sys file > > On Tue, 24 Oct 2017 21:49:15 +0000 > "Wang, Liang-min" <liang-min.wang@intel.com> wrote: > > > Just like any PCIe devices that supports SR-IOV. There are restrictions set for > VF. Also, there is a concept of trust VF now available for PF to manage certain > features that only selected VF could exercise. Are you saying all the devices > supporting SR-IOV all have security issue? > > Here's a simple example, most SR-IOV capable NICs, including those from > Intel, require the PF interface to be up in order to route traffic from > the VF. If the user controls the PF interface and VFs are used > elsewhere in the host, the PF driver in userspace can induce a denial > of service on the VFs. That doesn't even take into account that VFs > might be in separate IOMMU groups from the PF and therefore not > isolated from the host like the PF and that the PF driver can > potentially manipulate the VF, possibly performing DMA on behalf of the > PF. VFs are only considered secure today because the PF is managed by > a driver in the host kernel. Allowing simple enablement of VFs for a > user owned PF seems inherently insecure to me. Thanks, > > Alex Firstly, the concern is on user-space PF driver based upon vfio-pci, this patch doesn't change PF behavior so with/without this patch, the concern remains the same. Secondly, the security concern (including denial of service) in general is to ensure trust entity to be trust-worthy. No matter the PF driver is in kernel-space or in user- space, necessary mechanism needs to be enforced on the device driver to ensure it's trusted worthy. For example, ixgbe kernel driver introduces a Tx hang detection to avoid driver stays in a bad state. Therefore, it's the responsibility of user-space driver function, which based upon vfio-pci, to enforce necessary mechanism to ensure its trust-ness. That's a given.
On Fri, 27 Oct 2017 21:50:43 +0000 "Wang, Liang-min" <liang-min.wang@intel.com> wrote: > > -----Original Message----- > > From: Alex Williamson [mailto:alex.williamson@redhat.com] > > Sent: Tuesday, October 24, 2017 6:07 PM > > To: Wang, Liang-min <liang-min.wang@intel.com> > > Cc: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>; kvm@vger.kernel.org; > > linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; > > bhelgaas@google.com; Duyck, Alexander H <alexander.h.duyck@intel.com> > > Subject: Re: [PATCH] Enable SR-IOV instantiation through /sys file > > > > On Tue, 24 Oct 2017 21:49:15 +0000 > > "Wang, Liang-min" <liang-min.wang@intel.com> wrote: > > > > > Just like any PCIe devices that supports SR-IOV. There are restrictions set for > > VF. Also, there is a concept of trust VF now available for PF to manage certain > > features that only selected VF could exercise. Are you saying all the devices > > supporting SR-IOV all have security issue? > > > > Here's a simple example, most SR-IOV capable NICs, including those from > > Intel, require the PF interface to be up in order to route traffic from > > the VF. If the user controls the PF interface and VFs are used > > elsewhere in the host, the PF driver in userspace can induce a denial > > of service on the VFs. That doesn't even take into account that VFs > > might be in separate IOMMU groups from the PF and therefore not > > isolated from the host like the PF and that the PF driver can > > potentially manipulate the VF, possibly performing DMA on behalf of the > > PF. VFs are only considered secure today because the PF is managed by > > a driver in the host kernel. Allowing simple enablement of VFs for a > > user owned PF seems inherently insecure to me. Thanks, > > > > Alex > > Firstly, the concern is on user-space PF driver based upon vfio-pci, this patch doesn't > change PF behavior so with/without this patch, the concern remains the same. This patch enables SR-IOV to be enabled via the host on a user-owned PF, how is this not a change in behavior? > Secondly, the security concern (including denial of service) in general is to ensure trust > entity to be trust-worthy. No matter the PF driver is in kernel-space or in user- space, > necessary mechanism needs to be enforced on the device driver to ensure it's > trusted worthy. For example, ixgbe kernel driver introduces a Tx hang detection > to avoid driver stays in a bad state. Therefore, it's the responsibility of user-space > driver function, which based upon vfio-pci, to enforce necessary mechanism to ensure > its trust-ness. That's a given. Userspace is not trustworthy, therefore the host kernel cannot place responsibility on a userspace driver for anything, including the behavior of VFs. I'm sorry, but it's a NAK unless you intend to follow-up with some proposal to quarantine the VFs enabled by the userspace PF driver. Thanks, Alex
> -----Original Message----- > From: Alex Williamson [mailto:alex.williamson@redhat.com] > Sent: Friday, October 27, 2017 6:19 PM > To: Wang, Liang-min <liang-min.wang@intel.com> > Cc: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>; kvm@vger.kernel.org; > linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; > bhelgaas@google.com; Duyck, Alexander H <alexander.h.duyck@intel.com> > Subject: Re: [PATCH] Enable SR-IOV instantiation through /sys file > > On Fri, 27 Oct 2017 21:50:43 +0000 > "Wang, Liang-min" <liang-min.wang@intel.com> wrote: > > > > -----Original Message----- > > > From: Alex Williamson [mailto:alex.williamson@redhat.com] > > > Sent: Tuesday, October 24, 2017 6:07 PM > > > To: Wang, Liang-min <liang-min.wang@intel.com> > > > Cc: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>; kvm@vger.kernel.org; > > > linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; > > > bhelgaas@google.com; Duyck, Alexander H > <alexander.h.duyck@intel.com> > > > Subject: Re: [PATCH] Enable SR-IOV instantiation through /sys file > > > > > > On Tue, 24 Oct 2017 21:49:15 +0000 > > > "Wang, Liang-min" <liang-min.wang@intel.com> wrote: > > > > > > > Just like any PCIe devices that supports SR-IOV. There are restrictions set > for > > > VF. Also, there is a concept of trust VF now available for PF to manage > certain > > > features that only selected VF could exercise. Are you saying all the devices > > > supporting SR-IOV all have security issue? > > > > > > Here's a simple example, most SR-IOV capable NICs, including those from > > > Intel, require the PF interface to be up in order to route traffic from > > > the VF. If the user controls the PF interface and VFs are used > > > elsewhere in the host, the PF driver in userspace can induce a denial > > > of service on the VFs. That doesn't even take into account that VFs > > > might be in separate IOMMU groups from the PF and therefore not > > > isolated from the host like the PF and that the PF driver can > > > potentially manipulate the VF, possibly performing DMA on behalf of the > > > PF. VFs are only considered secure today because the PF is managed by > > > a driver in the host kernel. Allowing simple enablement of VFs for a > > > user owned PF seems inherently insecure to me. Thanks, > > > > > > Alex > > > > Firstly, the concern is on user-space PF driver based upon vfio-pci, this patch > doesn't > > change PF behavior so with/without this patch, the concern remains the > same. > > This patch enables SR-IOV to be enabled via the host on a user-owned > PF, how is this not a change in behavior? > Are you saying without this patch, you have no concern denial-of-service on Vfio-pci based user-space driver > > Secondly, the security concern (including denial of service) in general is to > ensure trust > > entity to be trust-worthy. No matter the PF driver is in kernel-space or in > user- space, > > necessary mechanism needs to be enforced on the device driver to ensure it's > > trusted worthy. For example, ixgbe kernel driver introduces a Tx hang > detection > > to avoid driver stays in a bad state. Therefore, it's the responsibility of user- > space > > driver function, which based upon vfio-pci, to enforce necessary mechanism > to ensure > > its trust-ness. That's a given. > > Userspace is not trustworthy, therefore the host kernel cannot place > responsibility on a userspace driver for anything, including the > behavior of VFs. I'm sorry, but it's a NAK unless you intend to > follow-up with some proposal to quarantine the VFs enabled by the > userspace PF driver. Thanks, > > Alex So, your suggestion is to have VF instantiated through user-space driver "quarantine". Could you elaborate your definition of "quarantine"? Do you expect the enforcement is in vfio-pci or in user-space driver function, or both? Liang-Min
On Sat, 2017-10-28 at 00:19 +0200, Alex Williamson wrote: > On Fri, 27 Oct 2017 21:50:43 +0000 > "Wang, Liang-min" <liang-min.wang@intel.com> wrote: > > > > -----Original Message----- > > > From: Alex Williamson [mailto:alex.williamson@redhat.com] > > > Sent: Tuesday, October 24, 2017 6:07 PM > > > To: Wang, Liang-min <liang-min.wang@intel.com> > > > Cc: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>; kvm@vger.kernel.org; > > > linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; > > > bhelgaas@google.com; Duyck, Alexander H <alexander.h.duyck@intel.com> > > > Subject: Re: [PATCH] Enable SR-IOV instantiation through /sys file > > > > > > On Tue, 24 Oct 2017 21:49:15 +0000 > > > "Wang, Liang-min" <liang-min.wang@intel.com> wrote: > > > > > > > Just like any PCIe devices that supports SR-IOV. There are restrictions set for > > > > > > VF. Also, there is a concept of trust VF now available for PF to manage certain > > > features that only selected VF could exercise. Are you saying all the devices > > > supporting SR-IOV all have security issue? > > > > > > Here's a simple example, most SR-IOV capable NICs, including those from > > > Intel, require the PF interface to be up in order to route traffic from > > > the VF. If the user controls the PF interface and VFs are used > > > elsewhere in the host, the PF driver in userspace can induce a denial > > > of service on the VFs. That doesn't even take into account that VFs > > > might be in separate IOMMU groups from the PF and therefore not > > > isolated from the host like the PF and that the PF driver can > > > potentially manipulate the VF, possibly performing DMA on behalf of the > > > PF. VFs are only considered secure today because the PF is managed by > > > a driver in the host kernel. Allowing simple enablement of VFs for a > > > user owned PF seems inherently insecure to me. Thanks, > > > > > > Alex > > > > Firstly, the concern is on user-space PF driver based upon vfio-pci, this patch doesn't > > change PF behavior so with/without this patch, the concern remains the same. > > This patch enables SR-IOV to be enabled via the host on a user-owned > PF, how is this not a change in behavior? > > > Secondly, the security concern (including denial of service) in general is to ensure trust > > entity to be trust-worthy. No matter the PF driver is in kernel-space or in user- space, > > necessary mechanism needs to be enforced on the device driver to ensure it's > > trusted worthy. For example, ixgbe kernel driver introduces a Tx hang detection > > to avoid driver stays in a bad state. Therefore, it's the responsibility of user-space > > driver function, which based upon vfio-pci, to enforce necessary mechanism to ensure > > its trust-ness. That's a given. > > Userspace is not trustworthy, therefore the host kernel cannot place > responsibility on a userspace driver for anything, including the > behavior of VFs. I'm sorry, but it's a NAK unless you intend to > follow-up with some proposal to quarantine the VFs enabled by the > userspace PF driver. Thanks, > > Alex I don't see this so much as a security problem per-se. It all depends on the hardware setup. If I recall correctly, there are devices where the PF function doesn't really do much other than act as a bit more heavy-weight VF, and the actual logic is handled by a firmware engine on the device. The only real issue is that for devices like the Intel NICs instead of trusting a firmware engine we have historically used a kernel driver and now we are wanting to trust a user-space agent instead. I do think that we probably need to have some sort of signaling between user-space and vfio-pci that would allow for notifying the user-space of the change and for user-space to notify vfio-pci that it is capable of handling the notification. This is something that can be toggled at any time after all and not all devices have a means of notifying the PF that this has been changed. Beyond that once the root user enables the VFs I would kind of think they know what driver they have running them. Enabling VFs implies the root user trusts the application running on top of vfio-pci to handle the PF responsibly. At least that is how it works in my mind. Thanks. - Alexander (using full name since 2 Alexs in one thread can be confusing)
On Fri, Oct 27, 2017 at 11:20:41PM +0000, Duyck, Alexander H wrote: > I don't see this so much as a security problem per-se. It all depends > on the hardware setup. If I recall correctly, there are devices where > the PF function doesn't really do much other than act as a bit more > heavy-weight VF, and the actual logic is handled by a firmware engine > on the device. Can you cite an example? While those surely could exist in theory, I can't think of a practical example. Maybe we can start with the practical use case for this patch. That is what device is this intended for?
On Sat, Oct 28, 2017 at 11:16 PM, Christoph Hellwig <hch@infradead.org> wrote: > On Fri, Oct 27, 2017 at 11:20:41PM +0000, Duyck, Alexander H wrote: >> I don't see this so much as a security problem per-se. It all depends >> on the hardware setup. If I recall correctly, there are devices where >> the PF function doesn't really do much other than act as a bit more >> heavy-weight VF, and the actual logic is handled by a firmware engine >> on the device. > > Can you cite an example? While those surely could exist in theory, > I can't think of a practical example. If I recall the neterion vxge driver fell into that category if I recall correctly. Basically the hardware is preconfigured for some number of VFs and their driver just calls pci_enable_sriov and enables them. One other thing I forgot about is the fact that we already have drivers such as igb, ixgbe, and vxge floating around that will leave SR-IOV enabled if the driver is loaded while VFs are direct assigned to guests. As a side effect we can sort of already support assigning vfio-pci to a driver that has SR-IOV enabled. > Maybe we can start with the practical use case for this patch. That > is what device is this intended for? If I am not mistaken the typical use case for a patch like this is to support loading something like DPDK on a networking device PF, whlie the VFs are assigned to virtualized guests and/or containers. It would move the control of the PF/VFs into use space, but in the grand scheme of things it isn't much different then when a virtualized guest has a VF and has no direct control over the configuration of it since the host is managing that. So if the root user is enabling SR-IOV and allocating VFs on a device that is running the vfio-pci driver the assumption is that the root user must be trusting the application that is running on top of the vfio-pci driver to behave correctly.
On Sat, 2017-10-28 at 23:16 -0700, Christoph Hellwig wrote: > On Fri, Oct 27, 2017 at 11:20:41PM +0000, Duyck, Alexander H wrote: > > > > I don't see this so much as a security problem per-se. It all depends > > on the hardware setup. If I recall correctly, there are devices where > > the PF function doesn't really do much other than act as a bit more > > heavy-weight VF, and the actual logic is handled by a firmware engine > > on the device. > > Can you cite an example? While those surely could exist in theory, > I can't think of a practical example. I have them, which is why I'm patching the UIO driver to allow num_vfs to be set. I don't even want to *use* the UIO driver for any purpose except to make that appear in sysfs. It's all handled in the device. (I think we might be able to just give the PF out to a guest as if it were just another VF, but I don't think we actually *do* that right now).
> -----Original Message----- > From: David Woodhouse [mailto:dwmw2@infradead.org] > Sent: Monday, October 30, 2017 8:39 AM > To: Christoph Hellwig <hch@infradead.org>; Duyck, Alexander H > <alexander.h.duyck@intel.com> > Cc: Wang, Liang-min <liang-min.wang@intel.com>; > alex.williamson@redhat.com; linux-kernel@vger.kernel.org; Kirsher, Jeffrey T > <jeffrey.t.kirsher@intel.com>; kvm@vger.kernel.org; bhelgaas@google.com; > linux-pci@vger.kernel.org > Subject: Re: [PATCH] Enable SR-IOV instantiation through /sys file > > On Sat, 2017-10-28 at 23:16 -0700, Christoph Hellwig wrote: > > On Fri, Oct 27, 2017 at 11:20:41PM +0000, Duyck, Alexander H wrote: > > > > > > I don't see this so much as a security problem per-se. It all depends > > > on the hardware setup. If I recall correctly, there are devices where > > > the PF function doesn't really do much other than act as a bit more > > > heavy-weight VF, and the actual logic is handled by a firmware engine > > > on the device. > > > > Can you cite an example? While those surely could exist in theory, > > I can't think of a practical example. > > I have them, which is why I'm patching the UIO driver to allow num_vfs > to be set. I don't even want to *use* the UIO driver for any purpose > except to make that appear in sysfs. It's all handled in the device. > > (I think we might be able to just give the PF out to a guest as if it > were just another VF, but I don't think we actually *do* that right > now). Under UEFI secure boot environment, kernel puts restrictions on UIO and its derivatives. So, user-space function/driver based upon UIO is no longer working under UEFI secure boot environment. The next viable option is vfio-pci, hence this patch in parallel with UIO work.
On Tue, Oct 24, 2017 at 01:04:26PM -0700, Jeff Kirsher wrote: > From: Liang-Min Wang <liang-min.wang@intel.com> > > When a SR-IOV supported device is bound with vfio-pci, the driver > could not create SR-IOV instance through /sys/bus/pci/devices/... > /sriov_numvfs. This patch re-activates this capability for a PCIe > device that supports SR-IOV and is bound with vfio-pci.ko. > > Signed-off-by: Liang-Min Wang <liang-min.wang@intel.com> Dropping this for now, given Alex W's issues. Please address his concerns and repost as needed. > --- > drivers/vfio/pci/vfio_pci.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c > index f041b1a6cf66..8fbd362607e1 100644 > --- a/drivers/vfio/pci/vfio_pci.c > +++ b/drivers/vfio/pci/vfio_pci.c > @@ -1256,6 +1256,7 @@ static void vfio_pci_remove(struct pci_dev *pdev) > if (!vdev) > return; > > + pci_disable_sriov(pdev); > vfio_iommu_group_put(pdev->dev.iommu_group, &pdev->dev); > kfree(vdev->region); > kfree(vdev); > @@ -1303,12 +1304,23 @@ static const struct pci_error_handlers vfio_err_handlers = { > .error_detected = vfio_pci_aer_err_detected, > }; > > +static int vfio_sriov_configure(struct pci_dev *pdev, int num_vfs) > +{ > + if (!num_vfs) { > + pci_disable_sriov(pdev); > + return 0; > + } > + > + return pci_enable_sriov(pdev, num_vfs); > +} > + > static struct pci_driver vfio_pci_driver = { > .name = "vfio-pci", > .id_table = NULL, /* only dynamic ids */ > .probe = vfio_pci_probe, > .remove = vfio_pci_remove, > .err_handler = &vfio_err_handlers, > + .sriov_configure = vfio_sriov_configure, > }; > > struct vfio_devices { > -- > 2.14.2 >
On Tue, 31 Oct 2017 12:55:20 +0000 "Wang, Liang-min" <liang-min.wang@intel.com> wrote: > > -----Original Message----- > > From: David Woodhouse [mailto:dwmw2@infradead.org] > > Sent: Monday, October 30, 2017 8:39 AM > > To: Christoph Hellwig <hch@infradead.org>; Duyck, Alexander H > > <alexander.h.duyck@intel.com> > > Cc: Wang, Liang-min <liang-min.wang@intel.com>; > > alex.williamson@redhat.com; linux-kernel@vger.kernel.org; Kirsher, Jeffrey T > > <jeffrey.t.kirsher@intel.com>; kvm@vger.kernel.org; bhelgaas@google.com; > > linux-pci@vger.kernel.org > > Subject: Re: [PATCH] Enable SR-IOV instantiation through /sys file > > > > On Sat, 2017-10-28 at 23:16 -0700, Christoph Hellwig wrote: > > > On Fri, Oct 27, 2017 at 11:20:41PM +0000, Duyck, Alexander H wrote: > > > > > > > > I don't see this so much as a security problem per-se. It all depends > > > > on the hardware setup. If I recall correctly, there are devices where > > > > the PF function doesn't really do much other than act as a bit more > > > > heavy-weight VF, and the actual logic is handled by a firmware engine > > > > on the device. > > > > > > Can you cite an example? While those surely could exist in theory, > > > I can't think of a practical example. > > > > I have them, which is why I'm patching the UIO driver to allow num_vfs > > to be set. I don't even want to *use* the UIO driver for any purpose > > except to make that appear in sysfs. It's all handled in the device. > > > > (I think we might be able to just give the PF out to a guest as if it > > were just another VF, but I don't think we actually *do* that right > > now). > > Under UEFI secure boot environment, kernel puts restrictions on UIO and its derivatives. > So, user-space function/driver based upon UIO is no longer working under UEFI secure > boot environment. The next viable option is vfio-pci, hence this patch in parallel with > UIO work. If you want a PF driver that does nothing other than allow SR-IOV to be enabled, doesn't that scream pci-stub? Trying to overlay it onto a driver that also allows userspace driver access to that device seems like the worst idea. Thanks, Alex
On Mon, Nov 6, 2017 at 3:27 PM, Alex Williamson <alex.williamson@redhat.com> wrote: > On Tue, 31 Oct 2017 12:55:20 +0000 > "Wang, Liang-min" <liang-min.wang@intel.com> wrote: > >> > -----Original Message----- >> > From: David Woodhouse [mailto:dwmw2@infradead.org] >> > Sent: Monday, October 30, 2017 8:39 AM >> > To: Christoph Hellwig <hch@infradead.org>; Duyck, Alexander H >> > <alexander.h.duyck@intel.com> >> > Cc: Wang, Liang-min <liang-min.wang@intel.com>; >> > alex.williamson@redhat.com; linux-kernel@vger.kernel.org; Kirsher, Jeffrey T >> > <jeffrey.t.kirsher@intel.com>; kvm@vger.kernel.org; bhelgaas@google.com; >> > linux-pci@vger.kernel.org >> > Subject: Re: [PATCH] Enable SR-IOV instantiation through /sys file >> > >> > On Sat, 2017-10-28 at 23:16 -0700, Christoph Hellwig wrote: >> > > On Fri, Oct 27, 2017 at 11:20:41PM +0000, Duyck, Alexander H wrote: >> > > > >> > > > I don't see this so much as a security problem per-se. It all depends >> > > > on the hardware setup. If I recall correctly, there are devices where >> > > > the PF function doesn't really do much other than act as a bit more >> > > > heavy-weight VF, and the actual logic is handled by a firmware engine >> > > > on the device. >> > > >> > > Can you cite an example? While those surely could exist in theory, >> > > I can't think of a practical example. >> > >> > I have them, which is why I'm patching the UIO driver to allow num_vfs >> > to be set. I don't even want to *use* the UIO driver for any purpose >> > except to make that appear in sysfs. It's all handled in the device. >> > >> > (I think we might be able to just give the PF out to a guest as if it >> > were just another VF, but I don't think we actually *do* that right >> > now). >> >> Under UEFI secure boot environment, kernel puts restrictions on UIO and its derivatives. >> So, user-space function/driver based upon UIO is no longer working under UEFI secure >> boot environment. The next viable option is vfio-pci, hence this patch in parallel with >> UIO work. > > If you want a PF driver that does nothing other than allow SR-IOV to be > enabled, doesn't that scream pci-stub? Trying to overlay it onto a > driver that also allows userspace driver access to that device seems > like the worst idea. Thanks, > > Alex So for the case where a user-space agent is running the actual PF how is it you suggest we go about quarantining the interfaces? What kind of behavior is it you are expecting to see? Should we look into a way to clear match_driver for the VF interfaces so the drivers won't load on them at all, or are you expecting something like vfio-pci to be selected to auto-load on them since the PF is already running that? Thanks. - Alexander
On Mon, 6 Nov 2017 15:47:41 -0800 Alexander Duyck <alexander.duyck@gmail.com> wrote: > On Mon, Nov 6, 2017 at 3:27 PM, Alex Williamson > <alex.williamson@redhat.com> wrote: > > On Tue, 31 Oct 2017 12:55:20 +0000 > > "Wang, Liang-min" <liang-min.wang@intel.com> wrote: > > > >> > -----Original Message----- > >> > From: David Woodhouse [mailto:dwmw2@infradead.org] > >> > Sent: Monday, October 30, 2017 8:39 AM > >> > To: Christoph Hellwig <hch@infradead.org>; Duyck, Alexander H > >> > <alexander.h.duyck@intel.com> > >> > Cc: Wang, Liang-min <liang-min.wang@intel.com>; > >> > alex.williamson@redhat.com; linux-kernel@vger.kernel.org; Kirsher, Jeffrey T > >> > <jeffrey.t.kirsher@intel.com>; kvm@vger.kernel.org; bhelgaas@google.com; > >> > linux-pci@vger.kernel.org > >> > Subject: Re: [PATCH] Enable SR-IOV instantiation through /sys file > >> > > >> > On Sat, 2017-10-28 at 23:16 -0700, Christoph Hellwig wrote: > >> > > On Fri, Oct 27, 2017 at 11:20:41PM +0000, Duyck, Alexander H wrote: > >> > > > > >> > > > I don't see this so much as a security problem per-se. It all depends > >> > > > on the hardware setup. If I recall correctly, there are devices where > >> > > > the PF function doesn't really do much other than act as a bit more > >> > > > heavy-weight VF, and the actual logic is handled by a firmware engine > >> > > > on the device. > >> > > > >> > > Can you cite an example? While those surely could exist in theory, > >> > > I can't think of a practical example. > >> > > >> > I have them, which is why I'm patching the UIO driver to allow num_vfs > >> > to be set. I don't even want to *use* the UIO driver for any purpose > >> > except to make that appear in sysfs. It's all handled in the device. > >> > > >> > (I think we might be able to just give the PF out to a guest as if it > >> > were just another VF, but I don't think we actually *do* that right > >> > now). > >> > >> Under UEFI secure boot environment, kernel puts restrictions on UIO and its derivatives. > >> So, user-space function/driver based upon UIO is no longer working under UEFI secure > >> boot environment. The next viable option is vfio-pci, hence this patch in parallel with > >> UIO work. > > > > If you want a PF driver that does nothing other than allow SR-IOV to be > > enabled, doesn't that scream pci-stub? Trying to overlay it onto a > > driver that also allows userspace driver access to that device seems > > like the worst idea. Thanks, > > > > Alex > > So for the case where a user-space agent is running the actual PF how > is it you suggest we go about quarantining the interfaces? What kind > of behavior is it you are expecting to see? Should we look into a way > to clear match_driver for the VF interfaces so the drivers won't load > on them at all, or are you expecting something like vfio-pci to be > selected to auto-load on them since the PF is already running that? Disabling driver auto probing would be a good start. I expect we don't want to disallow the admin from binding VFs to host drivers, but perhaps we should taint the host if they do. I don't think anyone wants to sign-up to support a device in the host that is potentially compromised by an unknown, possibly proprietary, userspace PF driver. Some work needs to be done on managing the VF enablement life cycle, for instance can VFs be enabled while the user is already using the PF? We need some sort of signaling and handshake if the user driver allows concurrent changes, blocking otherwise. Also how does the user being able to reset the PF affect the situation? That needs to be evaluated and handled. Thanks, Alex
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c index f041b1a6cf66..8fbd362607e1 100644 --- a/drivers/vfio/pci/vfio_pci.c +++ b/drivers/vfio/pci/vfio_pci.c @@ -1256,6 +1256,7 @@ static void vfio_pci_remove(struct pci_dev *pdev) if (!vdev) return; + pci_disable_sriov(pdev); vfio_iommu_group_put(pdev->dev.iommu_group, &pdev->dev); kfree(vdev->region); kfree(vdev); @@ -1303,12 +1304,23 @@ static const struct pci_error_handlers vfio_err_handlers = { .error_detected = vfio_pci_aer_err_detected, }; +static int vfio_sriov_configure(struct pci_dev *pdev, int num_vfs) +{ + if (!num_vfs) { + pci_disable_sriov(pdev); + return 0; + } + + return pci_enable_sriov(pdev, num_vfs); +} + static struct pci_driver vfio_pci_driver = { .name = "vfio-pci", .id_table = NULL, /* only dynamic ids */ .probe = vfio_pci_probe, .remove = vfio_pci_remove, .err_handler = &vfio_err_handlers, + .sriov_configure = vfio_sriov_configure, }; struct vfio_devices {