Don't allow the hardware domain to access the PCI config space of
devices not assigned to it. Ie: the config space of iommu devices
in use by Xen should not be accessible to the hardware domain.
Note that access from the hardware domain to config space regions
where Xen hasn't detected any devices is still allowed.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
xen/drivers/vpci/vpci.c | 31 +++++++++++++++++++++++++++----
1 file changed, 27 insertions(+), 4 deletions(-)
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 758d9420e7..761aa40f99 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -319,7 +319,21 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size)
/* Find the PCI dev matching the address. */
pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.devfn);
if ( !pdev )
+ {
+ pcidevs_lock();
+ pdev = pci_get_pdev(sbdf.seg, sbdf.bus, sbdf.devfn);
+ pcidevs_unlock();
+ if ( pdev )
+ /* Drop reads to devices not assigned to the domain. */
+ return data;
+
+ /*
+ * Let the hardware domain access config space regions for non-existent
+ * devices.
+ * TODO: revisit for domU support.
+ */
return vpci_read_hw(sbdf, reg, size);
+ }
spin_lock(&pdev->vpci->lock);
@@ -418,13 +432,22 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size,
return;
}
- /*
- * Find the PCI dev matching the address.
- * Passthrough everything that's not trapped.
- */
+ /* Find the PCI dev matching the address. */
pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.devfn);
if ( !pdev )
{
+ pcidevs_lock();
+ pdev = pci_get_pdev(sbdf.seg, sbdf.bus, sbdf.devfn);
+ pcidevs_unlock();
+ if ( pdev )
+ /* Ignore writes to devices not assigned to the domain. */
+ return;
+
+ /*
+ * Let the hardware domain access config space regions for non-existent
+ * devices.
+ * TODO: revisit for domU support.
+ */
vpci_write_hw(sbdf, reg, size, data);
return;
}
--
2.22.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-develOn 02.09.2019 13:30, Roger Pau Monne wrote:
> Don't allow the hardware domain to access the PCI config space of
> devices not assigned to it. Ie: the config space of iommu devices
> in use by Xen should not be accessible to the hardware domain.
Well, I agree with what you say above, but the code change disallows
much more than this. In particular Dom0 (and maybe stub domains too)
need to be able to access the config space of devices assigned to
guests, e.g. for qemu to control MSI and/or MSI-X.
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -319,7 +319,21 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size)
> /* Find the PCI dev matching the address. */
> pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.devfn);
> if ( !pdev )
> + {
> + pcidevs_lock();
> + pdev = pci_get_pdev(sbdf.seg, sbdf.bus, sbdf.devfn);
> + pcidevs_unlock();
The locking here points out a pre-existing issue: While
pci_get_pdev_by_domain() doesn't check that the pcidevs lock is
being held, it really should. It not doing so is (I guess) because
VT-d code too looks to be violating this. Kevin - thoughts?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On Mon, Sep 02, 2019 at 01:58:07PM +0200, Jan Beulich wrote: > On 02.09.2019 13:30, Roger Pau Monne wrote: > > Don't allow the hardware domain to access the PCI config space of > > devices not assigned to it. Ie: the config space of iommu devices > > in use by Xen should not be accessible to the hardware domain. > > Well, I agree with what you say above, but the code change disallows > much more than this. In particular Dom0 (and maybe stub domains too) > need to be able to access the config space of devices assigned to > guests, e.g. for qemu to control MSI and/or MSI-X. Right, I was overlooking the fact that a domain using vPCI itself should be able to handle passthrough backends for other domains. I think the condition should instead check if the device is assigned to dom_xen, and don't allow domains access to devices assigned to dom_xen. Thanks, Roger. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On 02.09.2019 15:58, Roger Pau Monné wrote: > On Mon, Sep 02, 2019 at 01:58:07PM +0200, Jan Beulich wrote: >> On 02.09.2019 13:30, Roger Pau Monne wrote: >>> Don't allow the hardware domain to access the PCI config space of >>> devices not assigned to it. Ie: the config space of iommu devices >>> in use by Xen should not be accessible to the hardware domain. >> >> Well, I agree with what you say above, but the code change disallows >> much more than this. In particular Dom0 (and maybe stub domains too) >> need to be able to access the config space of devices assigned to >> guests, e.g. for qemu to control MSI and/or MSI-X. > > Right, I was overlooking the fact that a domain using vPCI itself > should be able to handle passthrough backends for other domains. > > I think the condition should instead check if the device is assigned > to dom_xen, and don't allow domains access to devices assigned to > dom_xen. Even that goes too far imo: We deliberately allow read access to r/o devices, in order to avoid anomalies in bus enumeration in Dom0. And I'd very much hope write attempts already honor the pseg->ro_map bit for a device. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On Mon, Sep 02, 2019 at 04:15:02PM +0200, Jan Beulich wrote: > On 02.09.2019 15:58, Roger Pau Monné wrote: > > On Mon, Sep 02, 2019 at 01:58:07PM +0200, Jan Beulich wrote: > >> On 02.09.2019 13:30, Roger Pau Monne wrote: > >>> Don't allow the hardware domain to access the PCI config space of > >>> devices not assigned to it. Ie: the config space of iommu devices > >>> in use by Xen should not be accessible to the hardware domain. > >> > >> Well, I agree with what you say above, but the code change disallows > >> much more than this. In particular Dom0 (and maybe stub domains too) > >> need to be able to access the config space of devices assigned to > >> guests, e.g. for qemu to control MSI and/or MSI-X. > > > > Right, I was overlooking the fact that a domain using vPCI itself > > should be able to handle passthrough backends for other domains. > > > > I think the condition should instead check if the device is assigned > > to dom_xen, and don't allow domains access to devices assigned to > > dom_xen. > > Even that goes too far imo: We deliberately allow read access to > r/o devices, in order to avoid anomalies in bus enumeration in > Dom0. And I'd very much hope write attempts already honor the > pseg->ro_map bit for a device. Hm, no, AFAICT vPCI was just bypassing the ro_map ATM. So the problem I found, and that I was trying to address with this patch is that a PVH dom0 on AMD hardware finds the iommus by scanning the PCI bus, and a Linux dom0 seems to immediately turn off the MSI enable control bit on any devices it finds, thus leaving the iommu without being able to generate interrupts. I can implement the RO stuff, but it seems weird to me. AFAICT the only devices owned by Xen should be the serial console, the iommu and the HPET maybe. How can hiding those cause anomalies in bus enumeration? Thanks, Roger. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On 02.09.2019 16:23, Roger Pau Monné wrote: > So the problem I found, and that I was trying to address with this > patch is that a PVH dom0 on AMD hardware finds the iommus by scanning > the PCI bus, and a Linux dom0 seems to immediately turn off the MSI > enable control bit on any devices it finds, thus leaving the iommu > without being able to generate interrupts. > > I can implement the RO stuff, but it seems weird to me. AFAICT the > only devices owned by Xen should be the serial console, the iommu and > the HPET maybe. How can hiding those cause anomalies in bus > enumeration? Both the serial device and an IOMMU may in principle be func 0 of a multi-function device. By fully hiding such devices, you also hide funcs 1-7 afaict. Furthermore, from a tech support pov it seems rather desirable to have e.g. lspci output in Dom0 to be as complete as possible. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
> From: Jan Beulich [mailto:jbeulich@suse.com]
> Sent: Monday, September 2, 2019 7:58 PM
>
> On 02.09.2019 13:30, Roger Pau Monne wrote:
> > Don't allow the hardware domain to access the PCI config space of
> > devices not assigned to it. Ie: the config space of iommu devices
> > in use by Xen should not be accessible to the hardware domain.
>
> Well, I agree with what you say above, but the code change disallows
> much more than this. In particular Dom0 (and maybe stub domains too)
> need to be able to access the config space of devices assigned to
> guests, e.g. for qemu to control MSI and/or MSI-X.
>
> > --- a/xen/drivers/vpci/vpci.c
> > +++ b/xen/drivers/vpci/vpci.c
> > @@ -319,7 +319,21 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int
> reg, unsigned int size)
> > /* Find the PCI dev matching the address. */
> > pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.devfn);
> > if ( !pdev )
> > + {
> > + pcidevs_lock();
> > + pdev = pci_get_pdev(sbdf.seg, sbdf.bus, sbdf.devfn);
> > + pcidevs_unlock();
>
> The locking here points out a pre-existing issue: While
> pci_get_pdev_by_domain() doesn't check that the pcidevs lock is
> being held, it really should. It not doing so is (I guess) because
> VT-d code too looks to be violating this. Kevin - thoughts?
>
It's used by addr_to_dma_page_maddr, while the latter is used
in many code paths. Some of them already holds the lock while
others don't. Instead of introducing trickiness in those paths, I
think this may be fixed in an easier way - just removing the
invocation of pci_get_pdev_by_domain. The only purpose of
current usage is to find the related DRHD, and then do NUMA-
aware page table allocation. It's needless to repeat finding DRHD
here. We can just record it when assign_device happens.
Let's tweak a fix for it.
Thanks
Kevin
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
© 2016 - 2024 Red Hat, Inc.