Re: [PATCH 16/18] arch/x86: use XSM hooks for get_pg_owner access checks

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

On 08/06/2012 11:26 AM, Jan Beulich wrote:
>>>> On 06.08.12 at 16:32, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote:
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -2882,11 +2882,6 @@ static struct domain *get_pg_owner(domid_t domid)
>>          pg_owner = rcu_lock_domain(dom_io);
>>          break;
>>      case DOMID_XEN:
>> -        if ( !IS_PRIV(curr) )
>> -        {
>> -            MEM_LOG("Cannot set foreign dom");
>> -            break;
>> -        }
>>          pg_owner = rcu_lock_domain(dom_xen);
>>          break;
>>      default:
>> @@ -2895,12 +2890,6 @@ static struct domain *get_pg_owner(domid_t domid)
>>              MEM_LOG("Unknown domain '%u'", domid);
>>              break;
>>          }
>> -        if ( !IS_PRIV_FOR(curr, pg_owner) )
>> -        {
>> -            MEM_LOG("Cannot set foreign dom");
>> -            rcu_unlock_domain(pg_owner);
>> -            pg_owner = NULL;
>> -        }
>>          break;
>>      }
>>  
>> @@ -3008,6 +2997,13 @@ int do_mmuext_op(
>>          goto out;
>>      }
>>  
>> +    rc = xsm_mmuext_op(d, pg_owner);
> 
> Given the above, this could be
> 
> xsm_mmuext_op(dom0, DOMID_{IO,XEN});
> 
> yet ...
> 
>> +    if ( rc )
>> +    {
>> +        rcu_unlock_domain(pg_owner);
>> +        goto out;
>> +    }
>> +
>>      for ( i = 0; i < count; i++ )
>>      {
>>          if ( hypercall_preempt_check() )
>> @@ -3483,11 +3479,6 @@ int do_mmu_update(
>>              rc = -EINVAL;
>>              goto out;
>>          }
>> -        if ( !IS_PRIV_FOR(d, pt_owner) )
>> -        {
>> -            rc = -ESRCH;
>> -            goto out;
>> -        }
>>      }
>>  
>>      if ( (pg_owner = get_pg_owner((uint16_t)foreigndom)) == NULL )
>> @@ -3643,7 +3634,7 @@ int do_mmu_update(
>>              mfn = req.ptr >> PAGE_SHIFT;
>>              gpfn = req.val;
>>  
>> -            rc = xsm_mmu_machphys_update(d, mfn);
>> +            rc = xsm_mmu_machphys_update(d, pg_owner, mfn);
>>              if ( rc )
>>                  break;
>>  
>> --- a/xen/include/xsm/dummy.h
>> +++ b/xen/include/xsm/dummy.h
>> @@ -803,19 +803,35 @@ static XSM_DEFAULT(int, domain_memory_map) (struct domain *d)
>>  }
>>  
>>  static XSM_DEFAULT(int, mmu_normal_update) (struct domain *d, struct domain *t,
>> -                                    struct domain *f, intpte_t fpte)
>> +                                            struct domain *f, intpte_t fpte)
>>  {
>> +    if ( d != t && !IS_PRIV_FOR(d, t) )
>> +        return -EPERM;
>> +    if ( d != f && !IS_PRIV_FOR(d, f) )
>> +        return -EPERM;
>>      return 0;
>>  }
>>  
>> -static XSM_DEFAULT(int, mmu_machphys_update) (struct domain *d, unsigned long mfn)
>> +static XSM_DEFAULT(int, mmu_machphys_update) (struct domain *d, struct domain *f,
>> +                                              unsigned long mfn)
>>  {
>> +    if ( d != f && !IS_PRIV_FOR(d, f) )
>> +        return -EPERM;
>> +    return 0;
>> +}
>> +
>> +static XSM_DEFAULT(int, mmuext_op) (struct domain *d, struct domain *f)
>> +{
>> +    if ( d != f && !IS_PRIV_FOR(d, f) )
>> +        return -EPERM;
> 
> ... Dom0 is neither privileged for DOM_IO nor for DOM_XEN.

Actually, it is. IS_PRIV_FOR returns true for any domain when called from an
IS_PRIV domain.

> 
>>      return 0;
>>  }
>>  
>>  static XSM_DEFAULT(int, update_va_mapping) (struct domain *d, struct domain *f, 
>>                                                              l1_pgentry_t pte)
>>  {
>> +    if ( d != f && !IS_PRIV_FOR(d, f) )
>> +        return -EPERM;
>>      return 0;
>>  }
>>  
> 
> Didn't check the other cases in any detail.
> 
> Jan
> 


-- 
Daniel De Graaf
National Security Agency