From mboxrd@z Thu Jan 1 00:00:00 1970 From: Arianna Avanzini Subject: [PATCH v6 11/11] xen, common: do not implicitly permit access to mapped I/O memory Date: Mon, 21 Apr 2014 15:45:04 +0200 Message-ID: <1398087904-16594-12-git-send-email-avanzini.arianna@gmail.com> References: <1398087904-16594-1-git-send-email-avanzini.arianna@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1398087904-16594-1-git-send-email-avanzini.arianna@gmail.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: xen-devel@lists.xen.org Cc: Ian.Campbell@eu.citrix.com, paolo.valente@unimore.it, keir@xen.org, stefano.stabellini@eu.citrix.com, Ian.Jackson@eu.citrix.com, dario.faggioli@citrix.com, tim@xen.org, julien.grall@citrix.com, etrudeau@broadcom.com, JBeulich@suse.com, avanzini.arianna@gmail.com, viktor.kleinik@globallogic.com List-Id: xen-devel@lists.xenproject.org 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). Signed-off-by: Arianna Avanzini Cc: Dario Faggioli Cc: Paolo Valente Cc: Stefano Stabellini Cc: Julien Grall Cc: Ian Campbell Cc: Jan Beulich Cc: Keir Fraser Cc: Tim Deegan Cc: Ian Jackson Cc: Eric Trudeau Cc: Viktor Kleinik --- tools/libxl/libxl_create.c | 17 +++++++++++++++++ tools/libxl/libxl_pci.c | 24 ++++++++++++++++++------ xen/common/domctl.c | 37 ++++++++++--------------------------- 3 files changed, 45 insertions(+), 33 deletions(-) diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c index cdf03cd..924c2e9 100644 --- a/tools/libxl/libxl_create.c +++ b/tools/libxl/libxl_create.c @@ -1151,6 +1151,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..33dc3ce 100644 --- a/tools/libxl/libxl_pci.c +++ b/tools/libxl/libxl_pci.c @@ -867,7 +867,10 @@ static int do_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, i } if ( rc ) return ERROR_FAIL; - break; + /* + * fall through and let I/O memory permissions for PCI devices be given + * also to HVM domains + */ case LIBXL_DOMAIN_TYPE_PV: { char *sysfs_path = libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/resource", pcidev->domain, @@ -886,14 +889,14 @@ static int do_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, i continue; size = end - start + 1; if (start) { - if (flags & PCI_BAR_IO) { + if ((flags & PCI_BAR_IO) && !hvm) { rc = xc_domain_ioport_permission(ctx->xch, domid, start, size, 1); if (rc < 0) { LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Error: xc_domain_ioport_permission error 0x%llx/0x%llx", start, size); fclose(f); return ERROR_FAIL; } - } else { + } else if (!(flags & PCI_BAR_IO)) { rc = xc_domain_iomem_permission(ctx->xch, domid, start>>XC_PAGE_SHIFT, (size+(XC_PAGE_SIZE-1))>>XC_PAGE_SHIFT, 1); if (rc < 0) { @@ -905,6 +908,9 @@ static int do_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, i } } fclose(f); + if (hvm) + break; + sysfs_path = libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/irq", pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func); f = fopen(sysfs_path, "r"); @@ -1203,7 +1209,10 @@ static int do_pci_remove(libxl__gc *gc, uint32_t domid, rc = ERROR_FAIL; goto out_fail; } - break; + /* + * fall through and let I/O memory permissions for PCI devices be given + * also to HVM domains + */ case LIBXL_DOMAIN_TYPE_PV: { char *sysfs_path = libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/resource", pcidev->domain, @@ -1222,11 +1231,11 @@ static int do_pci_remove(libxl__gc *gc, uint32_t domid, continue; size = end - start + 1; if (start) { - if (flags & PCI_BAR_IO) { + if ((flags & PCI_BAR_IO) && !hvm) { rc = xc_domain_ioport_permission(ctx->xch, domid, start, size, 0); if (rc < 0) LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "xc_domain_ioport_permission error 0x%x/0x%x", start, size); - } else { + } else if (!(flags & PCI_BAR_IO)) { rc = xc_domain_iomem_permission(ctx->xch, domid, start>>XC_PAGE_SHIFT, (size+(XC_PAGE_SIZE-1))>>XC_PAGE_SHIFT, 0); if (rc < 0) @@ -1236,6 +1245,9 @@ static int do_pci_remove(libxl__gc *gc, uint32_t domid, } fclose(f); skip1: + if (hvm) + break; + sysfs_path = libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/irq", pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func); f = fopen(sysfs_path, "r"); diff --git a/xen/common/domctl.c b/xen/common/domctl.c index c550c9f..18b975d 100644 --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -838,7 +838,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) return ret; ret = -EPERM; - if ( !iomem_access_permitted(current->domain, mfn, mfn_end) ) + if ( !iomem_access_permitted(current->domain, mfn, mfn_end) || + !iomem_access_permitted(d, mfn, mfn_end) ) break; ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn_end, add); @@ -851,41 +852,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, gfn_end, 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, gfn_end, 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 tmp_rc = 0; - printk(XENLOG_G_INFO "memory_map: remove: dom%d gfn=%lx mfn=%lx nr=%lx\n", d->domain_id, gfn, mfn, nr_mfns); - tmp_rc = unmap_mmio_regions(d, gfn, gfn_end, mfn); - ret = iomem_deny_access(d, mfn, mfn_end); - if ( !ret ) - ret = tmp_rc; + ret = unmap_mmio_regions(d, gfn, gfn_end, mfn); if ( ret && is_hardware_domain(current->domain) ) printk(XENLOG_ERR - "memory_map: error %ld %s dom%d access to [%lx,%lx]\n", - ret, tmp_rc ? "removing" : "denying", d->domain_id, - mfn, mfn_end); + "memory_map: error %ld removing dom%d mapping " + "to [%lx,%lx]\n", ret, d->domain_id, mfn, mfn_end); } /* Do this unconditionally to cover errors on above failure paths. */ memory_type_changed(d); -- 1.9.1