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

Commit Message

Jan Beulich Feb. 17, 2021, 10:46 a.m. UTC

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 <[e-mail]>

Comments

Julien Grall Feb. 18, 2021, 10:25 a.m. UTC | #1

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 <[e-mail]>

The change in grant_table.c looks good to me:

Acked-by: Julien Grall <[e-mail]>

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

Jan Beulich Feb. 18, 2021, 11:31 a.m. UTC | #2

On 18.02.2021 11:25, Julien Grall wrote:
> 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 <[e-mail]>
> 
> The change in grant_table.c looks good to me:
> 
> Acked-by: Julien Grall <[e-mail]>

Thanks.

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

No, and I also don't think we need one. For one transitive grants
get implicitly treated as sub-page ones (I admit this is an
implementation detail, not really an ABI aspect) and the flag is
simply ignored in this case. Much like - see patch 3 - the flag
ought to also be ignored for v1 grants.

Jan

Patch

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