From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel De Graaf Subject: Re: [PATCH 19/22] arch/x86: check remote MMIO remap permissions Date: Thu, 13 Sep 2012 10:25:11 -0400 Message-ID: <5051ECC7.7050901@tycho.nsa.gov> References: <1347465586-20009-1-git-send-email-dgdegra@tycho.nsa.gov> <1347465586-20009-20-git-send-email-dgdegra@tycho.nsa.gov> <5051AED7020000780009AFC3@nat28.tlf.novell.com> <5051E3AC.2060700@tycho.nsa.gov> <50520634020000780009B162@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <50520634020000780009B162@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Tim Deegan , Keir Fraser , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 09/13/2012 10:13 AM, Jan Beulich wrote: >>>> On 13.09.12 at 15:46, Daniel De Graaf wrote: >> On 09/13/2012 04:00 AM, Jan Beulich wrote: >>>>>> On 12.09.12 at 17:59, Daniel De Graaf 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