[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

[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

[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