> -----Original Message-----
> From: Alex Williamson <alex.william...@redhat.com>
> Sent: Sunday, June 28, 2020 12:09
> To: Wang, Haiyue <haiyue.w...@intel.com>
> Cc: k...@vger.kernel.org; linux-kernel@vger.kernel.org;
> maxime.coque...@redhat.com; David Marchand
> <david.march...@redhat.com>; Kevin Traynor <ktray...@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.w...@intel.com> wrote:
>
> > > -----Original Message-----
> > > From: kvm-ow...@vger.kernel.org <kvm-ow...@vger.kernel.org> On Behalf Of
> > > Alex Williamson
> > > Sent: Friday, June 26, 2020 00:57
> > > To: alex.william...@redhat.com
> > > Cc: k...@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > maxime.coque...@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.william...@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)
> >
