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

[Julien Grall <julien@xxxxxxx>]

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