KVM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] vfio/pci: Fix SR-IOV VF handling with MMIO blocking
@ 2020-06-25 16:57 Alex Williamson
  2020-06-28  3:12 ` Wang, Haiyue
  0 siblings, 1 reply; 4+ messages in thread
From: Alex Williamson @ 2020-06-25 16:57 UTC (permalink / raw)
  To: alex.williamson; +Cc: kvm, linux-kernel, maxime.coquelin

SR-IOV VFs do not implement the memory enable bit of the command
register, therefore this bit is not set in config space after
pci_enable_device().  This leads to an unintended difference
between PF and VF in hand-off state to the user.  We can correct
this by setting the initial value of the memory enable bit in our
virtualized config space.  There's really no need however to
ever fault a user on a VF though as this would only indicate an
error in the user's management of the enable bit, versus a PF
where the same access could trigger hardware faults.

Fixes: abafbc551fdd ("vfio-pci: Invalidate mmaps and block MMIO access on disabled memory")
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 drivers/vfio/pci/vfio_pci_config.c |   17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
index 8746c943247a..d98843feddce 100644
--- a/drivers/vfio/pci/vfio_pci_config.c
+++ b/drivers/vfio/pci/vfio_pci_config.c
@@ -398,9 +398,15 @@ static inline void p_setd(struct perm_bits *p, int off, u32 virt, u32 write)
 /* Caller should hold memory_lock semaphore */
 bool __vfio_pci_memory_enabled(struct vfio_pci_device *vdev)
 {
+	struct pci_dev *pdev = vdev->pdev;
 	u16 cmd = le16_to_cpu(*(__le16 *)&vdev->vconfig[PCI_COMMAND]);
 
-	return cmd & PCI_COMMAND_MEMORY;
+	/*
+	 * SR-IOV VF memory enable is handled by the MSE bit in the
+	 * PF SR-IOV capability, there's therefore no need to trigger
+	 * faults based on the virtual value.
+	 */
+	return pdev->is_virtfn || (cmd & PCI_COMMAND_MEMORY);
 }
 
 /*
@@ -1728,6 +1734,15 @@ int vfio_config_init(struct vfio_pci_device *vdev)
 				 vconfig[PCI_INTERRUPT_PIN]);
 
 		vconfig[PCI_INTERRUPT_PIN] = 0; /* Gratuitous for good VFs */
+
+		/*
+		 * VFs do no implement the memory enable bit of the COMMAND
+		 * register therefore we'll not have it set in our initial
+		 * copy of config space after pci_enable_device().  For
+		 * consistency with PFs, set the virtual enable bit here.
+		 */
+		*(__le16 *)&vconfig[PCI_COMMAND] |=
+					cpu_to_le16(PCI_COMMAND_MEMORY);
 	}
 
 	if (!IS_ENABLED(CONFIG_VFIO_PCI_INTX) || vdev->nointx)


^ permalink raw reply	[flat|nested] 4+ messages in thread

* RE: [PATCH] vfio/pci: Fix SR-IOV VF handling with MMIO blocking
  2020-06-25 16:57 [PATCH] vfio/pci: Fix SR-IOV VF handling with MMIO blocking Alex Williamson
@ 2020-06-28  3:12 ` Wang, Haiyue
  2020-06-28  4:08   ` Alex Williamson
  0 siblings, 1 reply; 4+ messages in thread
From: Wang, Haiyue @ 2020-06-28  3:12 UTC (permalink / raw)
  To: Alex Williamson
  Cc: kvm, linux-kernel, maxime.coquelin, David Marchand, Kevin Traynor

> -----Original Message-----
> From: kvm-owner@vger.kernel.org <kvm-owner@vger.kernel.org> On Behalf Of Alex Williamson
> Sent: Friday, June 26, 2020 00:57
> To: alex.williamson@redhat.com
> Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org; maxime.coquelin@redhat.com
> Subject: [PATCH] vfio/pci: Fix SR-IOV VF handling with MMIO blocking
> 
> SR-IOV VFs do not implement the memory enable bit of the command
> register, therefore this bit is not set in config space after
> pci_enable_device().  This leads to an unintended difference
> between PF and VF in hand-off state to the user.  We can correct
> this by setting the initial value of the memory enable bit in our
> virtualized config space.  There's really no need however to
> ever fault a user on a VF though as this would only indicate an
> error in the user's management of the enable bit, versus a PF
> where the same access could trigger hardware faults.
> 
> Fixes: abafbc551fdd ("vfio-pci: Invalidate mmaps and block MMIO access on disabled memory")
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>  drivers/vfio/pci/vfio_pci_config.c |   17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> index 8746c943247a..d98843feddce 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c
> @@ -398,9 +398,15 @@ static inline void p_setd(struct perm_bits *p, int off, u32 virt, u32 write)
>  /* Caller should hold memory_lock semaphore */
>  bool __vfio_pci_memory_enabled(struct vfio_pci_device *vdev)
>  {
> +	struct pci_dev *pdev = vdev->pdev;
>  	u16 cmd = le16_to_cpu(*(__le16 *)&vdev->vconfig[PCI_COMMAND]);
> 
> -	return cmd & PCI_COMMAND_MEMORY;
> +	/*
> +	 * SR-IOV VF memory enable is handled by the MSE bit in the
> +	 * PF SR-IOV capability, there's therefore no need to trigger
> +	 * faults based on the virtual value.
> +	 */
> +	return pdev->is_virtfn || (cmd & PCI_COMMAND_MEMORY);

Hi Alex,

After set up the initial copy of config space for memory enable bit for VF, is it worth
to trigger SIGBUS into the bad user space process which intentionally try to disable the
memory access command (even it is VF) then access the memory to trigger CVE-2020-12888 ?

BR,
Haiyue

>  }
> 
>  /*
> @@ -1728,6 +1734,15 @@ int vfio_config_init(struct vfio_pci_device *vdev)
>  				 vconfig[PCI_INTERRUPT_PIN]);
> 
>  		vconfig[PCI_INTERRUPT_PIN] = 0; /* Gratuitous for good VFs */
> +
> +		/*
> +		 * VFs do no implement the memory enable bit of the COMMAND
> +		 * register therefore we'll not have it set in our initial
> +		 * copy of config space after pci_enable_device().  For
> +		 * consistency with PFs, set the virtual enable bit here.
> +		 */
> +		*(__le16 *)&vconfig[PCI_COMMAND] |=
> +					cpu_to_le16(PCI_COMMAND_MEMORY);
>  	}
> 
>  	if (!IS_ENABLED(CONFIG_VFIO_PCI_INTX) || vdev->nointx)


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] vfio/pci: Fix SR-IOV VF handling with MMIO blocking
  2020-06-28  3:12 ` Wang, Haiyue
@ 2020-06-28  4:08   ` Alex Williamson
  2020-06-28  4:22     ` Wang, Haiyue
  0 siblings, 1 reply; 4+ messages in thread
From: Alex Williamson @ 2020-06-28  4:08 UTC (permalink / raw)
  To: Wang, Haiyue
  Cc: kvm, linux-kernel, maxime.coquelin, David Marchand, Kevin Traynor

On Sun, 28 Jun 2020 03:12:12 +0000
"Wang, Haiyue" <haiyue.wang@intel.com> wrote:

> > -----Original Message-----
> > From: kvm-owner@vger.kernel.org <kvm-owner@vger.kernel.org> On Behalf Of Alex Williamson
> > Sent: Friday, June 26, 2020 00:57
> > To: alex.williamson@redhat.com
> > Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org; maxime.coquelin@redhat.com
> > Subject: [PATCH] vfio/pci: Fix SR-IOV VF handling with MMIO blocking
> > 
> > SR-IOV VFs do not implement the memory enable bit of the command
> > register, therefore this bit is not set in config space after
> > pci_enable_device().  This leads to an unintended difference
> > between PF and VF in hand-off state to the user.  We can correct
> > this by setting the initial value of the memory enable bit in our
> > virtualized config space.  There's really no need however to
> > ever fault a user on a VF though as this would only indicate an
> > error in the user's management of the enable bit, versus a PF
> > where the same access could trigger hardware faults.
> > 
> > Fixes: abafbc551fdd ("vfio-pci: Invalidate mmaps and block MMIO access on disabled memory")
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> >  drivers/vfio/pci/vfio_pci_config.c |   17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> > index 8746c943247a..d98843feddce 100644
> > --- a/drivers/vfio/pci/vfio_pci_config.c
> > +++ b/drivers/vfio/pci/vfio_pci_config.c
> > @@ -398,9 +398,15 @@ static inline void p_setd(struct perm_bits *p, int off, u32 virt, u32 write)
> >  /* Caller should hold memory_lock semaphore */
> >  bool __vfio_pci_memory_enabled(struct vfio_pci_device *vdev)
> >  {
> > +	struct pci_dev *pdev = vdev->pdev;
> >  	u16 cmd = le16_to_cpu(*(__le16 *)&vdev->vconfig[PCI_COMMAND]);
> > 
> > -	return cmd & PCI_COMMAND_MEMORY;
> > +	/*
> > +	 * SR-IOV VF memory enable is handled by the MSE bit in the
> > +	 * PF SR-IOV capability, there's therefore no need to trigger
> > +	 * faults based on the virtual value.
> > +	 */
> > +	return pdev->is_virtfn || (cmd & PCI_COMMAND_MEMORY);  
> 
> Hi Alex,
> 
> After set up the initial copy of config space for memory enable bit for VF, is it worth
> to trigger SIGBUS into the bad user space process which intentionally try to disable the
> memory access command (even it is VF) then access the memory to trigger CVE-2020-12888 ?

We're essentially only trying to catch the user in mismanaging the
enable bit if we trigger a fault based on the virtualized enabled bit,
right?  There's no risk that the VF would trigger a UR based on the
state of our virtual enable bit.  So is it worth triggering a user
fault when, for instance, the user might be aware that the device is a
VF and know that the memory enable bit is not relative to the physical
device?  Thanks,

Alex

> >  }
> > 
> >  /*
> > @@ -1728,6 +1734,15 @@ int vfio_config_init(struct vfio_pci_device *vdev)
> >  				 vconfig[PCI_INTERRUPT_PIN]);
> > 
> >  		vconfig[PCI_INTERRUPT_PIN] = 0; /* Gratuitous for good VFs */
> > +
> > +		/*
> > +		 * VFs do no implement the memory enable bit of the COMMAND
> > +		 * register therefore we'll not have it set in our initial
> > +		 * copy of config space after pci_enable_device().  For
> > +		 * consistency with PFs, set the virtual enable bit here.
> > +		 */
> > +		*(__le16 *)&vconfig[PCI_COMMAND] |=
> > +					cpu_to_le16(PCI_COMMAND_MEMORY);
> >  	}
> > 
> >  	if (!IS_ENABLED(CONFIG_VFIO_PCI_INTX) || vdev->nointx)  
> 


^ permalink raw reply	[flat|nested] 4+ messages in thread

* RE: [PATCH] vfio/pci: Fix SR-IOV VF handling with MMIO blocking
  2020-06-28  4:08   ` Alex Williamson
@ 2020-06-28  4:22     ` Wang, Haiyue
  0 siblings, 0 replies; 4+ messages in thread
From: Wang, Haiyue @ 2020-06-28  4:22 UTC (permalink / raw)
  To: Alex Williamson
  Cc: kvm, linux-kernel, maxime.coquelin, David Marchand, Kevin Traynor

> -----Original Message-----
> From: Alex Williamson <alex.williamson@redhat.com>
> Sent: Sunday, June 28, 2020 12:09
> To: Wang, Haiyue <haiyue.wang@intel.com>
> Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org; maxime.coquelin@redhat.com; David Marchand
> <david.marchand@redhat.com>; Kevin Traynor <ktraynor@redhat.com>
> Subject: Re: [PATCH] vfio/pci: Fix SR-IOV VF handling with MMIO blocking
> 
> On Sun, 28 Jun 2020 03:12:12 +0000
> "Wang, Haiyue" <haiyue.wang@intel.com> wrote:
> 
> > > -----Original Message-----
> > > From: kvm-owner@vger.kernel.org <kvm-owner@vger.kernel.org> On Behalf Of Alex Williamson
> > > Sent: Friday, June 26, 2020 00:57
> > > To: alex.williamson@redhat.com
> > > Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org; maxime.coquelin@redhat.com
> > > Subject: [PATCH] vfio/pci: Fix SR-IOV VF handling with MMIO blocking
> > >
> > > SR-IOV VFs do not implement the memory enable bit of the command
> > > register, therefore this bit is not set in config space after
> > > pci_enable_device().  This leads to an unintended difference
> > > between PF and VF in hand-off state to the user.  We can correct
> > > this by setting the initial value of the memory enable bit in our
> > > virtualized config space.  There's really no need however to
> > > ever fault a user on a VF though as this would only indicate an
> > > error in the user's management of the enable bit, versus a PF
> > > where the same access could trigger hardware faults.
> > >
> > > Fixes: abafbc551fdd ("vfio-pci: Invalidate mmaps and block MMIO access on disabled memory")
> > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > ---
> > >  drivers/vfio/pci/vfio_pci_config.c |   17 ++++++++++++++++-
> > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> > > index 8746c943247a..d98843feddce 100644
> > > --- a/drivers/vfio/pci/vfio_pci_config.c
> > > +++ b/drivers/vfio/pci/vfio_pci_config.c
> > > @@ -398,9 +398,15 @@ static inline void p_setd(struct perm_bits *p, int off, u32 virt, u32 write)
> > >  /* Caller should hold memory_lock semaphore */
> > >  bool __vfio_pci_memory_enabled(struct vfio_pci_device *vdev)
> > >  {
> > > +	struct pci_dev *pdev = vdev->pdev;
> > >  	u16 cmd = le16_to_cpu(*(__le16 *)&vdev->vconfig[PCI_COMMAND]);
> > >
> > > -	return cmd & PCI_COMMAND_MEMORY;
> > > +	/*
> > > +	 * SR-IOV VF memory enable is handled by the MSE bit in the
> > > +	 * PF SR-IOV capability, there's therefore no need to trigger
> > > +	 * faults based on the virtual value.
> > > +	 */
> > > +	return pdev->is_virtfn || (cmd & PCI_COMMAND_MEMORY);
> >
> > Hi Alex,
> >
> > After set up the initial copy of config space for memory enable bit for VF, is it worth
> > to trigger SIGBUS into the bad user space process which intentionally try to disable the
> > memory access command (even it is VF) then access the memory to trigger CVE-2020-12888 ?
> 
> We're essentially only trying to catch the user in mismanaging the
> enable bit if we trigger a fault based on the virtualized enabled bit,
> right?  There's no risk that the VF would trigger a UR based on the
> state of our virtual enable bit.  So is it worth triggering a user
> fault when, for instance, the user might be aware that the device is a
> VF and know that the memory enable bit is not relative to the physical

Emm .. I read the CVE attack description from: https://bugzilla.redhat.com/show_bug.cgi?id=1836244,
I thought it should also prevent the bad Guest VM, thanks for sharing the background more.

BR,
Haiyue

> device?  Thanks,
> 
> Alex
> 
> > >  }
> > >
> > >  /*
> > > @@ -1728,6 +1734,15 @@ int vfio_config_init(struct vfio_pci_device *vdev)
> > >  				 vconfig[PCI_INTERRUPT_PIN]);
> > >
> > >  		vconfig[PCI_INTERRUPT_PIN] = 0; /* Gratuitous for good VFs */
> > > +
> > > +		/*
> > > +		 * VFs do no implement the memory enable bit of the COMMAND
> > > +		 * register therefore we'll not have it set in our initial
> > > +		 * copy of config space after pci_enable_device().  For
> > > +		 * consistency with PFs, set the virtual enable bit here.
> > > +		 */
> > > +		*(__le16 *)&vconfig[PCI_COMMAND] |=
> > > +					cpu_to_le16(PCI_COMMAND_MEMORY);
> > >  	}
> > >
> > >  	if (!IS_ENABLED(CONFIG_VFIO_PCI_INTX) || vdev->nointx)
> >


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-25 16:57 [PATCH] vfio/pci: Fix SR-IOV VF handling with MMIO blocking Alex Williamson
2020-06-28  3:12 ` Wang, Haiyue
2020-06-28  4:08   ` Alex Williamson
2020-06-28  4:22     ` Wang, Haiyue

KVM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kvm/0 kvm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kvm kvm/ https://lore.kernel.org/kvm \
		kvm@vger.kernel.org
	public-inbox-index kvm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.kvm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git