[PATCH v7 10/10] xen/common: do not implicitly permit access to mapped I/O memory

[Arianna Avanzini <avanzini.arianna@gmail.com>]

Currently, the XEN_DOMCTL_memory_mapping hypercall implicitly grants
to a domain access permission to the I/O memory areas mapped in its
guest address space. This conflicts with the presence of a specific
hypercall (XEN_DOMCTL_iomem_permission) used to grant such a permission
to a domain.
This commit attempts to separate the functions of the two hypercalls by
having only the latter be able to permit I/O memory access to a domain,
and the former just performing the mapping after a permissions check.

This commit also attempts to change existing code to be sure to grant
access permission to PCI-related I/O memory ranges (for passthrough of
PCI devices specified in the domain's configuration) and to VGA-related
memory ranges (for VGA passthrough, if gfx_passthru = 1 in the domain
configuration). As of the latter, VGA needs some extra memory ranges to
be mapped with respect to PCI; the access to those memory ranges was
previously given implicitly when calling xc_domain_memory_mapping, while
now it must be explicitly granted.

Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
Cc: Dario Faggioli <dario.faggioli@citrix.com>
Cc: Paolo Valente <paolo.valente@unimore.it>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Julien Grall <julien.grall@citrix.com>
Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Eric Trudeau <etrudeau@broadcom.com>
Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>

---

    v7:
        - Let iomem_permission check if the calling domain is allowed to access
          memory ranges to be mapped to a domain. Remove such a check from the
          memory_mapping hypercall.
        - Do not handle I/O ports and I/O memory differently when allowing
          to a domain to access a PCI device.
        - Change the construct used by libxl during PCI-related initialization
          from a switch to an if to better suit the new execution flow.

---
 tools/libxl/libxl_create.c | 17 ++++++++++++++++
 tools/libxl/libxl_pci.c    | 26 +++++++++--------------
 xen/common/domctl.c        | 51 +++++++++++++++++-----------------------------
 3 files changed, 46 insertions(+), 48 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 4de0fb2..e544bbf 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1149,6 +1149,23 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
             libxl__spawn_stub_dm(egc, &dcs->dmss);
         else
             libxl__spawn_local_dm(egc, &dcs->dmss.dm);
+
+        /*
+         * If VGA passthru is enabled by domain config, be sure that the
+         * domain can access VGA-related iomem regions.
+         */
+        if (d_config->b_info.u.hvm.gfx_passthru.val) {
+            uint64_t vga_iomem_start = 0xa0000 >> XC_PAGE_SHIFT;
+            ret = xc_domain_iomem_permission(CTX->xch, domid,
+                                             vga_iomem_start, 0x20, 1);
+            if (ret < 0) {
+                LOGE(ERROR,
+                     "failed to give dom%d access to iomem range "
+                     "%"PRIx64"-%"PRIx64" for VGA passthru",
+                     domid, vga_iomem_start, (vga_iomem_start + 0x20 - 1));
+                goto error_out;
+            }
+        }
         return;
     }
     case LIBXL_DOMAIN_TYPE_PV:
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 44d0453..032e981 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -846,10 +846,13 @@ static int qemu_pci_add_xenstore(libxl__gc *gc, uint32_t domid,
 static int do_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, int starting)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
+    libxl_domain_type type = libxl__domain_type(gc, domid);
     int rc, hvm = 0;
 
-    switch (libxl__domain_type(gc, domid)) {
-    case LIBXL_DOMAIN_TYPE_HVM:
+    if (type == LIBXL_DOMAIN_TYPE_INVALID)
+        return ERROR_FAIL;
+
+    if (type == LIBXL_DOMAIN_TYPE_HVM) {
         hvm = 1;
         if (libxl__wait_for_device_model_deprecated(gc, domid, "running",
                                          NULL, NULL, NULL) < 0) {
@@ -867,8 +870,7 @@ static int do_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, i
         }
         if ( rc )
             return ERROR_FAIL;
-        break;
-    case LIBXL_DOMAIN_TYPE_PV:
+    }
     {
         char *sysfs_path = libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/resource", pcidev->domain,
                                          pcidev->bus, pcidev->dev, pcidev->func);
@@ -937,10 +939,6 @@ static int do_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, i
                 return ERROR_FAIL;
             }
         }
-        break;
-    }
-    case LIBXL_DOMAIN_TYPE_INVALID:
-        return ERROR_FAIL;
     }
 out:
     if (!libxl_is_stubdom(ctx, domid, NULL)) {
@@ -1166,6 +1164,7 @@ static int do_pci_remove(libxl__gc *gc, uint32_t domid,
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     libxl_device_pci *assigned;
+    libxl_domain_type type = libxl__domain_type(gc, domid);
     int hvm = 0, rc, num;
     int stubdomid = 0;
 
@@ -1181,8 +1180,7 @@ static int do_pci_remove(libxl__gc *gc, uint32_t domid,
     }
 
     rc = ERROR_FAIL;
-    switch (libxl__domain_type(gc, domid)) {
-    case LIBXL_DOMAIN_TYPE_HVM:
+    if (type == LIBXL_DOMAIN_TYPE_HVM) {
         hvm = 1;
         if (libxl__wait_for_device_model_deprecated(gc, domid, "running",
                                          NULL, NULL, NULL) < 0)
@@ -1203,8 +1201,8 @@ static int do_pci_remove(libxl__gc *gc, uint32_t domid,
             rc = ERROR_FAIL;
             goto out_fail;
         }
-        break;
-    case LIBXL_DOMAIN_TYPE_PV:
+    } else if (type != LIBXL_DOMAIN_TYPE_PV)
+        abort();
     {
         char *sysfs_path = libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/resource", pcidev->domain,
                                          pcidev->bus, pcidev->dev, pcidev->func);
@@ -1254,10 +1252,6 @@ skip1:
             }
         }
         fclose(f);
-        break;
-    }
-    default:
-        abort();
     }
 out:
     /* don't do multiple resets while some functions are still passed through */
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 866338b..8ee72eb 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -803,18 +803,22 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
     {
         unsigned long mfn = op->u.iomem_permission.first_mfn;
         unsigned long nr_mfns = op->u.iomem_permission.nr_mfns;
+        unsigned long mfn_end = mfn + nr_mfns - 1;
         int allow = op->u.iomem_permission.allow_access;
 
         ret = -EINVAL;
-        if ( (mfn + nr_mfns - 1) < mfn ) /* wrap? */
+        if ( mfn_end < mfn ) /* wrap? */
             break;
 
-        if ( xsm_iomem_permission(XSM_HOOK, d, mfn, mfn + nr_mfns - 1, allow) )
-            ret = -EPERM;
-        else if ( allow )
-            ret = iomem_permit_access(d, mfn, mfn + nr_mfns - 1);
+        ret = -EPERM;
+        if ( !iomem_access_permitted(current->domain, mfn, mfn_end) ||
+             xsm_iomem_permission(XSM_HOOK, d, mfn, mfn_end, allow) )
+            break;
+
+        if ( allow )
+            ret = iomem_permit_access(d, mfn, mfn_end);
         else
-            ret = iomem_deny_access(d, mfn, mfn + nr_mfns - 1);
+            ret = iomem_deny_access(d, mfn, mfn_end);
 #ifdef CONFIG_X86
         if ( !ret )
             memory_type_changed(d);
@@ -838,7 +842,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
             break;
 
         ret = -EPERM;
-        if ( !iomem_access_permitted(current->domain, mfn, mfn_end) )
+        if ( !iomem_access_permitted(d, mfn, mfn_end) )
             break;
 
         ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn_end, add);
@@ -851,40 +855,23 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
                    "memory_map:add: dom%d gfn=%lx mfn=%lx nr=%lx\n",
                    d->domain_id, gfn, mfn, nr_mfns);
 
-            ret = iomem_permit_access(d, mfn, mfn_end);
-            if ( !ret )
-            {
-                ret = map_mmio_regions(d, gfn, nr_mfns, _mfn(mfn));
-                if ( ret )
-                {
-                    printk(XENLOG_G_WARNING
-                           "memory_map:fail: dom%d gfn=%lx mfn=%lx ret:%ld\n",
-                           d->domain_id, gfn, mfn, ret);
-                    if ( iomem_deny_access(d, mfn, mfn_end) &&
-                         is_hardware_domain(current->domain) )
-                        printk(XENLOG_ERR
-                               "memory_map: failed to deny dom%d access to [%lx,%lx]\n",
-                               d->domain_id, mfn, mfn_end);
-                }
-            }
+            ret = map_mmio_regions(d, gfn, nr_mfns, _mfn(mfn));
+            if ( ret )
+                printk(XENLOG_G_WARNING
+                       "memory_map:fail: dom%d gfn=%lx mfn=%lx ret:%ld\n",
+                       d->domain_id, gfn, mfn, ret);
         }
         else
         {
-            int rc;
-
             printk(XENLOG_G_INFO
                    "memory_map:remove: dom%d gfn=%lx mfn=%lx nr=%lx\n",
                    d->domain_id, gfn, mfn, nr_mfns);
 
-            rc = unmap_mmio_regions(d, gfn, nr_mfns, _mfn(mfn));
-            ret = iomem_deny_access(d, mfn, mfn_end);
-            if ( !ret )
-                ret = rc;
+            ret = unmap_mmio_regions(d, gfn, nr_mfns, _mfn(mfn));
             if ( ret && is_hardware_domain(current->domain) )
                 printk(XENLOG_ERR
-                       "memory_map: error %ld %s dom%d access to [%lx,%lx]\n",
-                       ret, rc ? "removing" : "denying", d->domain_id,
-                       mfn, mfn_end);
+                       "memory_map: error %ld removing dom%d access to [%lx,%lx]\n",
+                       ret, d->domain_id, mfn, mfn_end);
         }
 #ifdef CONFIG_X86
         /* Do this unconditionally to cover errors on above failure paths. */
-- 
1.9.2