passthrough: quarantine PCI devices

When a PCI device is assigned to an untrusted domain, it is possible for
that domain to program the device to DMA to an arbitrary address. The
IOMMU is used to protect the host from malicious DMA by making sure that
the device addresses can only target memory assigned to the guest. However,
when the guest domain is torn down the device is assigned back to dom0,
thus allowing any in-flight DMA to potentially target critical host data.

This patch introduces a 'quarantine' for PCI devices using dom_io. When
the toolstack makes a device assignable (by binding it to pciback), it
will now also assign it to DOMID_IO and the device will only be assigned
back to dom0 when the device is made unassignable again. Whilst device is
assignable it will only ever transfer between dom_io and guest domains.
dom_io is actually only used as a sentinel domain for quarantining purposes;
it is not configured with any IOMMU mappings. Assignment to dom_io simply
means that the device's initiator (requestor) identifier is not present in
the IOMMU's device table and thus any DMA transactions issued will be
terminated with a fault condition.

In addition, a fix to assignment handling is made for VT-d.  Failure
during the assignment step should not lead to a device still being
associated with its prior owner. Hand the device to DomIO temporarily,
until the assignment step has completed successfully.  Remove the PI
hooks from the source domain then earlier as well.

Failure of the recovery reassign_device_ownership() may not go silent:
There e.g. may still be left over RMRR mappings in the domain assignment
to which has failed, and hence we can't allow that domain to continue
executing.

NOTE: This patch also includes one printk() cleanup; the
      "XEN_DOMCTL_assign_device: " tag is dropped in iommu_do_pci_domctl(),
      since similar printk()-s elsewhere also don't log such a tag.

This is XSA-302.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index b5444d15523aad651402eb5c04e91d0440d546fc..2ccab033b4607e92bc9c2660271754cabfefbff4 100644 (file)
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -766,6 +766,7 @@ static int libxl__device_pci_assignable_add(libxl__gc *gc,
                                             libxl_device_pci *pcidev,
                                             int rebind)
 {
+    libxl_ctx *ctx = libxl__gc_owner(gc);
     unsigned dom, bus, dev, func;
     char *spath, *driver_path = NULL;
     int rc;
@@ -791,7 +792,7 @@ static int libxl__device_pci_assignable_add(libxl__gc *gc,
     }
     if ( rc ) {
         LOG(WARN, PCI_BDF" already assigned to pciback", dom, bus, dev, func);
-        return 0;
+        goto quarantine;
     }
 
     /* Check to see if there's already a driver that we need to unbind from */
@@ -822,6 +823,19 @@ static int libxl__device_pci_assignable_add(libxl__gc *gc,
         return ERROR_FAIL;
     }
 
+quarantine:
+    /*
+     * DOMID_IO is just a sentinel domain, without any actual mappings,
+     * so always pass XEN_DOMCTL_DEV_RDM_RELAXED to avoid assignment being
+     * unnecessarily denied.
+     */
+    rc = xc_assign_device(ctx->xch, DOMID_IO, pcidev_encode_bdf(pcidev),
+                          XEN_DOMCTL_DEV_RDM_RELAXED);
+    if ( rc < 0 ) {
+        LOG(ERROR, "failed to quarantine "PCI_BDF, dom, bus, dev, func);
+        return ERROR_FAIL;
+    }
+
     return 0;
 }
 
@@ -829,9 +843,18 @@ static int libxl__device_pci_assignable_remove(libxl__gc *gc,
                                                libxl_device_pci *pcidev,
                                                int rebind)
 {
+    libxl_ctx *ctx = libxl__gc_owner(gc);
     int rc;
     char *driver_path;
 
+    /* De-quarantine */
+    rc = xc_deassign_device(ctx->xch, DOMID_IO, pcidev_encode_bdf(pcidev));
+    if ( rc < 0 ) {
+        LOG(ERROR, "failed to de-quarantine "PCI_BDF, pcidev->domain, pcidev->bus,
+            pcidev->dev, pcidev->func);
+        return ERROR_FAIL;
+    }
+
     /* Unbind from pciback */
     if ( (rc=pciback_dev_is_assigned(gc, pcidev)) < 0 ) {
         return ERROR_FAIL;

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 3addb54ebbbeb8c9c681dab1f745cf31724b0cac..611116c7fcafb70f87e850b098d06e494dded7f5 100644 (file)
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -548,6 +548,7 @@ void __init setup_system_domains(void)
      * Initialise our DOMID_IO domain.
      * This domain owns I/O pages that are within the range of the page_info
      * array. Mappings occur at the priv of the caller.
+     * Quarantined PCI devices will be associated with this domain.
      */
     dom_io = domain_create(DOMID_IO, NULL, false);
     if ( IS_ERR(dom_io) )

diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index d597a09f98793028335a1dbabc37d3d9e4537e7c..03d022603979c1e6771362b64d392b6248460de8 100644 (file)
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -383,6 +383,16 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
 
     switch ( op->cmd )
     {
+    case XEN_DOMCTL_assign_device:
+    case XEN_DOMCTL_deassign_device:
+        if ( op->domain == DOMID_IO )
+        {
+            d = dom_io;
+            break;
+        }
+        else if ( op->domain == DOMID_INVALID )
+            return -ESRCH;
+        /* fall through */
     case XEN_DOMCTL_test_assign_device:
     case XEN_DOMCTL_vm_event_op:
         if ( op->domain == DOMID_INVALID )
@@ -405,7 +415,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
 
     if ( !domctl_lock_acquire() )
     {
-        if ( d )
+        if ( d && d != dom_io )
             rcu_unlock_domain(d);
         return hypercall_create_continuation(
             __HYPERVISOR_domctl, "h", u_domctl);
@@ -1064,7 +1074,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
     domctl_lock_release();
 
  domctl_out_unlock_domonly:
-    if ( d )
+    if ( d && d != dom_io )
         rcu_unlock_domain(d);
 
     if ( copyback && __copy_to_guest(u_domctl, op, 1) )

diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index 053e7355c42bb8bfcd7e766f1a1840f67c5fccba..b2046245d7908b48aab48a2ec43ffd91921b6566 100644 (file)
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -95,6 +95,10 @@ static void amd_iommu_setup_domain_device(
     u8 bus = pdev->bus;
     const struct domain_iommu *hd = dom_iommu(domain);
 
+    /* dom_io is used as a sentinel for quarantined devices */
+    if ( domain == dom_io )
+        return;
+
     BUG_ON( !hd->arch.root_table || !hd->arch.paging_mode ||
             !iommu->dev_table.buffer );
 
@@ -276,6 +280,10 @@ static void amd_iommu_disable_domain_device(const struct domain *domain,
     int req_id;
     u8 bus = pdev->bus;
 
+    /* dom_io is used as a sentinel for quarantined devices */
+    if ( domain == dom_io )
+        return;
+
     BUG_ON ( iommu->dev_table.buffer == NULL );
     req_id = get_dma_requestor_id(iommu->seg, PCI_BDF2(bus, devfn));
     table = iommu->dev_table.buffer;
@@ -373,7 +381,7 @@ static int amd_iommu_assign_device(struct domain *d, u8 devfn,
             ivrs_mappings[req_id].read_permission);
     }
 
-    return reassign_device(hardware_domain, d, devfn, pdev);
+    return reassign_device(pdev->domain, d, devfn, pdev);
 }
 
 static void deallocate_next_page_table(struct page_info *pg, int level)

diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index 27e265b936ce7b52bc78d6cd3e10bc7b804c92bb..999b831d900a1c21ff7842d82827925f4f214eec 100644 (file)
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -245,6 +245,9 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
             break;
         }
 
+        if ( d == dom_io )
+            return -EINVAL;
+
         ret = iommu_add_dt_device(dev);
         /*
          * Ignore "-EEXIST" error code as it would mean that the device is
@@ -283,6 +286,9 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
 
         ret = xsm_deassign_dtdevice(XSM_HOOK, d, dt_node_full_name(dev));
 
+        if ( d == dom_io )
+            return -EINVAL;
+
         ret = iommu_deassign_dt_device(d, dev);
 
         if ( ret )

diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 9ef5f376c5b4d796a8a1e69cc120f4cca95fc0a9..8cbe908fff3318f78f51d1d22983fbe3adcc19ed 100644 (file)
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -181,7 +181,7 @@ int iommu_domain_init(struct domain *d, unsigned int opts)
 
     hd->platform_ops = iommu_get_ops();
     ret = hd->platform_ops->init(d);
-    if ( ret )
+    if ( ret || is_system_domain(d) )
         return ret;
 
     if ( is_hardware_domain(d) )
@@ -473,6 +473,10 @@ int __init iommu_setup(void)
     }
     else
     {
+        dom_io->options |= XEN_DOMCTL_CDF_iommu;
+        if ( iommu_domain_init(dom_io, 0) )
+            panic("Could not set up quarantine\n");
+
         printk(" - Dom0 mode: %s\n",
                iommu_hwdom_passthrough ? "Passthrough" :
                iommu_hwdom_strict ? "Strict" : "Relaxed");

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index bdcc482d819b64c31b8607f74df18f327418c44a..18a7dc7224db7ef64717eba171359f4fe31cfff7 100644 (file)
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -910,6 +910,7 @@ static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
 {
     const struct domain_iommu *hd = dom_iommu(d);
     struct pci_dev *pdev;
+    struct domain *target;
     int ret = 0;
 
     if ( !is_iommu_enabled(d) )
@@ -920,12 +921,16 @@ static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
     if ( !pdev )
         return -ENODEV;
 
+    /* De-assignment from dom_io should de-quarantine the device */
+    target = (pdev->quarantine && pdev->domain != dom_io) ?
+        dom_io : hardware_domain;
+
     while ( pdev->phantom_stride )
     {
         devfn += pdev->phantom_stride;
         if ( PCI_SLOT(devfn) != PCI_SLOT(pdev->devfn) )
             break;
-        ret = hd->platform_ops->reassign_device(d, hardware_domain, devfn,
+        ret = hd->platform_ops->reassign_device(d, target, devfn,
                                                 pci_to_dev(pdev));
         if ( !ret )
             continue;
@@ -936,7 +941,7 @@ static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
     }
 
     devfn = pdev->devfn;
-    ret = hd->platform_ops->reassign_device(d, hardware_domain, devfn,
+    ret = hd->platform_ops->reassign_device(d, target, devfn,
                                             pci_to_dev(pdev));
     if ( ret )
     {
@@ -946,6 +951,9 @@ static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
         return ret;
     }
 
+    if ( pdev->domain == hardware_domain  )
+        pdev->quarantine = false;
+
     pdev->fault.count = 0;
 
     return ret;
@@ -1462,19 +1470,29 @@ static int iommu_remove_device(struct pci_dev *pdev)
     return hd->platform_ops->remove_device(pdev->devfn, pci_to_dev(pdev));
 }
 
-/*
- * If the device isn't owned by the hardware domain, it means it already
- * has been assigned to other domain, or it doesn't exist.
- */
 static int device_assigned(u16 seg, u8 bus, u8 devfn)
 {
     struct pci_dev *pdev;
+    int rc = 0;
 
     pcidevs_lock();
-    pdev = pci_get_pdev_by_domain(hardware_domain, seg, bus, devfn);
+
+    pdev = pci_get_pdev(seg, bus, devfn);
+
+    if ( !pdev )
+        rc = -ENODEV;
+    /*
+     * If the device exists and it is not owned by either the hardware
+     * domain or dom_io then it must be assigned to a guest, or be
+     * hidden (owned by dom_xen).
+     */
+    else if ( pdev->domain != hardware_domain &&
+              pdev->domain != dom_io )
+        rc = -EBUSY;
+
     pcidevs_unlock();
 
-    return pdev ? 0 : -EBUSY;
+    return rc;
 }
 
 static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
@@ -1488,7 +1506,8 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
 
     /* Prevent device assign if mem paging or mem sharing have been 
      * enabled for this domain */
-    if ( unlikely((is_hvm_domain(d) &&
+    if ( d != dom_io &&
+         unlikely((is_hvm_domain(d) &&
                    d->arch.hvm.mem_sharing_enabled) ||
                   vm_event_check_ring(d->vm_event_paging) ||
                   p2m_get_hostp2m(d)->global_logdirty) )
@@ -1497,12 +1516,20 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
     if ( !pcidevs_trylock() )
         return -ERESTART;
 
-    pdev = pci_get_pdev_by_domain(hardware_domain, seg, bus, devfn);
+    pdev = pci_get_pdev(seg, bus, devfn);
+
+    rc = -ENODEV;
     if ( !pdev )
-    {
-        rc = pci_get_pdev(seg, bus, devfn) ? -EBUSY : -ENODEV;
         goto done;
-    }
+
+    rc = 0;
+    if ( d == pdev->domain )
+        goto done;
+
+    rc = -EBUSY;
+    if ( pdev->domain != hardware_domain &&
+         pdev->domain != dom_io )
+        goto done;
 
     if ( pdev->msix )
     {
@@ -1530,6 +1557,10 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
     }
 
  done:
+    /* The device is assigned to dom_io so mark it as quarantined */
+    if ( !rc && d == dom_io )
+        pdev->quarantine = true;
+
     pcidevs_unlock();
 
     return rc;
@@ -1705,7 +1736,7 @@ int iommu_do_pci_domctl(
             ret = hypercall_create_continuation(__HYPERVISOR_domctl,
                                                 "h", u_domctl);
         else if ( ret )
-            printk(XENLOG_G_ERR "XEN_DOMCTL_assign_device: "
+            printk(XENLOG_G_ERR
                    "assign %04x:%02x:%02x.%u to dom%d failed (%d)\n",
                    seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
                    d->domain_id, ret);

diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 4a759d33cdb4ae780cf39f26c998888f656e9497..68e7f5fb58123d929fa69ee6a0a21ab6d717d808 100644 (file)
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1289,6 +1289,10 @@ int domain_context_mapping_one(
     int agaw, rc, ret;
     bool_t flush_dev_iotlb;
 
+    /* dom_io is used as a sentinel for quarantined devices */
+    if ( domain == dom_io )
+        return 0;
+
     ASSERT(pcidevs_locked());
     spin_lock(&iommu->lock);
     maddr = bus_to_context_maddr(iommu, bus);
@@ -1535,6 +1539,10 @@ int domain_context_unmap_one(
     int iommu_domid, rc, ret;
     bool_t flush_dev_iotlb;
 
+    /* dom_io is used as a sentinel for quarantined devices */
+    if ( domain == dom_io )
+        return 0;
+
     ASSERT(pcidevs_locked());
     spin_lock(&iommu->lock);
 
@@ -1667,6 +1675,10 @@ static int domain_context_unmap(struct domain *domain, u8 devfn,
         goto out;
     }
 
+    /* dom_io is used as a sentinel for quarantined devices */
+    if ( domain == dom_io )
+        goto out;
+
     /*
      * if no other devices under the same iommu owned by this domain,
      * clear iommu in iommu_bitmap and clear domain_id in domid_bitmp
@@ -2388,6 +2400,15 @@ static int reassign_device_ownership(
     if ( ret )
         return ret;
 
+    if ( devfn == pdev->devfn && pdev->domain != dom_io )
+    {
+        list_move(&pdev->domain_list, &dom_io->pdev_list);
+        pdev->domain = dom_io;
+    }
+
+    if ( !has_arch_pdevs(source) )
+        vmx_pi_hooks_deassign(source);
+
     if ( !has_arch_pdevs(target) )
         vmx_pi_hooks_assign(target);
 
@@ -2400,21 +2421,19 @@ static int reassign_device_ownership(
         return ret;
     }
 
-    if ( devfn == pdev->devfn )
+    if ( devfn == pdev->devfn && pdev->domain != target )
     {
         list_move(&pdev->domain_list, &target->pdev_list);
         pdev->domain = target;
     }
 
-    if ( !has_arch_pdevs(source) )
-        vmx_pi_hooks_deassign(source);
-
     return ret;
 }
 
 static int intel_iommu_assign_device(
     struct domain *d, u8 devfn, struct pci_dev *pdev, u32 flag)
 {
+    struct domain *s = pdev->domain;
     struct acpi_rmrr_unit *rmrr;
     int ret = 0, i;
     u16 bdf, seg;
@@ -2457,8 +2476,8 @@ static int intel_iommu_assign_device(
         }
     }
 
-    ret = reassign_device_ownership(hardware_domain, d, devfn, pdev);
-    if ( ret )
+    ret = reassign_device_ownership(s, d, devfn, pdev);
+    if ( ret || d == dom_io )
         return ret;
 
     /* Setup rmrr identity mapping */
@@ -2471,11 +2490,20 @@ static int intel_iommu_assign_device(
             ret = rmrr_identity_mapping(d, 1, rmrr, flag);
             if ( ret )
             {
-                reassign_device_ownership(d, hardware_domain, devfn, pdev);
+                int rc;
+
+                rc = reassign_device_ownership(d, s, devfn, pdev);
                 printk(XENLOG_G_ERR VTDPREFIX
                        " cannot map reserved region (%"PRIx64",%"PRIx64"] for Dom%d (%d)\n",
                        rmrr->base_address, rmrr->end_address,
                        d->domain_id, ret);
+                if ( rc )
+                {
+                    printk(XENLOG_ERR VTDPREFIX
+                           " failed to reclaim %04x:%02x:%02x.%u from %pd (%d)\n",
+                           seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), d, rc);
+                    domain_crash(d);
+                }
                 break;
             }
         }

diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 393cb45de347cd74748c1346f419b8bff44c7b69..2bc4aaf4530cb09e6575534271167363347eacc8 100644 (file)
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -99,6 +99,9 @@ struct pci_dev {
 
     nodeid_t node; /* NUMA node */
 
+    /* Device to be quarantined, don't automatically re-assign to dom0 */
+    bool quarantine;
+
     /* Device with errata, ignore the BARs. */
     bool ignore_bars;