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

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

On 09/13/2012 11:04 AM, Jan Beulich wrote:
>>>> On 13.09.12 at 16:25, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote:
>> 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?
> 
> I'm sorry, each of your replies get me more confused. Can you,
> with the current code (i.e. without your patches or any emulation
> you might be doing) explain (perhaps with an example) what
> specific case you see needing more checking than there is
> currently? That would probably allow us to start talking about the
> same thing.
> 
> Jan
> 

For this example, assume domain A has access to map domain B's memory
read-only. Domain B has access to a device with MMIO where reads to the
device's memory cause state changes - an example of such as device is a
TPM, where replies are read by repeated reads to a single 4-byte 
address. Domain A does not have access to this device, and would like to
maliciously interfere with the device.

If domain A maps the MMIO page from domain B using pg_owner == domB, the
iomem_access_permitted check will be done from domain B's perspective. 
While this is needed when domain A is mapping pages on behalf of domB, 
it is not sufficient when attempting to constrain domain A's access. 

These checks only apply to MMIO, so the condition on line 735 will
evaluate to true (!mfn_valid || real_pg_owner == dom_io).

The (existing) check on domain B's MMIO access is:
        if ( !iomem_access_permitted(pg_owner, mfn, mfn) )

This patch adds a check on domain A:
        if ( !iomem_access_permitted(curr->domain, mfn, mfn) )
            
This extra checking has not been required without XSM because only FLASK
implements the ability to grant one domain read-only access to another 
domain. With read-write access, this extra access check is simple to get
around: domain A can modify some part of the executable code in domain B
to act as a proxy for its accesses. Additionally, domain A is usually
dom0 or the device model, which have equal or greater MMIO access.

-- 
Daniel De Graaf
National Security Agency