[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 1/3] gnttab: never permit mapping transitive grants



Hi Jan,

On 17/02/2021 10:46, Jan Beulich wrote:
Transitive grants allow an intermediate domain I to grant a target
domain T access to a page which origin domain O did grant I access to.
As an implementation restriction, T is not allowed to map such a grant.
This restriction is currently tried to be enforced by marking active
entries resulting from transitive grants as is-sub-page; sub-page grants
for obvious reasons don't allow mapping. However, marking (and checking)
only active entries is insufficient, as a map attempt may also occur on
a grant not otherwise in use. When not presently in use (pin count zero)
the grant type itself needs checking. Otherwise T may be able to map an
unrelated page owned by I. This is because the "transitive" sub-
structure of the v2 union would end up being interpreted as "full_page"
sub-structure instead. The low 32 bits of the GFN used would match the
grant reference specified in I's transitive grant entry, while the upper
32 bits could be random (depending on how exactly I sets up its grant
table entries).

Note that if one mapping already exists and the granting domain _then_
changes the grant to GTF_transitive (which the domain is not supposed to
do), the changed type will only be honored after the pin count has gone
back to zero. This is no different from e.g. GTF_readonly or
GTF_sub_page becoming set when a grant is already in use.

While adjusting the implementation, also adjust commentary in the public
header to better reflect reality.

Fixes: 3672ce675c93 ("Transitive grant support")
Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

The change in grant_table.c looks good to me:

Acked-by: Julien Grall <jgrall@xxxxxxxxxx>


--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -851,9 +851,10 @@ static int _set_status_v2(const grant_en
          mask |= GTF_sub_page;
/* If not already pinned, check the grant domid and type. */
-    if ( !act->pin && ((((scombo.flags & mask) != GTF_permit_access) &&
-                        ((scombo.flags & mask) != GTF_transitive)) ||
-                       (scombo.domid != ldomid)) )
+    if ( !act->pin &&
+         ((((scombo.flags & mask) != GTF_permit_access) &&
+           (mapflag || ((scombo.flags & mask) != GTF_transitive))) ||
+          (scombo.domid != ldomid)) )
          PIN_FAIL(done, GNTST_general_error,
                   "Bad flags (%x) or dom (%d); expected d%d, flags %x\n",
                   scombo.flags, scombo.domid, ldomid, mask);
@@ -879,7 +880,7 @@ static int _set_status_v2(const grant_en
      if ( !act->pin )
      {
          if ( (((scombo.flags & mask) != GTF_permit_access) &&
-              ((scombo.flags & mask) != GTF_transitive)) ||
+              (mapflag || ((scombo.flags & mask) != GTF_transitive))) ||
               (scombo.domid != ldomid) ||
               (!readonly && (scombo.flags & GTF_readonly)) )
          {
--- a/xen/include/public/grant_table.h
+++ b/xen/include/public/grant_table.h
@@ -166,11 +166,13 @@ typedef struct grant_entry_v1 grant_entr
  #define GTF_type_mask       (3U<<0)
/*
- * Subflags for GTF_permit_access.
+ * Subflags for GTF_permit_access and GTF_transitive.
   *  GTF_readonly: Restrict @domid to read-only mappings and accesses. [GST]
   *  GTF_reading: Grant entry is currently mapped for reading by @domid. [XEN]
   *  GTF_writing: Grant entry is currently mapped for writing by @domid. [XEN]
- *  GTF_PAT, GTF_PWT, GTF_PCD: (x86) cache attribute flags for the grant [GST]
+ * Further subflags for GTF_permit_access only.
+ *  GTF_PAT, GTF_PWT, GTF_PCD: (x86) cache attribute flags to be used for
+ *                             mappings of the grant [GST]
   *  GTF_sub_page: Grant access to only a subrange of the page.  @domid
   *                will only be allowed to copy from the grant, and not
   *                map it. [GST]

Do we have any check preventing GTF_sub_page and GTF_transitive to be set together?

Cheers,

--
Julien Grall



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.