Re: [PATCH 19/22] arch/x86: check remote MMIO remap permissions

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

On 09/13/2012 10:13 AM, Jan Beulich wrote:
>>>> On 13.09.12 at 15:46, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote:
>> On 09/13/2012 04:00 AM, Jan Beulich wrote:
>>>>>> On 12.09.12 at 17:59, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote:
>>>> When a domain is mapping pages from a different pg_owner domain, the
>>>> iomem_access checks are currently only applied to the pg_owner domain,
>>>> potentially allowing the current domain to bypass its more restrictive
>>>> iomem_access policy using another domain that it has access to.
>>>
>>> Are you sure about this? I ask because ...
>>>
>>>> --- a/xen/arch/x86/mm.c
>>>> +++ b/xen/arch/x86/mm.c
>>>> @@ -754,6 +754,18 @@ get_page_from_l1e(
>>>>              return -EINVAL;
>>>>          }
>>>>  
>>>> +        if ( pg_owner != curr->domain &&
>>>> +             !iomem_access_permitted(curr->domain, mfn, mfn) )
>>>> +        {
>>>> +            if ( mfn != (PADDR_MASK >> PAGE_SHIFT) ) /* INVALID_MFN? */
>>>> +            {
>>>> +                MEM_LOG("Domain %u attempted to map I/O space %08lx in 
>> domain %u",
>>>> +                        curr->domain->domain_id, mfn, pg_owner->domain_id);
>>>> +                return -EPERM;
>>>> +            }
>>>> +            return -EINVAL;
>>>> +        }
>>>> +
>>>
>>> ... the place you insert this is after non-RAM pages got filtered
>>> out already, so you're applying an IOMEM permission check to a
>>> RAM page.
>>>
>>> Jan
>>>
>>>>          if ( !(l1f & _PAGE_RW) ||
>>>>               !rangeset_contains_singleton(mmio_ro_ranges, mfn) )
>>>>              return 0;
>>
>> If that's true, then the check a few lines above is also applying IOMEM
>> checks to RAM pages. I can see non-privileged attempts being filtered
>> above,

"above" refers to "if ( !iomem_access_permitted(pg_owner, mfn, mfn) )"
 
> I can't see how that would happen given this primary conditional
> 
>     if ( !mfn_valid(mfn) ||
>          (real_pg_owner = page_get_owner_and_reference(page)) == dom_io )
> 
> Please clarify what you're observing.

As I understand it, the contents of this block will be executed if the MFN is
invalid (interpreted as MMIO space) or if the page's owner is DOMID_IO, which
is how MMIO space is marked.

>> but successful mappings will continue to the check I added.
> 
> Of course. I would think that if anything, you would want to add
> a second call to iomem_access_permitted() with "curr->domain"
> right at the place where the current one is (in particular inside
> the above quoted conditional).
> 
> Jan

I was emulating the existing iomem_access_permitted check being run on pg_owner;
moving the curr->domain check up into this first conditional would end up
treating the MMIO mapping as a regular RAM mapping if the iomem_access_permitted
fails. Unless you're talking about a different quoted conditional?

-- 
Daniel De Graaf
National Security Agency