RE: [PATCH] vfio/pci: Fix SR-IOV VF handling with MMIO blocking

[Wang, Haiyue]

> -----Original Message-----
> From: Alex Williamson <alex.williamson@xxxxxxxxxx>
> Sent: Sunday, June 28, 2020 12:09
> To: Wang, Haiyue <haiyue.wang@xxxxxxxxx>
> Cc: kvm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; maxime.coquelin@xxxxxxxxxx; David Marchand
> <david.marchand@xxxxxxxxxx>; Kevin Traynor <ktraynor@xxxxxxxxxx>
> 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@xxxxxxxxx> wrote:
> 
> > > -----Original Message-----
> > > From: kvm-owner@xxxxxxxxxxxxxxx <kvm-owner@xxxxxxxxxxxxxxx> On Behalf Of Alex Williamson
> > > Sent: Friday, June 26, 2020 00:57
> > > To: alex.williamson@xxxxxxxxxx
> > > Cc: kvm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; maxime.coquelin@xxxxxxxxxx
> > > 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@xxxxxxxxxx>
> > > ---
> > >  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)
> >