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

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

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@oracle.com>
> CC: dgdegra@tycho.nsa.gov
> ---
>   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(