Re: [PATCH 11/18] xen: use XSM instead of IS_PRIV where duplicated

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

On 08/06/2012 11:18 AM, Jan Beulich wrote:
>>>> On 06.08.12 at 16:32, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote:
>> --- a/xen/arch/x86/domctl.c
>> +++ b/xen/arch/x86/domctl.c
>> @@ -54,6 +54,26 @@ long arch_do_domctl(
>>  
>>      switch ( domctl->cmd )
>>      {
>> +    /* TODO: the following do not have XSM hooks yet */
>> +    case XEN_DOMCTL_set_cpuid:
>> +    case XEN_DOMCTL_suppress_spurious_page_faults:
>> +    case XEN_DOMCTL_debug_op:
>> +    case XEN_DOMCTL_gettscinfo:
>> +    case XEN_DOMCTL_settscinfo:
>> +    case XEN_DOMCTL_audit_p2m:
>> +    case XEN_DOMCTL_gdbsx_guestmemio:
>> +    case XEN_DOMCTL_gdbsx_pausevcpu:
>> +    case XEN_DOMCTL_gdbsx_unpausevcpu:
>> +    case XEN_DOMCTL_gdbsx_domstatus:
>> +    /* getpageframeinfo[23] will leak XEN_DOMCTL_PFINFO_XTAB on target GFNs */
> 
> Is that to state that the patch introduces a leak here? Or are you
> trying to carefully tell us you spotted a problem in the existing
> code?

This is an information leak, not a memory leak. It's a (minor) problem with
the placement of the existing XSM hooks allowing a domain to query information
on remote domains. A later patch fixes this by adding an XSM hook for the
entire query operation.

>> +    case XEN_DOMCTL_getpageframeinfo2:
>> +    case XEN_DOMCTL_getpageframeinfo3:
>> +        if ( !IS_PRIV(current->domain) )
>> +            return -EPERM;
>> +    }
>> +
>> +    switch ( domctl->cmd )
>> +    {
>>  
>>      case XEN_DOMCTL_shadow_op:
>>      {
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -3366,12 +3366,12 @@ static int hvmop_set_pci_intx_level(
>>      if ( (op.domain > 0) || (op.bus > 0) || (op.device > 31) || (op.intx > 3) )
>>          return -EINVAL;
>>  
>> -    rc = rcu_lock_remote_target_domain_by_id(op.domid, &d);
>> -    if ( rc != 0 )
>> -        return rc;
>> +    d = rcu_lock_domain_by_id(op.domid);
>> +    if ( d == NULL )
>> +        return -ESRCH;
>>  
>>      rc = -EINVAL;
>> -    if ( !is_hvm_domain(d) )
>> +    if ( d == current->domain || !is_hvm_domain(d) )
> 
> What's wrong with rcu_lock_remote_target_domain_by_id() here
> and in other places below? I think this minimally would deserve
> a comment in the patch description, the more that this huge a
> patch is already bad enough to look at.
> 
> Jan
> 

The main reason for this change is that rcu_lock_remote_target_domain_by_id
calls IS_PRIV, and this patch is attempting to remove the duplicated calls.
Would you prefer making another rcu_lock_* function that only checks against
current->domain and doesn't include the IS_PRIV_FOR check?

-- 
Daniel De Graaf
National Security Agency