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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.09.12 at 16:25, Daniel De Graaf <dgdegra@xxxxxxxxxxxxx> wrote:
> On 09/13/2012 10:13 AM, Jan Beulich wrote:
>>>>> On 13.09.12 at 15:46, Daniel De Graaf <dgdegra@xxxxxxxxxxxxx> wrote:
>>> On 09/13/2012 04:00 AM, Jan Beulich wrote:
>>>>>>> On 12.09.12 at 17:59, Daniel De Graaf <dgdegra@xxxxxxxxxxxxx> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel