Re: [Xen-devel] [PATCH v6 11/11] xen, common: do not implicitly permit access to mapped I/O memory

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.04.14 at 15:45, <avanzini.arianna@xxxxxxxxx> wrote:
>  tools/libxl/libxl_create.c | 17 +++++++++++++++++
>  tools/libxl/libxl_pci.c    | 24 ++++++++++++++++++------

I doubt that doing it at this layer is correct: There are quite a few uses
of xc_domain_memory_mapping() in qemu, so I think as a first step
you should retain libxc functionality unchanged by issuing both domctls
there. If that ever needs further separation, that would then be a
second patch.

> --- 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;
> +            }
> +        }

Considering that this code isn't being moved here - what's the
rationale for the sudden need to special case VGA pass-through
here?

> @@ -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 this whole thing gets unified, treating I/O ports and MMIO differently
isn't an optimal choice.

> --- 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) )

The former check should now be pointless here, and ought to be done
in the XEN_DOMCTL_iomem_permission handler instead (in fact it
should always have got done there, and that would be a security issue
if there wasn't the waiver put in place by XSA-77).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel