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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.05.14 at 17:54, <avanzini.arianna@xxxxxxxxx> wrote:
>  tools/libxl/libxl_create.c | 17 ++++++++++++++++
>  tools/libxl/libxl_pci.c    | 26 +++++++++--------------
>  xen/common/domctl.c        | 51 
> +++++++++++++++++-----------------------------

First of all - is it (from a functionality pov) really necessary to mix
tools and hypervisor changes here.

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

While you added some text to the description regarding this change,
it still remains unclear why this is really needed, since no other code
is being removed in its place. So my minimum expectation here would
be for you to point out which code this replaces/duplicates, and why
the original needs to remain in place.

Furthermore (I'm not sure if the same mistake is being done
elsewhere, you may not be the original one to blame for this) I think
it is a mistake to mix up VGA and graphics pass-through: When you
want a graphics card (usually the primary one) to behave as VGA, it
needs the legacy VGA memory and I/O port ranges to be under its
control. Any other (usually secondary) card would not need this, as
can be easily seen by the otherwise resulting conflict if you pass
through multiple graphics cards to distinct domains.

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

I'd prefer this to remain an if/else-if sequence, like done in
http://lists.xenproject.org/archives/html/xen-devel/2014-04/msg03988.html.
Perhaps that patch (once I get to submit v2) could serve as a
prereq for this change of yours?

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

I'm not sure if we shouldn't rather be conservative here and check
both domains' permissions.

And now that I reached the end of the patch I'm still missing the
similar adjustments for I/O port handling, while I think I saw you
claim somewhere that in this version you did deal with that.

Jan

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