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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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,

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.

> 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


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