Re: [Xen-devel] [V3 PATCH 7/9] pvh: change xsm_add_to_physmap

[Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>]

On 11/26/2013 09:27 PM, Mukesh Rathor wrote:
> In preparation for the next patch, we update xsm_add_to_physmap to
> allow for checking of foreign domain. Thus, the current domain must
> have the right to update the mappings of target domain with pages from
> foreign domain.
>
> Signed-off-by: Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
> CC: dgdegra@xxxxxxxxxxxxx
> ---
>   xen/arch/x86/mm.c       |   16 +++++++++++++---
>   xen/include/xsm/dummy.h |   10 ++++++++--
>   xen/include/xsm/xsm.h   |    6 +++---
>   xen/xsm/flask/hooks.c   |   10 ++++++++--
>   4 files changed, 32 insertions(+), 10 deletions(-)
The XSM changes look good; however, the calling code needs a bit of
tweaking.  Currently, if domain 0 is specified as the foreign domain,
the check is skipped, and the check is also run unnecessarily when
foreign_domid is nonzero but the operation is not XENMAPSPACE_gmfn_foreign.
The locking in this version also implies a potential TOCTOU bug, but
which in reality is impossible to trigger due to the existing RCU lock
held on (d).  I would suggest passing the foreign struct domain instead
of the domid, as below.

An unrelated question about XENMAPSPACE_gmfn_foreign that came up while
looking at this: is the domain parameter (d) supposed to be ignored
here, with maps always modifying the current domain? I would have
expected this call to manipulate d's physmap, with the common case being
(d == current->domain).

---

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 797fbc7..9afbcb9 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4531,23 +4531,17 @@ static int handle_iomem_range(unsigned long s, unsigned 
long e, void *p)
  * Returns: 0 ==> success
  */
 static int xenmem_add_foreign_to_pmap(unsigned long fgfn, unsigned long gpfn,
-                                      domid_t foreign_domid)
+                                      struct domain *fdom)
 {
     p2m_type_t p2mt, p2mt_prev;
     int rc = 0;
     unsigned long prev_mfn, mfn = 0;
-    struct domain *fdom, *currd = current->domain;
+    struct domain *currd = current->domain;
     struct page_info *page = NULL;
 
-    if ( currd->domain_id == foreign_domid || foreign_domid == DOMID_SELF ||
-         !is_pvh_domain(currd) )
+    if ( currd == fdom || !fdom || !is_pvh_domain(currd) )
         return -EINVAL;
 
-    /* Note, access check is done in the caller via xsm_add_to_physmap */
-    if ( !is_control_domain(currd) ||
-         (fdom = get_pg_owner(foreign_domid)) == NULL )
-        return -EPERM;
-
     /* following will take a refcnt on the mfn */
     page = get_page_from_gfn(fdom, fgfn, &p2mt, P2M_ALLOC);
     if ( !page || !p2m_is_valid(p2mt) )
@@ -4579,7 +4573,7 @@ static int xenmem_add_foreign_to_pmap(unsigned long fgfn, 
unsigned long gpfn,
     {
         gdprintk(XENLOG_WARNING, "set_foreign_p2m_entry failed. "
                  "gpfn:%lx mfn:%lx fgfn:%lx fd:%d\n",
-                 gpfn, mfn, fgfn, foreign_domid);
+                 gpfn, mfn, fgfn, fdom->domain_id);
         put_page(page);
         rc = -EINVAL;
     }
@@ -4590,14 +4584,13 @@ static int xenmem_add_foreign_to_pmap(unsigned long 
fgfn, unsigned long gpfn,
      */
     put_gfn(currd, gpfn);
 
-    put_pg_owner(fdom);
     return rc;
 }
 
 static int xenmem_add_to_physmap_once(
     struct domain *d,
     const struct xen_add_to_physmap *xatp,
-    domid_t foreign_domid)
+    struct domain *foreign_dom)
 {
     struct page_info *page = NULL;
     unsigned long gfn = 0; /* gcc ... */
@@ -4660,7 +4653,7 @@ static int xenmem_add_to_physmap_once(
         case XENMAPSPACE_gmfn_foreign:
         {
             rc = xenmem_add_foreign_to_pmap(xatp->idx, xatp->gpfn,
-                                            foreign_domid);
+                                            foreign_dom);
             return rc;
         }
 
@@ -4729,7 +4722,7 @@ static int xenmem_add_to_physmap(struct domain *d,
         start_xatp = *xatp;
         while ( xatp->size > 0 )
         {
-            rc = xenmem_add_to_physmap_once(d, xatp, DOMID_INVALID);
+            rc = xenmem_add_to_physmap_once(d, xatp, NULL);
             if ( rc < 0 )
                 return rc;
 
@@ -4755,11 +4748,12 @@ static int xenmem_add_to_physmap(struct domain *d,
         return rc;
     }
 
-    return xenmem_add_to_physmap_once(d, xatp, DOMID_INVALID);
+    return xenmem_add_to_physmap_once(d, xatp, NULL);
 }
 
 static int xenmem_add_to_physmap_range(struct domain *d,
-                                       struct xen_add_to_physmap_range *xatpr)
+                                       struct xen_add_to_physmap_range *xatpr,
+                                       struct domain *foreign_dom)
 {
     int rc;
 
@@ -4779,7 +4773,7 @@ static int xenmem_add_to_physmap_range(struct domain *d,
         xatp.space = xatpr->space;
         xatp.idx = idx;
         xatp.gpfn = gpfn;
-        rc = xenmem_add_to_physmap_once(d, &xatp, xatpr->foreign_domid);
+        rc = xenmem_add_to_physmap_once(d, &xatp, foreign_dom);
 
         if ( copy_to_guest_offset(xatpr->errs, xatpr->size-1, &rc, 1) )
             return -EFAULT;
@@ -4855,25 +4849,29 @@ long arch_memory_op(int op, 
XEN_GUEST_HANDLE_PARAM(void) arg)
         if ( d == NULL )
             return -ESRCH;
 
-        if ( xatpr.foreign_domid )
+        if ( xatpr.space == XENMAPSPACE_gmfn_foreign )
         {
-            if ( (fd = rcu_lock_domain_by_any_id(xatpr.foreign_domid)) == NULL 
)
+            fd = get_pg_owner(xatpr.foreign_domid);
+            if ( fd == NULL )
             {
                 rcu_unlock_domain(d);
                 return -ESRCH;
             }
-            rcu_unlock_domain(fd);
         }
 
         if ( (rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d, fd)) )
         {
             rcu_unlock_domain(d);
+            if (fd)
+                put_pg_owner(fd);
             return rc;
         }
 
-        rc = xenmem_add_to_physmap_range(d, &xatpr);
+        rc = xenmem_add_to_physmap_range(d, &xatpr, fd);
 
         rcu_unlock_domain(d);
+        if (fd)
+            put_pg_owner(fd);
 
         if ( rc == -EAGAIN )
             rc = hypercall_create_continuation(

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