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

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

On 05/06/2014 11:06 AM, Jan Beulich wrote:
>>>> On 05.05.14 at 17:54, <avanzini.arianna@gmail.com> 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.
> 

I think it's not. Thank you for pointing that out, I will split the patch
according to your suggestion.

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

Right, the commit description is still very confused, thank you for pointing
that out.

QEMU seems to rely only on the memory_mapping domctl to map VGA-related memory
areas in case gfx passthru is enabled, if I'm seeing things correctly. The
xc_domain_memory_mapping() function is invoked from the register_vga_regions()
function (defined in hw/pt-graphics.c). The latter function seems to be executed
upon registration of a physical device (register_real_device() ->
pt_register_regions() in hw/pass-through.c), and to be actually executed only if
gfx passthru is enabled for the device (if it is not, the function seems to
immediately return without performing any mapping operation of I/O memory or ports).

Since this patch aims at separating the functions of the memory_mapping and
iomem_permission domctls, memory_mapping does not implicitly grants permission
to mapped I/O-memory ranges; having QEMU invoking just the memory_mapping domctl
would lead to a failure. This hunk was supposed to allow to the domain access to
the necessary VGA-related memory ranges in case gfx passthru is enabled by
domain configuration.


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

I see, thank you for the clear explanation. Unfortunately I just relied on the
QEMU code, which seems to execute the function (and map those memory ranges) for
all devices, provided that gfx passthru is enabled and there is a VGA
controller. I most probably overlooked something.


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

Certainly, thank you for the pointer.

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

Sure, thank you for the suggestion.

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

The previous version of the patch gave access permission to I/O ports (in
do_pci_add()) only to paravirtualized domains. This version of the patch
executes also for HVM domains all the code that was previously executed only for
PV domains, including the invocation of ioport_permission. As Ian Campbell
suggested (and hoping I understood his suggestion correctly), it does:
if (hvm)
    device_model_related_code();
previously_pv_specific_code();


> Jan
> 


-- 
/*
 * Arianna Avanzini
 * avanzini.arianna@gmail.com
 * 73628@studenti.unimore.it
 */