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

[Jan Beulich <jbeulich@xxxxxxxx>]

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 <jbeulich@xxxxxxxx>
> 
> The change in grant_table.c looks good to me:
> 
> Acked-by: Julien Grall <jgrall@xxxxxxxxxx>

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