[PATCH] Enable SR-IOV instantiation through /sys file

[PATCH] Enable SR-IOV instantiation through /sys file

[Jeff Kirsher]

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(+)

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

Re: [PATCH] Enable SR-IOV instantiation through /sys file

[Alex Williamson]

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 {

RE: [PATCH] Enable SR-IOV instantiation through /sys file

[Wang, Liang-min]

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 {

Re: [PATCH] Enable SR-IOV instantiation through /sys file

[Alex Williamson]

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

RE: [PATCH] Enable SR-IOV instantiation through /sys file

[Wang, Liang-min]

> -----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

Re: [PATCH] Enable SR-IOV instantiation through /sys file

[Alex Williamson]

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

RE: [PATCH] Enable SR-IOV instantiation through /sys file

[Wang, Liang-min]

> -----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.

Re: [PATCH] Enable SR-IOV instantiation through /sys file

[Alex Williamson]

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

RE: [PATCH] Enable SR-IOV instantiation through /sys file

[Wang, Liang-min]

> -----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

Re: [PATCH] Enable SR-IOV instantiation through /sys file

[Duyck, Alexander H]

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)

Re: [PATCH] Enable SR-IOV instantiation through /sys file

[Duyck, Alexander H]

T24gU2F0LCAyMDE3LTEwLTI4IGF0IDAwOjE5ICswMjAwLCBBbGV4IFdpbGxpYW1zb24gd3JvdGU6
DQo+IE9uIEZyaSwgMjcgT2N0IDIwMTcgMjE6NTA6NDMgKzAwMDANCj4gIldhbmcsIExpYW5nLW1p
biIgPGxpYW5nLW1pbi53YW5nQGludGVsLmNvbT4gd3JvdGU6DQo+IA0KPiA+ID4gLS0tLS1Pcmln
aW5hbCBNZXNzYWdlLS0tLS0NCj4gPiA+IEZyb206IEFsZXggV2lsbGlhbXNvbiBbbWFpbHRvOmFs
ZXgud2lsbGlhbXNvbkByZWRoYXQuY29tXQ0KPiA+ID4gU2VudDogVHVlc2RheSwgT2N0b2JlciAy
NCwgMjAxNyA2OjA3IFBNDQo+ID4gPiBUbzogV2FuZywgTGlhbmctbWluIDxsaWFuZy1taW4ud2Fu
Z0BpbnRlbC5jb20+DQo+ID4gPiBDYzogS2lyc2hlciwgSmVmZnJleSBUIDxqZWZmcmV5LnQua2ly
c2hlckBpbnRlbC5jb20+OyBrdm1Admdlci5rZXJuZWwub3JnOw0KPiA+ID4gbGludXgtcGNpQHZn
ZXIua2VybmVsLm9yZzsgbGludXgta2VybmVsQHZnZXIua2VybmVsLm9yZzsNCj4gPiA+IGJoZWxn
YWFzQGdvb2dsZS5jb207IER1eWNrLCBBbGV4YW5kZXIgSCA8YWxleGFuZGVyLmguZHV5Y2tAaW50
ZWwuY29tPg0KPiA+ID4gU3ViamVjdDogUmU6IFtQQVRDSF0gRW5hYmxlIFNSLUlPViBpbnN0YW50
aWF0aW9uIHRocm91Z2ggL3N5cyBmaWxlDQo+ID4gPiANCj4gPiA+IE9uIFR1ZSwgMjQgT2N0IDIw
MTcgMjE6NDk6MTUgKzAwMDANCj4gPiA+ICJXYW5nLCBMaWFuZy1taW4iIDxsaWFuZy1taW4ud2Fu
Z0BpbnRlbC5jb20+IHdyb3RlOg0KPiA+ID4gICANCj4gPiA+ID4gSnVzdCBsaWtlIGFueSBQQ0ll
IGRldmljZXMgdGhhdCBzdXBwb3J0cyBTUi1JT1YuIFRoZXJlIGFyZSByZXN0cmljdGlvbnMgc2V0
IGZvciAgDQo+ID4gPiANCj4gPiA+IFZGLiBBbHNvLCB0aGVyZSBpcyBhIGNvbmNlcHQgb2YgdHJ1
c3QgVkYgbm93IGF2YWlsYWJsZSBmb3IgUEYgdG8gbWFuYWdlIGNlcnRhaW4NCj4gPiA+IGZlYXR1
cmVzIHRoYXQgb25seSBzZWxlY3RlZCBWRiBjb3VsZCBleGVyY2lzZS4gQXJlIHlvdSBzYXlpbmcg
YWxsIHRoZSBkZXZpY2VzDQo+ID4gPiBzdXBwb3J0aW5nIFNSLUlPViBhbGwgaGF2ZSBzZWN1cml0
eSBpc3N1ZT8NCj4gPiA+IA0KPiA+ID4gSGVyZSdzIGEgc2ltcGxlIGV4YW1wbGUsIG1vc3QgU1It
SU9WIGNhcGFibGUgTklDcywgaW5jbHVkaW5nIHRob3NlIGZyb20NCj4gPiA+IEludGVsLCByZXF1
aXJlIHRoZSBQRiBpbnRlcmZhY2UgdG8gYmUgdXAgaW4gb3JkZXIgdG8gcm91dGUgdHJhZmZpYyBm
cm9tDQo+ID4gPiB0aGUgVkYuICBJZiB0aGUgdXNlciBjb250cm9scyB0aGUgUEYgaW50ZXJmYWNl
IGFuZCBWRnMgYXJlIHVzZWQNCj4gPiA+IGVsc2V3aGVyZSBpbiB0aGUgaG9zdCwgdGhlIFBGIGRy
aXZlciBpbiB1c2Vyc3BhY2UgY2FuIGluZHVjZSBhIGRlbmlhbA0KPiA+ID4gb2Ygc2VydmljZSBv
biB0aGUgVkZzLiAgVGhhdCBkb2Vzbid0IGV2ZW4gdGFrZSBpbnRvIGFjY291bnQgdGhhdCBWRnMN
Cj4gPiA+IG1pZ2h0IGJlIGluIHNlcGFyYXRlIElPTU1VIGdyb3VwcyBmcm9tIHRoZSBQRiBhbmQg
dGhlcmVmb3JlIG5vdA0KPiA+ID4gaXNvbGF0ZWQgZnJvbSB0aGUgaG9zdCBsaWtlIHRoZSBQRiBh
bmQgdGhhdCB0aGUgUEYgZHJpdmVyIGNhbg0KPiA+ID4gcG90ZW50aWFsbHkgbWFuaXB1bGF0ZSB0
aGUgVkYsIHBvc3NpYmx5IHBlcmZvcm1pbmcgRE1BIG9uIGJlaGFsZiBvZiB0aGUNCj4gPiA+IFBG
LiAgVkZzIGFyZSBvbmx5IGNvbnNpZGVyZWQgc2VjdXJlIHRvZGF5IGJlY2F1c2UgdGhlIFBGIGlz
IG1hbmFnZWQgYnkNCj4gPiA+IGEgZHJpdmVyIGluIHRoZSBob3N0IGtlcm5lbC4gIEFsbG93aW5n
IHNpbXBsZSBlbmFibGVtZW50IG9mIFZGcyBmb3IgYQ0KPiA+ID4gdXNlciBvd25lZCBQRiBzZWVt
cyBpbmhlcmVudGx5IGluc2VjdXJlIHRvIG1lLiAgVGhhbmtzLA0KPiA+ID4gDQo+ID4gPiBBbGV4
ICANCj4gPiANCj4gPiBGaXJzdGx5LCB0aGUgY29uY2VybiBpcyBvbiB1c2VyLXNwYWNlIFBGIGRy
aXZlciBiYXNlZCB1cG9uIHZmaW8tcGNpLCB0aGlzIHBhdGNoIGRvZXNuJ3QNCj4gPiBjaGFuZ2Ug
UEYgYmVoYXZpb3Igc28gd2l0aC93aXRob3V0IHRoaXMgcGF0Y2gsIHRoZSBjb25jZXJuIHJlbWFp
bnMgdGhlIHNhbWUuDQo+IA0KPiBUaGlzIHBhdGNoIGVuYWJsZXMgU1ItSU9WIHRvIGJlIGVuYWJs
ZWQgdmlhIHRoZSBob3N0IG9uIGEgdXNlci1vd25lZA0KPiBQRiwgaG93IGlzIHRoaXMgbm90IGEg
Y2hhbmdlIGluIGJlaGF2aW9yPw0KPiANCj4gPiBTZWNvbmRseSwgdGhlIHNlY3VyaXR5IGNvbmNl
cm4gKGluY2x1ZGluZyBkZW5pYWwgb2Ygc2VydmljZSkgaW4gZ2VuZXJhbCBpcyB0byBlbnN1cmUg
dHJ1c3QNCj4gPiBlbnRpdHkgdG8gYmUgdHJ1c3Qtd29ydGh5LiBObyBtYXR0ZXIgdGhlIFBGIGRy
aXZlciBpcyBpbiBrZXJuZWwtc3BhY2Ugb3IgaW4gdXNlci0gc3BhY2UsDQo+ID4gbmVjZXNzYXJ5
IG1lY2hhbmlzbSBuZWVkcyB0byBiZSBlbmZvcmNlZCBvbiB0aGUgZGV2aWNlIGRyaXZlciB0byBl
bnN1cmUgaXQncw0KPiA+IHRydXN0ZWQgd29ydGh5LiBGb3IgZXhhbXBsZSwgaXhnYmUga2VybmVs
IGRyaXZlciBpbnRyb2R1Y2VzIGEgVHggaGFuZyBkZXRlY3Rpb24NCj4gPiB0byBhdm9pZCBkcml2
ZXIgc3RheXMgaW4gYSBiYWQgc3RhdGUuIFRoZXJlZm9yZSwgaXQncyB0aGUgcmVzcG9uc2liaWxp
dHkgb2YgdXNlci1zcGFjZQ0KPiA+IGRyaXZlciBmdW5jdGlvbiwgd2hpY2ggYmFzZWQgdXBvbiB2
ZmlvLXBjaSwgdG8gZW5mb3JjZSBuZWNlc3NhcnkgbWVjaGFuaXNtIHRvIGVuc3VyZQ0KPiA+IGl0
cyB0cnVzdC1uZXNzLiBUaGF0J3MgYSBnaXZlbi4NCj4gDQo+IFVzZXJzcGFjZSBpcyBub3QgdHJ1
c3R3b3J0aHksIHRoZXJlZm9yZSB0aGUgaG9zdCBrZXJuZWwgY2Fubm90IHBsYWNlDQo+IHJlc3Bv
bnNpYmlsaXR5IG9uIGEgdXNlcnNwYWNlIGRyaXZlciBmb3IgYW55dGhpbmcsIGluY2x1ZGluZyB0
aGUNCj4gYmVoYXZpb3Igb2YgVkZzLiAgSSdtIHNvcnJ5LCBidXQgaXQncyBhIE5BSyB1bmxlc3Mg
eW91IGludGVuZCB0bw0KPiBmb2xsb3ctdXAgd2l0aCBzb21lIHByb3Bvc2FsIHRvIHF1YXJhbnRp
bmUgdGhlIFZGcyBlbmFibGVkIGJ5IHRoZQ0KPiB1c2Vyc3BhY2UgUEYgZHJpdmVyLiAgVGhhbmtz
LA0KPiANCj4gQWxleA0KDQpJIGRvbid0IHNlZSB0aGlzIHNvIG11Y2ggYXMgYSBzZWN1cml0eSBw
cm9ibGVtIHBlci1zZS4gSXQgYWxsIGRlcGVuZHMNCm9uIHRoZSBoYXJkd2FyZSBzZXR1cC4gSWYg
SSByZWNhbGwgY29ycmVjdGx5LCB0aGVyZSBhcmUgZGV2aWNlcyB3aGVyZQ0KdGhlIFBGIGZ1bmN0
aW9uIGRvZXNuJ3QgcmVhbGx5IGRvIG11Y2ggb3RoZXIgdGhhbiBhY3QgYXMgYSBiaXQgbW9yZQ0K
aGVhdnktd2VpZ2h0IFZGLCBhbmQgdGhlIGFjdHVhbCBsb2dpYyBpcyBoYW5kbGVkIGJ5IGEgZmly
bXdhcmUgZW5naW5lDQpvbiB0aGUgZGV2aWNlLiBUaGUgb25seSByZWFsIGlzc3VlIGlzIHRoYXQg
Zm9yIGRldmljZXMgbGlrZSB0aGUgSW50ZWwNCk5JQ3MgaW5zdGVhZCBvZiB0cnVzdGluZyBhIGZp
cm13YXJlIGVuZ2luZSB3ZSBoYXZlIGhpc3RvcmljYWxseSB1c2VkIGENCmtlcm5lbCBkcml2ZXIg
YW5kIG5vdyB3ZSBhcmUgd2FudGluZyB0byB0cnVzdCBhIHVzZXItc3BhY2UgYWdlbnQNCmluc3Rl
YWQuDQoNCkkgZG8gdGhpbmsgdGhhdCB3ZSBwcm9iYWJseSBuZWVkIHRvIGhhdmUgc29tZSBzb3J0
IG9mIHNpZ25hbGluZyBiZXR3ZWVuDQp1c2VyLXNwYWNlIGFuZCB2ZmlvLXBjaSB0aGF0IHdvdWxk
IGFsbG93IGZvciBub3RpZnlpbmcgdGhlIHVzZXItc3BhY2UNCm9mIHRoZSBjaGFuZ2UgYW5kIGZv
ciB1c2VyLXNwYWNlIHRvIG5vdGlmeSB2ZmlvLXBjaSB0aGF0IGl0IGlzIGNhcGFibGUNCm9mIGhh
bmRsaW5nIHRoZSBub3RpZmljYXRpb24uIFRoaXMgaXMgc29tZXRoaW5nIHRoYXQgY2FuIGJlIHRv
Z2dsZWQgYXQNCmFueSB0aW1lIGFmdGVyIGFsbCBhbmQgbm90IGFsbCBkZXZpY2VzIGhhdmUgYSBt
ZWFucyBvZiBub3RpZnlpbmcgdGhlIFBGDQp0aGF0IHRoaXMgaGFzIGJlZW4gY2hhbmdlZC4NCg0K
QmV5b25kIHRoYXQgb25jZSB0aGUgcm9vdCB1c2VyIGVuYWJsZXMgdGhlIFZGcyBJIHdvdWxkIGtp
bmQgb2YgdGhpbmsNCnRoZXkga25vdyB3aGF0IGRyaXZlciB0aGV5IGhhdmUgcnVubmluZyB0aGVt
LiBFbmFibGluZyBWRnMgaW1wbGllcyB0aGUNCnJvb3QgdXNlciB0cnVzdHMgdGhlIGFwcGxpY2F0
aW9uIHJ1bm5pbmcgb24gdG9wIG9mIHZmaW8tcGNpIHRvIGhhbmRsZQ0KdGhlIFBGIHJlc3BvbnNp
Ymx5LiBBdCBsZWFzdCB0aGF0IGlzIGhvdyBpdCB3b3JrcyBpbiBteSBtaW5kLg0KDQpUaGFua3Mu
DQoNCi0gQWxleGFuZGVyDQogICh1c2luZyBmdWxsIG5hbWUgc2luY2UgMiBBbGV4cyBpbiBvbmUg
dGhyZWFkIGNhbiBiZSBjb25mdXNpbmcpDQoNCg==

Re: [PATCH] Enable SR-IOV instantiation through /sys file

[Christoph Hellwig]

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?

Re: [PATCH] Enable SR-IOV instantiation through /sys file

[Alexander Duyck]

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.

Re: [PATCH] Enable SR-IOV instantiation through /sys file

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 936 bytes --]

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).

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 4938 bytes --]

RE: [PATCH] Enable SR-IOV instantiation through /sys file

[Wang, Liang-min]

> -----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.

RE: [PATCH] Enable SR-IOV instantiation through /sys file

[Wang, Liang-min]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogRGF2aWQgV29vZGhvdXNl
IFttYWlsdG86ZHdtdzJAaW5mcmFkZWFkLm9yZ10NCj4gU2VudDogTW9uZGF5LCBPY3RvYmVyIDMw
LCAyMDE3IDg6MzkgQU0NCj4gVG86IENocmlzdG9waCBIZWxsd2lnIDxoY2hAaW5mcmFkZWFkLm9y
Zz47IER1eWNrLCBBbGV4YW5kZXIgSA0KPiA8YWxleGFuZGVyLmguZHV5Y2tAaW50ZWwuY29tPg0K
PiBDYzogV2FuZywgTGlhbmctbWluIDxsaWFuZy1taW4ud2FuZ0BpbnRlbC5jb20+Ow0KPiBhbGV4
LndpbGxpYW1zb25AcmVkaGF0LmNvbTsgbGludXgta2VybmVsQHZnZXIua2VybmVsLm9yZzsgS2ly
c2hlciwgSmVmZnJleSBUDQo+IDxqZWZmcmV5LnQua2lyc2hlckBpbnRlbC5jb20+OyBrdm1Admdl
ci5rZXJuZWwub3JnOyBiaGVsZ2Fhc0Bnb29nbGUuY29tOw0KPiBsaW51eC1wY2lAdmdlci5rZXJu
ZWwub3JnDQo+IFN1YmplY3Q6IFJlOiBbUEFUQ0hdIEVuYWJsZSBTUi1JT1YgaW5zdGFudGlhdGlv
biB0aHJvdWdoIC9zeXMgZmlsZQ0KPiANCj4gT24gU2F0LCAyMDE3LTEwLTI4IGF0IDIzOjE2IC0w
NzAwLCBDaHJpc3RvcGggSGVsbHdpZyB3cm90ZToNCj4gPiBPbiBGcmksIE9jdCAyNywgMjAxNyBh
dCAxMToyMDo0MVBNICswMDAwLCBEdXljaywgQWxleGFuZGVyIEggd3JvdGU6DQo+ID4gPg0KPiA+
ID4gSSBkb24ndCBzZWUgdGhpcyBzbyBtdWNoIGFzIGEgc2VjdXJpdHkgcHJvYmxlbSBwZXItc2Uu
IEl0IGFsbCBkZXBlbmRzDQo+ID4gPiBvbiB0aGUgaGFyZHdhcmUgc2V0dXAuIElmIEkgcmVjYWxs
IGNvcnJlY3RseSwgdGhlcmUgYXJlIGRldmljZXMgd2hlcmUNCj4gPiA+IHRoZSBQRiBmdW5jdGlv
biBkb2Vzbid0IHJlYWxseSBkbyBtdWNoIG90aGVyIHRoYW4gYWN0IGFzIGEgYml0IG1vcmUNCj4g
PiA+IGhlYXZ5LXdlaWdodCBWRiwgYW5kIHRoZSBhY3R1YWwgbG9naWMgaXMgaGFuZGxlZCBieSBh
IGZpcm13YXJlIGVuZ2luZQ0KPiA+ID4gb24gdGhlIGRldmljZS4NCj4gPg0KPiA+IENhbiB5b3Ug
Y2l0ZSBhbiBleGFtcGxlP8KgwqBXaGlsZSB0aG9zZSBzdXJlbHkgY291bGQgZXhpc3QgaW4gdGhl
b3J5LA0KPiA+IEkgY2FuJ3QgdGhpbmsgb2YgYSBwcmFjdGljYWwgZXhhbXBsZS4NCj4gDQo+IEkg
aGF2ZSB0aGVtLCB3aGljaCBpcyB3aHkgSSdtIHBhdGNoaW5nIHRoZSBVSU8gZHJpdmVyIHRvIGFs
bG93IG51bV92ZnMNCj4gdG8gYmUgc2V0LiBJIGRvbid0IGV2ZW4gd2FudCB0byAqdXNlKiB0aGUg
VUlPIGRyaXZlciBmb3IgYW55IHB1cnBvc2UNCj4gZXhjZXB0IHRvIG1ha2UgdGhhdCBhcHBlYXIg
aW4gc3lzZnMuIEl0J3MgYWxsIGhhbmRsZWQgaW4gdGhlIGRldmljZS4NCj4gDQo+IChJIHRoaW5r
IHdlIG1pZ2h0IGJlIGFibGUgdG8ganVzdCBnaXZlIHRoZSBQRiBvdXQgdG8gYSBndWVzdCBhcyBp
ZiBpdA0KPiB3ZXJlIGp1c3QgYW5vdGhlciBWRiwgYnV0IEkgZG9uJ3QgdGhpbmsgd2UgYWN0dWFs
bHkgKmRvKiB0aGF0IHJpZ2h0DQo+IG5vdykuDQoNClVuZGVyIFVFRkkgc2VjdXJlIGJvb3QgZW52
aXJvbm1lbnQsIGtlcm5lbCBwdXRzIHJlc3RyaWN0aW9ucyBvbiBVSU8gYW5kIGl0cyBkZXJpdmF0
aXZlcy4NClNvLCB1c2VyLXNwYWNlIGZ1bmN0aW9uL2RyaXZlciBiYXNlZCB1cG9uIFVJTyBpcyBu
byBsb25nZXIgd29ya2luZyB1bmRlciBVRUZJIHNlY3VyZQ0KYm9vdCBlbnZpcm9ubWVudC4gVGhl
IG5leHQgdmlhYmxlIG9wdGlvbiBpcyB2ZmlvLXBjaSwgaGVuY2UgdGhpcyBwYXRjaCBpbiBwYXJh
bGxlbCB3aXRoDQpVSU8gd29yay4NCg==

Re: [PATCH] Enable SR-IOV instantiation through /sys file

[Bjorn Helgaas]

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
> 

Re: [PATCH] Enable SR-IOV instantiation through /sys file

[Alex Williamson]

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

Re: [PATCH] Enable SR-IOV instantiation through /sys file

[Alexander Duyck]

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

Re: [PATCH] Enable SR-IOV instantiation through /sys file

[Alex Williamson]

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