Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: wh6vsxY6pasuiMjCPqayP3Wu0heqvWs/6dPmd5k5DAsUtywQDB0vFiSQLwvM6YgMQUI4Sg1jic
 NZkAeL8/GvmA==
IronPort-SDR: +KPFkKeH2aWFV7wYMgIPp/ALvsPKkhm5ksRdVG5D1QepdcDTGQ4XogEQyf4vNmjsO79s620F9o
 yOkF4m+jGKMA==
From:   "Wang, Haiyue" <haiyue.wang@intel.com>
To:     Alex Williamson <alex.williamson@redhat.com>
CC:     "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "maxime.coquelin@redhat.com" <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
Thread-Topic: [PATCH] vfio/pci: Fix SR-IOV VF handling with MMIO blocking
Thread-Index: AQHWSxHG3MhsyYc5LU2HHyb99u21najtWeJwgAATpACAAACcoA==
Date:   Sun, 28 Jun 2020 04:22:23 +0000
Message-ID: <BN8PR11MB379514FADAAA03FC8A63F3FDF7910@BN8PR11MB3795.namprd11.prod.outlook.com>
References: <159310421505.27590.16617666489295503039.stgit@gimli.home>
        <BN8PR11MB3795C3338B3C7F67A6EDB43AF7910@BN8PR11MB3795.namprd11.prod.outlook.com>
 <20200627220852.13b3fa7f@x1.home>
In-Reply-To: <20200627220852.13b3fa7f@x1.home>
Accept-Language: en-US
Content-Language: en-US
dlp-product: dlpe-windows
dlp-reaction: no-action
dlp-version: 11.5.1.3
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 6b4d6ae7-708e-4e47-3989-08d81b1adb57
X-MS-Exchange-CrossTenant-originalarrivaltime: 28 Jun 2020 04:22:23.5311
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: PkP8mACLFPR6XJORe/vD+yswvv1NnE41K7ipFYtDlonQaHLn8UtbQqmVznd1G4RQnUgLJFZR+HEz6vqfe/lu4g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN8PR11MB3634
X-OriginatorOrg: intel.com
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> -----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@re=
dhat.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
>=20
> On Sun, 28 Jun 2020 03:12:12 +0000
> "Wang, Haiyue" <haiyue.wang@intel.com> wrote:
>=20
> > > -----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.coqueli=
n@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 acces=
s 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/vf=
io_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, i=
nt 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 =3D vdev->pdev;
> > >  	u16 cmd =3D 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 t=
ry to disable the
> > memory access command (even it is VF) then access the memory to trigger=
 CVE-2020-12888 ?
>=20
> 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=3D1836244,
I thought it should also prevent the bad Guest VM, thanks for sharing the b=
ackground more.

BR,
Haiyue

> device?  Thanks,
>=20
> Alex
>=20
> > >  }
> > >
> > >  /*
> > > @@ -1728,6 +1734,15 @@ int vfio_config_init(struct vfio_pci_device *v=
dev)
> > >  				 vconfig[PCI_INTERRUPT_PIN]);
> > >
> > >  		vconfig[PCI_INTERRUPT_PIN] =3D 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] |=3D
> > > +					cpu_to_le16(PCI_COMMAND_MEMORY);
> > >  	}
> > >
> > >  	if (!IS_ENABLED(CONFIG_VFIO_PCI_INTX) || vdev->nointx)
> >