Re: [PATCH 1/6] xen: use domid check in is_hardware_domain

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

On 03/05/2014 04:23 AM, Jan Beulich wrote:
>>>> On 04.03.14 at 23:51, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote:
>> The hardware domain manages devices for PCI pass-through to driver
>> domains or can act as a driver domain itself, depending on the desired
>> degree of disaggregation.  It is also the domain managing devices that
>> do not support pass-through: PCI configuration space access, parsing the
>> hardware ACPI tables and system power or machine check events.  This is
>> the only domain where is_hardware_domain() is true.  The return of
>> is_control_domain() is false for this domain.
>
> "s/is/may be/" ?

I had intended this sentence to describe the model, where the return is
always false. However, I agree with the change to avoid confusion that
the two is_*_domain() functions are exclusive.

>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -1505,7 +1505,7 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
>>           }
>>
>>           set_cpuid_faulting(is_pv_vcpu(next) &&
>> -                           (next->domain->domain_id != 0));
>> +                           !is_control_domain(next->domain));
>
> I can't see why the hardware domain (which can't be migrated)
> should be prevented from seeing the real CPUID values. It's
> less obvious whether a control domain should always see real
> values. Hence if you think the latter should be the case (as the
> change above shows), I think you should check for both cases
> here.

Agreed, the hardware domain also needs to be checked for here. The
reason the control domain is present is that it needs to see real
CPUID values in order to set CPUID policy for guests based on the
real hardware values.

>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -738,7 +738,7 @@ void pv_cpuid(struct cpu_user_regs *regs)
>>       c = regs->ecx;
>>       d = regs->edx;
>>
>> -    if ( current->domain->domain_id != 0 )
>> +    if ( !is_control_domain(current->domain) )
>
> The same consideration applies here then, obviously.
>
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -242,7 +242,7 @@ struct domain *domain_create(
>>       else if ( domcr_flags & DOMCRF_pvh )
>>           d->guest_type = guest_type_pvh;
>>
>> -    if ( domid == 0 )
>> +    if ( is_hardware_domain(d) )
>>       {
>>           d->is_pinned = opt_dom0_vcpus_pin;
>>           d->disable_migrate = 1;
>> @@ -267,10 +267,10 @@ struct domain *domain_create(
>>           d->is_paused_by_controller = 1;
>>           atomic_inc(&d->pause_count);
>>
>> -        if ( domid )
>> -            d->nr_pirqs = nr_static_irqs + extra_domU_irqs;
>> -        else
>> +        if ( is_hardware_domain(d) )
>>               d->nr_pirqs = nr_static_irqs + extra_dom0_irqs;
>> +        else
>> +            d->nr_pirqs = nr_static_irqs + extra_domU_irqs;
>
> I'd prefer the if/else cases to remain as they are - makes the patch
> smaller, and fits better with the (weak) model of using the if branch
> for the common case and the else one for the special one (outside
> of error handling of course).

OK. I prefer to avoid the if (!foo) bar; else baz; construct where
possible, but common case first is a good reason to use it.

>> --- a/xen/common/xenoprof.c
>> +++ b/xen/common/xenoprof.c
>> @@ -603,7 +603,7 @@ static int xenoprof_op_init(XEN_GUEST_HANDLE_PARAM(void) arg)
>>
>>       xenoprof_init.is_primary =
>>           ((xenoprof_primary_profiler == d) ||
>> -         ((xenoprof_primary_profiler == NULL) && (d->domain_id == 0)));
>> +         ((xenoprof_primary_profiler == NULL) && is_control_domain(d)));
>
> Do you really consider profiling a control domain property? This is
> even more so questionable without knowing whether you checked
> that there are no issues with all of the sudden there perhaps
> being more than one domain eligible for becoming the primary
> profiler - the oprofile code isn't in that good a shape to be certain
> without explicit checking.
>
> Jan

I don't directly consider profiling to be a control domain property, but
I also don't consider it a hardware domain property, and it does need to
be restricted in some way. I could make a separate patch changing the
condition to use an XSM hook, only setting xenoprof_primary_profiler if
the domain is allowed the XEN__PRIVPROFILE permission, but this still
could cause multiple domains to be eligible.

 From my cursory examination when I made this change, the first domain to
try becoming the primary profiler will succeed and be assigned to
xenoprof_primary_profiler. Later domains will not be considered since the
primary will already be set.

One thing I had not considered that may be a problem: if the profiling
domain is shut down without calling XENOPROF_shutdown, it will not be
possible to use profiling this boot unless the struct domain* is reused.
This may then become a security issue because an arbitrary domain is
now the primary profiler, although the XSM policy should prevent any
actions by a domain not permitted to use the profiling hypercalls.
Using is_hardware_domain here avoids that problem (as the hardware domain
may never shut down or be destroyed), so that may be the simplest
solution until a better model of who is responsible for profiling is
presented.

-- 
Daniel De Graaf
National Security Agency