[PATCH 0/3] gnttab: misc fixes

[PATCH 0/3] gnttab: misc fixes

[Jan Beulich]

Patches 1 and 2 clearly are intended for 4.15; patch 3 is possibly
controversial (along the lines of similar relaxation proposed for
other (sub-)hypercalls), and hence unlikely to be a candidate as
well.

1: never permit mapping transitive grants
2: bypass IOMMU (un)mapping when a domain is (un)mapping its own grant
3: GTF_sub_page is a v2-only flag

Jan

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

[Jan Beulich]

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@suse.com>

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

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

[Julien Grall]

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@suse.com>

The change in grant_table.c looks good to me:

Acked-by: Julien Grall <jgrall@amazon.com>

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

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

[Jan Beulich]

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

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 2/3] gnttab: bypass IOMMU (un)mapping when a domain is (un)mapping its own grant

[Jan Beulich]

Mappings for a domain's own pages should already be present in the
IOMMU. While installing the same mapping again is merely redundant (and
inefficient), removing the mapping when the grant mapping gets removed
is outright wrong in this case: The mapping was there before the map, so
should remain in place after unmapping.

This affects
- Arm Dom0 in the direct mapped case,
- x86 PV Dom0 in the "iommu=dom0-strict" / "dom0-iommu=strict" cases,
- all x86 PV DomU-s, including driver domains.

Reported-by: Rahul Singh <Rahul.Singh@arm.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1243,7 +1243,7 @@ map_grant_ref(
         goto undo_out;
     }
 
-    need_iommu = gnttab_need_iommu_mapping(ld);
+    need_iommu = ld != rd && gnttab_need_iommu_mapping(ld);
     if ( need_iommu )
     {
         unsigned int kind;
@@ -1493,7 +1493,7 @@ unmap_common(
     if ( put_handle )
         put_maptrack_handle(lgt, op->handle);
 
-    if ( rc == GNTST_okay && gnttab_need_iommu_mapping(ld) )
+    if ( rc == GNTST_okay && ld != rd && gnttab_need_iommu_mapping(ld) )
     {
         unsigned int kind;
         int err = 0;

Re: [PATCH 2/3] gnttab: bypass IOMMU (un)mapping when a domain is (un)mapping its own grant

[Julien Grall]

Hi Jan,

On 17/02/2021 10:46, Jan Beulich wrote:
> Mappings for a domain's own pages should already be present in the
> IOMMU. While installing the same mapping again is merely redundant (and
> inefficient), removing the mapping when the grant mapping gets removed
> is outright wrong in this case: The mapping was there before the map, so
> should remain in place after unmapping.
> 
> This affects
> - Arm Dom0 in the direct mapped case,
> - x86 PV Dom0 in the "iommu=dom0-strict" / "dom0-iommu=strict" cases,
> - all x86 PV DomU-s, including driver domains.
> 
> Reported-by: Rahul Singh <Rahul.Singh@arm.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1243,7 +1243,7 @@ map_grant_ref(
>           goto undo_out;
>       }
>   
> -    need_iommu = gnttab_need_iommu_mapping(ld);
> +    need_iommu = ld != rd && gnttab_need_iommu_mapping(ld);

AFAICT, the owner of the page may not always be rd. So do we want to 
check against the owner instead?

Cheers,

-- 
Julien Grall

Re: [PATCH 2/3] gnttab: bypass IOMMU (un)mapping when a domain is (un)mapping its own grant

[Jan Beulich]

On 17.02.2021 12:03, Julien Grall wrote:
> On 17/02/2021 10:46, Jan Beulich wrote:
>> Mappings for a domain's own pages should already be present in the
>> IOMMU. While installing the same mapping again is merely redundant (and
>> inefficient), removing the mapping when the grant mapping gets removed
>> is outright wrong in this case: The mapping was there before the map, so
>> should remain in place after unmapping.
>>
>> This affects
>> - Arm Dom0 in the direct mapped case,
>> - x86 PV Dom0 in the "iommu=dom0-strict" / "dom0-iommu=strict" cases,
>> - all x86 PV DomU-s, including driver domains.
>>
>> Reported-by: Rahul Singh <Rahul.Singh@arm.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -1243,7 +1243,7 @@ map_grant_ref(
>>           goto undo_out;
>>       }
>>   
>> -    need_iommu = gnttab_need_iommu_mapping(ld);
>> +    need_iommu = ld != rd && gnttab_need_iommu_mapping(ld);
> 
> AFAICT, the owner of the page may not always be rd. So do we want to 
> check against the owner instead?

For the DomIO case - specifically not. And the DomCOW case can't
happen when an IOMMU is in use. Did I overlook any other cases
where the page may not be owned by rd?

Jan

Re: [PATCH 2/3] gnttab: bypass IOMMU (un)mapping when a domain is (un)mapping its own grant

[Julien Grall]

Hi Jan,

On 17/02/2021 11:38, Jan Beulich wrote:
> On 17.02.2021 12:03, Julien Grall wrote:
>> On 17/02/2021 10:46, Jan Beulich wrote:
>>> Mappings for a domain's own pages should already be present in the
>>> IOMMU. While installing the same mapping again is merely redundant (and
>>> inefficient), removing the mapping when the grant mapping gets removed
>>> is outright wrong in this case: The mapping was there before the map, so
>>> should remain in place after unmapping.
>>>
>>> This affects
>>> - Arm Dom0 in the direct mapped case,
>>> - x86 PV Dom0 in the "iommu=dom0-strict" / "dom0-iommu=strict" cases,
>>> - all x86 PV DomU-s, including driver domains.
>>>
>>> Reported-by: Rahul Singh <Rahul.Singh@arm.com>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> --- a/xen/common/grant_table.c
>>> +++ b/xen/common/grant_table.c
>>> @@ -1243,7 +1243,7 @@ map_grant_ref(
>>>            goto undo_out;
>>>        }
>>>    
>>> -    need_iommu = gnttab_need_iommu_mapping(ld);
>>> +    need_iommu = ld != rd && gnttab_need_iommu_mapping(ld);
>>
>> AFAICT, the owner of the page may not always be rd. So do we want to
>> check against the owner instead?
> 
> For the DomIO case - specifically not. And the DomCOW case can't
> happen when an IOMMU is in use. Did I overlook any other cases
> where the page may not be owned by rd?

For the current code, it looks like not. But it feels to me this code is 
fragile as we are assuming that other cases should never happen.

I think it would be worth explaining in a comment and the commit message 
why check rd rather than the page owner is sufficient.

Cheers,

-- 
Julien Grall

Re: [PATCH 2/3] gnttab: bypass IOMMU (un)mapping when a domain is (un)mapping its own grant

[Jan Beulich]

On 17.02.2021 12:41, Julien Grall wrote:
> Hi Jan,
> 
> On 17/02/2021 11:38, Jan Beulich wrote:
>> On 17.02.2021 12:03, Julien Grall wrote:
>>> On 17/02/2021 10:46, Jan Beulich wrote:
>>>> Mappings for a domain's own pages should already be present in the
>>>> IOMMU. While installing the same mapping again is merely redundant (and
>>>> inefficient), removing the mapping when the grant mapping gets removed
>>>> is outright wrong in this case: The mapping was there before the map, so
>>>> should remain in place after unmapping.
>>>>
>>>> This affects
>>>> - Arm Dom0 in the direct mapped case,
>>>> - x86 PV Dom0 in the "iommu=dom0-strict" / "dom0-iommu=strict" cases,
>>>> - all x86 PV DomU-s, including driver domains.
>>>>
>>>> Reported-by: Rahul Singh <Rahul.Singh@arm.com>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> --- a/xen/common/grant_table.c
>>>> +++ b/xen/common/grant_table.c
>>>> @@ -1243,7 +1243,7 @@ map_grant_ref(
>>>>            goto undo_out;
>>>>        }
>>>>    
>>>> -    need_iommu = gnttab_need_iommu_mapping(ld);
>>>> +    need_iommu = ld != rd && gnttab_need_iommu_mapping(ld);
>>>
>>> AFAICT, the owner of the page may not always be rd. So do we want to
>>> check against the owner instead?
>>
>> For the DomIO case - specifically not. And the DomCOW case can't
>> happen when an IOMMU is in use. Did I overlook any other cases
>> where the page may not be owned by rd?
> 
> For the current code, it looks like not. But it feels to me this code is 
> fragile as we are assuming that other cases should never happen.
> 
> I think it would be worth explaining in a comment and the commit message 
> why check rd rather than the page owner is sufficient.

Well, I've added

    /*
     * This is deliberately not checking the page's owner: get_paged_frame()
     * explicitly rejects foreign pages, and all success paths above yield
     * either owner == rd or owner == dom_io (the dom_cow case is irrelevant
     * as mem-sharing and IOMMU use are incompatible). The dom_io case would
     * need checking separately if we compared against owner here.
     */

to map_grant_ref(), and a reference to this comment to both
unmap_common() and the commit message. Will this do?

Jan

Re: [PATCH 2/3] gnttab: bypass IOMMU (un)mapping when a domain is (un)mapping its own grant

[Julien Grall]

Hi Jan,

On 17/02/2021 13:16, Jan Beulich wrote:
> On 17.02.2021 12:41, Julien Grall wrote:
>> Hi Jan,
>>
>> On 17/02/2021 11:38, Jan Beulich wrote:
>>> On 17.02.2021 12:03, Julien Grall wrote:
>>>> On 17/02/2021 10:46, Jan Beulich wrote:
>>>>> Mappings for a domain's own pages should already be present in the
>>>>> IOMMU. While installing the same mapping again is merely redundant (and
>>>>> inefficient), removing the mapping when the grant mapping gets removed
>>>>> is outright wrong in this case: The mapping was there before the map, so
>>>>> should remain in place after unmapping.
>>>>>
>>>>> This affects
>>>>> - Arm Dom0 in the direct mapped case,
>>>>> - x86 PV Dom0 in the "iommu=dom0-strict" / "dom0-iommu=strict" cases,
>>>>> - all x86 PV DomU-s, including driver domains.
>>>>>
>>>>> Reported-by: Rahul Singh <Rahul.Singh@arm.com>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>
>>>>> --- a/xen/common/grant_table.c
>>>>> +++ b/xen/common/grant_table.c
>>>>> @@ -1243,7 +1243,7 @@ map_grant_ref(
>>>>>             goto undo_out;
>>>>>         }
>>>>>     
>>>>> -    need_iommu = gnttab_need_iommu_mapping(ld);
>>>>> +    need_iommu = ld != rd && gnttab_need_iommu_mapping(ld);
>>>>
>>>> AFAICT, the owner of the page may not always be rd. So do we want to
>>>> check against the owner instead?
>>>
>>> For the DomIO case - specifically not. And the DomCOW case can't
>>> happen when an IOMMU is in use. Did I overlook any other cases
>>> where the page may not be owned by rd?
>>
>> For the current code, it looks like not. But it feels to me this code is
>> fragile as we are assuming that other cases should never happen.
>>
>> I think it would be worth explaining in a comment and the commit message
>> why check rd rather than the page owner is sufficient.
> 
> Well, I've added
> 
>      /*
>       * This is deliberately not checking the page's owner: get_paged_frame()
>       * explicitly rejects foreign pages, and all success paths above yield
>       * either owner == rd or owner == dom_io (the dom_cow case is irrelevant
>       * as mem-sharing and IOMMU use are incompatible). The dom_io case would
>       * need checking separately if we compared against owner here.
>       */
> 
> to map_grant_ref(), and a reference to this comment to both
> unmap_common() and the commit message. Will this do?

LGTM. With that, you can add:

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

[PATCH 3/3] gnttab: GTF_sub_page is a v2-only flag

[Jan Beulich]

Prior to its introduction, v1 entries weren't checked for this flag, and
the flag also has been meaningless for v1 entries. Therefore it also
shouldn't be checked. (The only consistent alternative would be to also
check for all currently undefined flags to be clear.)

Fixes: b545941b6638 ("Implement sub-page grant support")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -762,13 +762,11 @@ static int _set_status_v1(const grant_en
                           struct domain *rd,
                           struct active_grant_entry *act,
                           int readonly,
-                          int mapflag,
-                          domid_t  ldomid)
+                          domid_t ldomid)
 {
     int rc = GNTST_okay;
     uint32_t *raw_shah = (uint32_t *)shah;
     union grant_combo scombo;
-    uint16_t mask = GTF_type_mask;
 
     /*
      * We bound the number of times we retry CMPXCHG on memory locations that
@@ -780,11 +778,6 @@ static int _set_status_v1(const grant_en
      */
     int retries = 0;
 
-    /* if this is a grant mapping operation we should ensure GTF_sub_page
-       is not set */
-    if ( mapflag )
-        mask |= GTF_sub_page;
-
     scombo.raw = ACCESS_ONCE(*raw_shah);
 
     /*
@@ -798,8 +791,9 @@ static int _set_status_v1(const grant_en
         union grant_combo prev, new;
 
         /* If not already pinned, check the grant domid and type. */
-        if ( !act->pin && (((scombo.flags & mask) != GTF_permit_access) ||
-                           (scombo.domid != ldomid)) )
+        if ( !act->pin &&
+             (((scombo.flags & GTF_type_mask) != GTF_permit_access) ||
+              (scombo.domid != ldomid)) )
             PIN_FAIL(done, GNTST_general_error,
                      "Bad flags (%x) or dom (%d); expected d%d\n",
                      scombo.flags, scombo.domid, ldomid);
@@ -916,7 +910,7 @@ static int _set_status(const grant_entry
 {
 
     if ( evaluate_nospec(rgt_version == 1) )
-        return _set_status_v1(shah, rd, act, readonly, mapflag, ldomid);
+        return _set_status_v1(shah, rd, act, readonly, ldomid);
     else
         return _set_status_v2(shah, status, rd, act, readonly, mapflag, ldomid);
 }
--- a/xen/include/public/grant_table.h
+++ b/xen/include/public/grant_table.h
@@ -175,7 +175,7 @@ typedef struct grant_entry_v1 grant_entr
  *                             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]
+ *                map it. [GST, v2]
  */
 #define _GTF_readonly       (2)
 #define GTF_readonly        (1U<<_GTF_readonly)

Re: [PATCH 3/3] gnttab: GTF_sub_page is a v2-only flag

[Andrew Cooper]

On 17/02/2021 10:46, Jan Beulich wrote:
> Prior to its introduction, v1 entries weren't checked for this flag, and
> the flag also has been meaningless for v1 entries. Therefore it also
> shouldn't be checked. (The only consistent alternative would be to also
> check for all currently undefined flags to be clear.)

We recently had a similar discussion for the stable libs.

Whatever we do, an unexpected corner case needs to break.  Checking for
all undefined flags up front is far cleaner - absolutely nothing good
can come for a guest which set GTF_sub_page with v1, and is expecting it
to work, and this way, we do all breaking in one go, rather than
breaking $N times in the future as new flags get added.

~Andrew

Re: [PATCH 3/3] gnttab: GTF_sub_page is a v2-only flag

[Jan Beulich]

On 18.02.2021 15:22, Andrew Cooper wrote:
> On 17/02/2021 10:46, Jan Beulich wrote:
>> Prior to its introduction, v1 entries weren't checked for this flag, and
>> the flag also has been meaningless for v1 entries. Therefore it also
>> shouldn't be checked. (The only consistent alternative would be to also
>> check for all currently undefined flags to be clear.)
> 
> We recently had a similar discussion for the stable libs.
> 
> Whatever we do, an unexpected corner case needs to break.  Checking for
> all undefined flags up front is far cleaner - absolutely nothing good
> can come for a guest which set GTF_sub_page with v1, and is expecting it
> to work, and this way, we do all breaking in one go, rather than
> breaking $N times in the future as new flags get added.

Except that there doesn't need to be any breaking: v1 could continue
to ignore all originally undefined flags. v1 and v2 could continue to
ignore all flags presently undefined. See also Julien's question
about GTF_sub_page on a transitive grant entry. That's presently an
ignored setting as well (and, as an implementation detail, in fact
getting forced to be set, but with a range covering the entire page).

Retroactively starting to check (and reject) undefined flags isn't
going to be nice; nevertheless I wanted to put this up as an at
least theoretical alternative. Perhaps a topic for the next community
call, if I don't forget by the time an agenda page appears.

Jan