[PATCH 0/2] grant table and add-to-physmap adjustments on top of XSAs 379 and 384

[PATCH 0/2] grant table and add-to-physmap adjustments on top of XSAs 379 and 384

[Jan Beulich]

I'm prepared for the "how" aspect of the 1st patch here to end up
controversial. Since the observed quirk will imo want dealing with,
I'd appreciate any objection to the proposed change to be accompanied
by an alternative suggestion. An intention of mine was to not further
increase the number of arch hooks needed. I further realize that this
change conflicts with Oleksandr's "xen/gnttab: Store frame GFN in
struct page_info on Arm", at the very least contextually.

1: gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame()
2: memory: XENMEM_add_to_physmap (almost) wrapping checks

Jan

[PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame()

[Jan Beulich]

Without holding appropriate locks, attempting to remove a prior mapping
of the underlying page is pointless, as the same (or another) mapping
could be re-established by a parallel request on another vCPU. Move the
code to Arm's gnttab_set_frame_gfn(). Of course this new placement
doesn't improve things in any way as far as the security of grant status
frame mappings goes (see XSA-379). Proper locking would be needed here
to allow status frames to be mapped securely.

In turn this then requires replacing the other use in
gnttab_unpopulate_status_frames(), which yet in turn requires adjusting
x86's gnttab_set_frame_gfn(). Note that with proper locking inside
guest_physmap_remove_page() combined with checking the GFN's mapping
there against the passed in MFN, there then is no issue with the
involved multiple gnttab_set_frame_gfn()-s potentially returning varying
values (due to a racing XENMAPSPACE_grant_table request).

This, as a side effect, does away with gnttab_map_frame() having a local
variable "gfn" which shadows a function parameter of the same name.

Together with XSA-379 this points out that XSA-255's addition to
gnttab_map_frame() was really useless.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -92,7 +92,7 @@ struct grant_table {
     struct radix_tree_root maptrack_tree;
 
     /* Domain to which this struct grant_table belongs. */
-    const struct domain *domain;
+    struct domain *domain;
 
     struct grant_table_arch arch;
 };
@@ -1795,8 +1795,8 @@ gnttab_unpopulate_status_frames(struct d
         {
             int rc = gfn_eq(gfn, INVALID_GFN)
                      ? 0
-                     : guest_physmap_remove_page(d, gfn,
-                                                 page_to_mfn(pg), 0);
+                     : gnttab_set_frame_gfn(gt, true, i, INVALID_GFN,
+                                            page_to_mfn(pg));
 
             if ( rc )
             {
@@ -1806,7 +1806,6 @@ gnttab_unpopulate_status_frames(struct d
                 domain_crash(d);
                 return rc;
             }
-            gnttab_set_frame_gfn(gt, true, i, INVALID_GFN);
         }
 
         BUG_ON(page_get_owner(pg) != d);
@@ -4132,6 +4131,9 @@ int gnttab_map_frame(struct domain *d, u
     struct grant_table *gt = d->grant_table;
     bool status = false;
 
+    if ( gfn_eq(gfn, INVALID_GFN) )
+        return -EINVAL;
+
     grant_write_lock(gt);
 
     if ( evaluate_nospec(gt->gt_version == 2) && (idx & XENMAPIDX_grant_table_status) )
@@ -4144,24 +4146,18 @@ int gnttab_map_frame(struct domain *d, u
     else
         rc = gnttab_get_shared_frame_mfn(d, idx, mfn);
 
-    if ( !rc && paging_mode_translate(d) )
-    {
-        gfn_t gfn = gnttab_get_frame_gfn(gt, status, idx);
-
-        if ( !gfn_eq(gfn, INVALID_GFN) )
-            rc = guest_physmap_remove_page(d, gfn, *mfn, 0);
-    }
-
     if ( !rc )
     {
+        struct page_info *pg = mfn_to_page(*mfn);
+
         /*
          * Make sure gnttab_unpopulate_status_frames() won't (successfully)
          * free the page until our caller has completed its operation.
          */
-        if ( get_page(mfn_to_page(*mfn), d) )
-            gnttab_set_frame_gfn(gt, status, idx, gfn);
-        else
+        if ( !get_page(pg, d) )
             rc = -EBUSY;
+        else if ( (rc = gnttab_set_frame_gfn(gt, status, idx, gfn, *mfn)) )
+            put_page(pg);
     }
 
     grant_write_unlock(gt);
--- a/xen/include/asm-arm/grant_table.h
+++ b/xen/include/asm-arm/grant_table.h
@@ -71,11 +71,17 @@ int replace_grant_host_mapping(unsigned
         XFREE((gt)->arch.status_gfn);                                    \
     } while ( 0 )
 
-#define gnttab_set_frame_gfn(gt, st, idx, gfn)                           \
-    do {                                                                 \
-        ((st) ? (gt)->arch.status_gfn : (gt)->arch.shared_gfn)[idx] =    \
-            (gfn);                                                       \
-    } while ( 0 )
+#define gnttab_set_frame_gfn(gt, st, idx, gfn, mfn)                      \
+    ({                                                                   \
+        int rc_ = 0;                                                     \
+        gfn_t ogfn = gnttab_get_frame_gfn(gt, st, idx);                  \
+        if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) ||           \
+             (rc_ = guest_physmap_remove_page((gt)->domain, ogfn, mfn,   \
+                                              0)) == 0 )                 \
+            ((st) ? (gt)->arch.status_gfn                                \
+                  : (gt)->arch.shared_gfn)[idx] = (gfn);                 \
+        rc_;                                                             \
+    })
 
 #define gnttab_get_frame_gfn(gt, st, idx) ({                             \
    (st) ? gnttab_status_gfn(NULL, gt, idx)                               \
--- a/xen/include/asm-x86/grant_table.h
+++ b/xen/include/asm-x86/grant_table.h
@@ -37,7 +37,12 @@ static inline int replace_grant_host_map
 
 #define gnttab_init_arch(gt) 0
 #define gnttab_destroy_arch(gt) do {} while ( 0 )
-#define gnttab_set_frame_gfn(gt, st, idx, gfn) do {} while ( 0 )
+#define gnttab_set_frame_gfn(gt, st, idx, gfn, mfn)                      \
+    (gfn_eq(gfn, INVALID_GFN)                                            \
+     ? guest_physmap_remove_page((gt)->domain,                           \
+                                 gnttab_get_frame_gfn(gt, st, idx),      \
+                                 mfn, 0)                                 \
+     : 0 /* Handled in add_to_physmap_one(). */)
 #define gnttab_get_frame_gfn(gt, st, idx) ({                             \
     mfn_t mfn_ = (st) ? gnttab_status_mfn(gt, idx)                       \
                       : gnttab_shared_mfn(gt, idx);                      \

[PATCH 2/2] memory: XENMEM_add_to_physmap (almost) wrapping checks

[Jan Beulich]

Determining that behavior is correct (i.e. results in failure) for a
passed in GFN equaling INVALID_GFN is non-trivial. Make this quite a
bit more obvious by checking input in generic code - both for singular
requests to not match the value and for range ones to not pass / wrap
through it.

For Arm similarly make more obvious that no wrapping of MFNs passed
for XENMAPSPACE_dev_mmio and thus to map_dev_mmio_region() can occur:
Drop the "nr" parameter of the function to avoid future callers
appearing which might not themselves check for wrapping. Otherwise
the respective ASSERT() in rangeset_contains_range() could trigger.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I find it odd that map_dev_mmio_region() returns success upon
iomem_access_permitted() indicating failure - is this really intended?
As per commit 102984bb1987 introducing it this also was added for ACPI
only - any reason XENMAPSPACE_dev_mmio isn't restricted to CONFIG_ACPI
builds?

I'd be happy to take suggestions towards avoiding the need to #define
_gfn() around the BUILD_BUG_ON() being added. I couldn't think of
anything prettier.

--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1479,7 +1479,7 @@ int xenmem_add_to_physmap_one(
         break;
     }
     case XENMAPSPACE_dev_mmio:
-        rc = map_dev_mmio_region(d, gfn, 1, _mfn(idx));
+        rc = map_dev_mmio_region(d, gfn, _mfn(idx));
         return rc;
 
     default:
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1360,19 +1360,18 @@ int unmap_mmio_regions(struct domain *d,
 
 int map_dev_mmio_region(struct domain *d,
                         gfn_t gfn,
-                        unsigned long nr,
                         mfn_t mfn)
 {
     int res;
 
-    if ( !(nr && iomem_access_permitted(d, mfn_x(mfn), mfn_x(mfn) + nr - 1)) )
+    if ( !iomem_access_permitted(d, mfn_x(mfn), mfn_x(mfn)) )
         return 0;
 
-    res = p2m_insert_mapping(d, gfn, nr, mfn, p2m_mmio_direct_c);
+    res = p2m_insert_mapping(d, gfn, 1, mfn, p2m_mmio_direct_c);
     if ( res < 0 )
     {
-        printk(XENLOG_G_ERR "Unable to map MFNs [%#"PRI_mfn" - %#"PRI_mfn" in Dom%d\n",
-               mfn_x(mfn), mfn_x(mfn) + nr - 1, d->domain_id);
+        printk(XENLOG_G_ERR "Unable to map MFN %#"PRI_mfn" in %pd\n",
+               mfn_x(mfn), d);
         return res;
     }
 
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -4132,7 +4132,10 @@ int gnttab_map_frame(struct domain *d, u
     bool status = false;
 
     if ( gfn_eq(gfn, INVALID_GFN) )
+    {
+        ASSERT_UNREACHABLE();
         return -EINVAL;
+    }
 
     grant_write_lock(gt);
 
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -831,6 +831,9 @@ int xenmem_add_to_physmap(struct domain
         return -EACCES;
     }
 
+    if ( gfn_eq(_gfn(xatp->gpfn), INVALID_GFN) )
+        return -EINVAL;
+
     if ( xatp->space == XENMAPSPACE_gmfn_foreign )
         extra.foreign_domid = DOMID_INVALID;
 
@@ -841,6 +844,15 @@ int xenmem_add_to_physmap(struct domain
     if ( xatp->size < start )
         return -EILSEQ;
 
+    if ( xatp->gpfn + xatp->size < xatp->gpfn ||
+         xatp->idx + xatp->size < xatp->idx )
+    {
+#define _gfn(x) (x)
+        BUILD_BUG_ON(INVALID_GFN + 1);
+#undef _gfn
+        return -EOVERFLOW;
+    }
+
     xatp->idx += start;
     xatp->gpfn += start;
     xatp->size -= start;
@@ -961,6 +973,9 @@ static int xenmem_add_to_physmap_batch(s
                                                extent, 1)) )
             return -EFAULT;
 
+        if ( gfn_eq(_gfn(gpfn), INVALID_GFN) )
+            return -EINVAL;
+
         rc = xenmem_add_to_physmap_one(d, xatpb->space, extra,
                                        idx, _gfn(gpfn));
 
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -297,7 +297,6 @@ int unmap_regions_p2mt(struct domain *d,
 
 int map_dev_mmio_region(struct domain *d,
                         gfn_t gfn,
-                        unsigned long nr,
                         mfn_t mfn);
 
 int guest_physmap_add_entry(struct domain *d,

Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame()

[Roger Pau Monné]

On Mon, Sep 13, 2021 at 08:41:47AM +0200, Jan Beulich wrote:
> Without holding appropriate locks, attempting to remove a prior mapping
> of the underlying page is pointless, as the same (or another) mapping
> could be re-established by a parallel request on another vCPU. Move the
> code to Arm's gnttab_set_frame_gfn(). Of course this new placement
> doesn't improve things in any way as far as the security of grant status
> frame mappings goes (see XSA-379). Proper locking would be needed here
> to allow status frames to be mapped securely.
> 
> In turn this then requires replacing the other use in
> gnttab_unpopulate_status_frames(), which yet in turn requires adjusting
> x86's gnttab_set_frame_gfn(). Note that with proper locking inside
> guest_physmap_remove_page() combined with checking the GFN's mapping
> there against the passed in MFN, there then is no issue with the
> involved multiple gnttab_set_frame_gfn()-s potentially returning varying
> values (due to a racing XENMAPSPACE_grant_table request).
> 
> This, as a side effect, does away with gnttab_map_frame() having a local
> variable "gfn" which shadows a function parameter of the same name.
> 
> Together with XSA-379 this points out that XSA-255's addition to
> gnttab_map_frame() was really useless.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -92,7 +92,7 @@ struct grant_table {
>      struct radix_tree_root maptrack_tree;
>  
>      /* Domain to which this struct grant_table belongs. */
> -    const struct domain *domain;
> +    struct domain *domain;
>  
>      struct grant_table_arch arch;
>  };
> @@ -1795,8 +1795,8 @@ gnttab_unpopulate_status_frames(struct d
>          {
>              int rc = gfn_eq(gfn, INVALID_GFN)
>                       ? 0
> -                     : guest_physmap_remove_page(d, gfn,
> -                                                 page_to_mfn(pg), 0);
> +                     : gnttab_set_frame_gfn(gt, true, i, INVALID_GFN,
> +                                            page_to_mfn(pg));
>  
>              if ( rc )
>              {
> @@ -1806,7 +1806,6 @@ gnttab_unpopulate_status_frames(struct d
>                  domain_crash(d);
>                  return rc;
>              }
> -            gnttab_set_frame_gfn(gt, true, i, INVALID_GFN);
>          }
>  
>          BUG_ON(page_get_owner(pg) != d);
> @@ -4132,6 +4131,9 @@ int gnttab_map_frame(struct domain *d, u
>      struct grant_table *gt = d->grant_table;
>      bool status = false;
>  
> +    if ( gfn_eq(gfn, INVALID_GFN) )
> +        return -EINVAL;
> +
>      grant_write_lock(gt);
>  
>      if ( evaluate_nospec(gt->gt_version == 2) && (idx & XENMAPIDX_grant_table_status) )
> @@ -4144,24 +4146,18 @@ int gnttab_map_frame(struct domain *d, u
>      else
>          rc = gnttab_get_shared_frame_mfn(d, idx, mfn);
>  
> -    if ( !rc && paging_mode_translate(d) )
> -    {
> -        gfn_t gfn = gnttab_get_frame_gfn(gt, status, idx);
> -
> -        if ( !gfn_eq(gfn, INVALID_GFN) )
> -            rc = guest_physmap_remove_page(d, gfn, *mfn, 0);
> -    }
> -
>      if ( !rc )
>      {
> +        struct page_info *pg = mfn_to_page(*mfn);
> +
>          /*
>           * Make sure gnttab_unpopulate_status_frames() won't (successfully)
>           * free the page until our caller has completed its operation.
>           */
> -        if ( get_page(mfn_to_page(*mfn), d) )
> -            gnttab_set_frame_gfn(gt, status, idx, gfn);
> -        else
> +        if ( !get_page(pg, d) )
>              rc = -EBUSY;
> +        else if ( (rc = gnttab_set_frame_gfn(gt, status, idx, gfn, *mfn)) )
> +            put_page(pg);
>      }
>  
>      grant_write_unlock(gt);
> --- a/xen/include/asm-arm/grant_table.h
> +++ b/xen/include/asm-arm/grant_table.h
> @@ -71,11 +71,17 @@ int replace_grant_host_mapping(unsigned
>          XFREE((gt)->arch.status_gfn);                                    \
>      } while ( 0 )
>  
> -#define gnttab_set_frame_gfn(gt, st, idx, gfn)                           \
> -    do {                                                                 \
> -        ((st) ? (gt)->arch.status_gfn : (gt)->arch.shared_gfn)[idx] =    \
> -            (gfn);                                                       \
> -    } while ( 0 )
> +#define gnttab_set_frame_gfn(gt, st, idx, gfn, mfn)                      \
> +    ({                                                                   \
> +        int rc_ = 0;                                                     \
> +        gfn_t ogfn = gnttab_get_frame_gfn(gt, st, idx);                  \

Newline maybe? Not sure whether we decided that macros should also
follow coding style regarding variable definition followed by newline.

> +        if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) ||           \

I'm slightly confused by this checks, don't you need to check for
gfn_eq(gfn, INVALID_GFN) (not ogfn) in order to call
guest_physmap_remove_page?

Or assuming that ogfn is not invalid can be used to imply a removal?

Also the check for ogfn == gfn is only used on Arm, while I would
assume a similar one would be needed on x86 to guarantee the same
behavior?

Thanks, Roger.

Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame()

[Jan Beulich]

On 20.09.2021 12:20, Roger Pau Monné wrote:
> On Mon, Sep 13, 2021 at 08:41:47AM +0200, Jan Beulich wrote:
>> --- a/xen/include/asm-arm/grant_table.h
>> +++ b/xen/include/asm-arm/grant_table.h
>> @@ -71,11 +71,17 @@ int replace_grant_host_mapping(unsigned
>>          XFREE((gt)->arch.status_gfn);                                    \
>>      } while ( 0 )
>>  
>> -#define gnttab_set_frame_gfn(gt, st, idx, gfn)                           \
>> -    do {                                                                 \
>> -        ((st) ? (gt)->arch.status_gfn : (gt)->arch.shared_gfn)[idx] =    \
>> -            (gfn);                                                       \
>> -    } while ( 0 )
>> +#define gnttab_set_frame_gfn(gt, st, idx, gfn, mfn)                      \
>> +    ({                                                                   \
>> +        int rc_ = 0;                                                     \
>> +        gfn_t ogfn = gnttab_get_frame_gfn(gt, st, idx);                  \
> 
> Newline maybe? Not sure whether we decided that macros should also
> follow coding style regarding variable definition followed by newline.

To be honest in macros I'm always on the fence: A long line of all blanks
and just a trailing backslash is about as ugly to me as the missing
separator. I think normally I opt for the way chosen above, but I'm not
going to claim to know I'm consistent with this.

>> +        if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) ||           \
> 
> I'm slightly confused by this checks, don't you need to check for
> gfn_eq(gfn, INVALID_GFN) (not ogfn) in order to call
> guest_physmap_remove_page?

Why? It's ogfn which gets passed to the function. And it indeed is the
prior GFN's mapping that we want to remove here.

> Or assuming that ogfn is not invalid can be used to imply a removal?

That implication can be (and on x86 is) used for the incoming argument,
i.e. "gfn". I don't think "ogfn" can serve this purpose.

> Also the check for ogfn == gfn is only used on Arm, while I would
> assume a similar one would be needed on x86 to guarantee the same
> behavior?

Things are sufficiently different on Arm and x86. As said above, on
x86 I'm indeed using "gfn" being INVALID_GFN as an indication that a
removal is requested. This is simply transforming the behavior from
prior to this change, with the function invocation moved into the
per-arch macros. It may be relevant to note that
gnttab_unpopulate_status_frames() calls gnttab_set_frame_gfn() with
INVALID_GFN only when gnttab_get_frame_gfn() didn't return
INVALID_GFN, i.e. the gnttab_get_frame_gfn() used in
gnttab_set_frame_gfn() won't return this value either (we've not
dropped any locks in between). And the only other caller of
gnttab_set_frame_gfn() guarantees "gfn" not to be INVALID_GFN. A
little fragile (towards hypothetical further callers of the macro/
function), yes, but I couldn't think of anything substantially
better.

Jan

Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame()

[Roger Pau Monné]

On Mon, Sep 20, 2021 at 05:27:17PM +0200, Jan Beulich wrote:
> On 20.09.2021 12:20, Roger Pau Monné wrote:
> > On Mon, Sep 13, 2021 at 08:41:47AM +0200, Jan Beulich wrote:
> >> --- a/xen/include/asm-arm/grant_table.h
> >> +++ b/xen/include/asm-arm/grant_table.h
> >> +        if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) ||           \
> > 
> > I'm slightly confused by this checks, don't you need to check for
> > gfn_eq(gfn, INVALID_GFN) (not ogfn) in order to call
> > guest_physmap_remove_page?
> 
> Why? It's ogfn which gets passed to the function. And it indeed is the
> prior GFN's mapping that we want to remove here.
> 
> > Or assuming that ogfn is not invalid can be used to imply a removal?
> 
> That implication can be (and on x86 is) used for the incoming argument,
> i.e. "gfn". I don't think "ogfn" can serve this purpose.

I guess I'm confused due to the ogfn checks done on the Arm side that
are not performed on x86. So on Arm you always need to explicitly
unhook the previous GFN before attempting to setup a new mapping,
while on x86 you only need to do this when it's a removal in order to
clear the entry?

So you are effectively only removing the call to
guest_physmap_remove_page in gnttab_map_frame for x86, because Arm
will still perform it in gnttab_set_frame_gfn.

This seems like a limitation of Arm's guest_physmap_add_entry.

Thanks, Roger.

Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame()

[Jan Beulich]

On 21.09.2021 10:32, Roger Pau Monné wrote:
> On Mon, Sep 20, 2021 at 05:27:17PM +0200, Jan Beulich wrote:
>> On 20.09.2021 12:20, Roger Pau Monné wrote:
>>> On Mon, Sep 13, 2021 at 08:41:47AM +0200, Jan Beulich wrote:
>>>> --- a/xen/include/asm-arm/grant_table.h
>>>> +++ b/xen/include/asm-arm/grant_table.h
>>>> +        if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) ||           \
>>>
>>> I'm slightly confused by this checks, don't you need to check for
>>> gfn_eq(gfn, INVALID_GFN) (not ogfn) in order to call
>>> guest_physmap_remove_page?
>>
>> Why? It's ogfn which gets passed to the function. And it indeed is the
>> prior GFN's mapping that we want to remove here.
>>
>>> Or assuming that ogfn is not invalid can be used to imply a removal?
>>
>> That implication can be (and on x86 is) used for the incoming argument,
>> i.e. "gfn". I don't think "ogfn" can serve this purpose.
> 
> I guess I'm confused due to the ogfn checks done on the Arm side that
> are not performed on x86. So on Arm you always need to explicitly
> unhook the previous GFN before attempting to setup a new mapping,
> while on x86 you only need to do this when it's a removal in order to
> clear the entry?

The difference isn't with guest_physmap_add_entry() (both x86 and
Arm only insert a new mapping there), but with
xenmem_add_to_physmap_one(): Arm's variant doesn't care about prior
mappings. And gnttab_map_frame() gets called only from there. This
is effectively what the first paragraph of the description is about.

> So you are effectively only removing the call to
> guest_physmap_remove_page in gnttab_map_frame for x86, because Arm
> will still perform it in gnttab_set_frame_gfn.

Yes.

> This seems like a limitation of Arm's guest_physmap_add_entry.

As per above I'm viewing this as a limitation of Arm's
xenmem_add_to_physmap_one().

Jan

Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame()

[Roger Pau Monné]

On Tue, Sep 21, 2021 at 12:12:05PM +0200, Jan Beulich wrote:
> On 21.09.2021 10:32, Roger Pau Monné wrote:
> > On Mon, Sep 20, 2021 at 05:27:17PM +0200, Jan Beulich wrote:
> >> On 20.09.2021 12:20, Roger Pau Monné wrote:
> >>> On Mon, Sep 13, 2021 at 08:41:47AM +0200, Jan Beulich wrote:
> >>>> --- a/xen/include/asm-arm/grant_table.h
> >>>> +++ b/xen/include/asm-arm/grant_table.h
> >>>> +        if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) ||           \
> >>>
> >>> I'm slightly confused by this checks, don't you need to check for
> >>> gfn_eq(gfn, INVALID_GFN) (not ogfn) in order to call
> >>> guest_physmap_remove_page?
> >>
> >> Why? It's ogfn which gets passed to the function. And it indeed is the
> >> prior GFN's mapping that we want to remove here.
> >>
> >>> Or assuming that ogfn is not invalid can be used to imply a removal?
> >>
> >> That implication can be (and on x86 is) used for the incoming argument,
> >> i.e. "gfn". I don't think "ogfn" can serve this purpose.
> > 
> > I guess I'm confused due to the ogfn checks done on the Arm side that
> > are not performed on x86. So on Arm you always need to explicitly
> > unhook the previous GFN before attempting to setup a new mapping,
> > while on x86 you only need to do this when it's a removal in order to
> > clear the entry?
> 
> The difference isn't with guest_physmap_add_entry() (both x86 and
> Arm only insert a new mapping there), but with
> xenmem_add_to_physmap_one(): Arm's variant doesn't care about prior
> mappings. And gnttab_map_frame() gets called only from there. This
> is effectively what the first paragraph of the description is about.

OK, sorry, it wasn't clear to me from the description. Could you
explicitly mention in the description that the removal is moved into
gnttab_set_frame_gfn on Arm in order to cope with the fact that
xenmem_add_to_physmap_one doesn't perform it.

TBH I think it would be in our best interest to try to make
xenmem_add_to_physmap_one behave as close as possible between arches.
This discrepancy between x86 and Arm regarding page removal is just
going to bring more trouble in the long term, and hiding the
differences inside gnttab_set_frame_gfn just makes it even more
obscure.

Thanks, Roger.

Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame()

[Jan Beulich]

On 22.09.2021 11:26, Roger Pau Monné wrote:
> On Tue, Sep 21, 2021 at 12:12:05PM +0200, Jan Beulich wrote:
>> On 21.09.2021 10:32, Roger Pau Monné wrote:
>>> On Mon, Sep 20, 2021 at 05:27:17PM +0200, Jan Beulich wrote:
>>>> On 20.09.2021 12:20, Roger Pau Monné wrote:
>>>>> On Mon, Sep 13, 2021 at 08:41:47AM +0200, Jan Beulich wrote:
>>>>>> --- a/xen/include/asm-arm/grant_table.h
>>>>>> +++ b/xen/include/asm-arm/grant_table.h
>>>>>> +        if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) ||           \
>>>>>
>>>>> I'm slightly confused by this checks, don't you need to check for
>>>>> gfn_eq(gfn, INVALID_GFN) (not ogfn) in order to call
>>>>> guest_physmap_remove_page?
>>>>
>>>> Why? It's ogfn which gets passed to the function. And it indeed is the
>>>> prior GFN's mapping that we want to remove here.
>>>>
>>>>> Or assuming that ogfn is not invalid can be used to imply a removal?
>>>>
>>>> That implication can be (and on x86 is) used for the incoming argument,
>>>> i.e. "gfn". I don't think "ogfn" can serve this purpose.
>>>
>>> I guess I'm confused due to the ogfn checks done on the Arm side that
>>> are not performed on x86. So on Arm you always need to explicitly
>>> unhook the previous GFN before attempting to setup a new mapping,
>>> while on x86 you only need to do this when it's a removal in order to
>>> clear the entry?
>>
>> The difference isn't with guest_physmap_add_entry() (both x86 and
>> Arm only insert a new mapping there), but with
>> xenmem_add_to_physmap_one(): Arm's variant doesn't care about prior
>> mappings. And gnttab_map_frame() gets called only from there. This
>> is effectively what the first paragraph of the description is about.
> 
> OK, sorry, it wasn't clear to me from the description. Could you
> explicitly mention in the description that the removal is moved into
> gnttab_set_frame_gfn on Arm in order to cope with the fact that
> xenmem_add_to_physmap_one doesn't perform it.

Well, it's not really "in order to cope" - that's true for the placement
prior to this change as well, so not a justification for the change.
Nevertheless I've tried to make this more clear by changing the 1st
paragraph to:

"Without holding appropriate locks, attempting to remove a prior mapping
 of the underlying page is pointless, as the same (or another) mapping
 could be re-established by a parallel request on another vCPU. Move the
 code to Arm's gnttab_set_frame_gfn(); it cannot be dropped there since
 xenmem_add_to_physmap_one() doesn't call it either (unlike on x86). Of
 course this new placement doesn't improve things in any way as far as
 the security of grant status frame mappings goes (see XSA-379). Proper
 locking would be needed here to allow status frames to be mapped
 securely."

> TBH I think it would be in our best interest to try to make
> xenmem_add_to_physmap_one behave as close as possible between arches.
> This discrepancy between x86 and Arm regarding page removal is just
> going to bring more trouble in the long term, and hiding the
> differences inside gnttab_set_frame_gfn just makes it even more
> obscure.

Stefano, Julien?

Jan

Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame()

[Roger Pau Monné]

On Wed, Sep 22, 2021 at 11:42:30AM +0200, Jan Beulich wrote:
> On 22.09.2021 11:26, Roger Pau Monné wrote:
> > On Tue, Sep 21, 2021 at 12:12:05PM +0200, Jan Beulich wrote:
> >> On 21.09.2021 10:32, Roger Pau Monné wrote:
> >>> On Mon, Sep 20, 2021 at 05:27:17PM +0200, Jan Beulich wrote:
> >>>> On 20.09.2021 12:20, Roger Pau Monné wrote:
> >>>>> On Mon, Sep 13, 2021 at 08:41:47AM +0200, Jan Beulich wrote:
> >>>>>> --- a/xen/include/asm-arm/grant_table.h
> >>>>>> +++ b/xen/include/asm-arm/grant_table.h
> >>>>>> +        if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) ||           \
> >>>>>
> >>>>> I'm slightly confused by this checks, don't you need to check for
> >>>>> gfn_eq(gfn, INVALID_GFN) (not ogfn) in order to call
> >>>>> guest_physmap_remove_page?
> >>>>
> >>>> Why? It's ogfn which gets passed to the function. And it indeed is the
> >>>> prior GFN's mapping that we want to remove here.
> >>>>
> >>>>> Or assuming that ogfn is not invalid can be used to imply a removal?
> >>>>
> >>>> That implication can be (and on x86 is) used for the incoming argument,
> >>>> i.e. "gfn". I don't think "ogfn" can serve this purpose.
> >>>
> >>> I guess I'm confused due to the ogfn checks done on the Arm side that
> >>> are not performed on x86. So on Arm you always need to explicitly
> >>> unhook the previous GFN before attempting to setup a new mapping,
> >>> while on x86 you only need to do this when it's a removal in order to
> >>> clear the entry?
> >>
> >> The difference isn't with guest_physmap_add_entry() (both x86 and
> >> Arm only insert a new mapping there), but with
> >> xenmem_add_to_physmap_one(): Arm's variant doesn't care about prior
> >> mappings. And gnttab_map_frame() gets called only from there. This
> >> is effectively what the first paragraph of the description is about.
> > 
> > OK, sorry, it wasn't clear to me from the description. Could you
> > explicitly mention in the description that the removal is moved into
> > gnttab_set_frame_gfn on Arm in order to cope with the fact that
> > xenmem_add_to_physmap_one doesn't perform it.
> 
> Well, it's not really "in order to cope" - that's true for the placement
> prior to this change as well, so not a justification for the change.
> Nevertheless I've tried to make this more clear by changing the 1st
> paragraph to:
> 
> "Without holding appropriate locks, attempting to remove a prior mapping
>  of the underlying page is pointless, as the same (or another) mapping
>  could be re-established by a parallel request on another vCPU. Move the
>  code to Arm's gnttab_set_frame_gfn(); it cannot be dropped there since
>  xenmem_add_to_physmap_one() doesn't call it either (unlike on x86). Of
>  course this new placement doesn't improve things in any way as far as
>  the security of grant status frame mappings goes (see XSA-379). Proper
>  locking would be needed here to allow status frames to be mapped
>  securely."

Thanks, this is indeed much clearer IMO:

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Albeit I still think we need to fix Arm side to do the removal in
xenmem_add_to_physmap_one (or the x86 side to not do it).

Thanks, Roger.

Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame()

[Julien Grall]

Hi,

On 22/09/2021 14:42, Jan Beulich wrote:
> On 22.09.2021 11:26, Roger Pau Monné wrote:
>> On Tue, Sep 21, 2021 at 12:12:05PM +0200, Jan Beulich wrote:
>>> On 21.09.2021 10:32, Roger Pau Monné wrote:
>>>> On Mon, Sep 20, 2021 at 05:27:17PM +0200, Jan Beulich wrote:
>>>>> On 20.09.2021 12:20, Roger Pau Monné wrote:
>>>>>> On Mon, Sep 13, 2021 at 08:41:47AM +0200, Jan Beulich wrote:
>>>>>>> --- a/xen/include/asm-arm/grant_table.h
>>>>>>> +++ b/xen/include/asm-arm/grant_table.h
>>>>>>> +        if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) ||           \
>>>>>>
>>>>>> I'm slightly confused by this checks, don't you need to check for
>>>>>> gfn_eq(gfn, INVALID_GFN) (not ogfn) in order to call
>>>>>> guest_physmap_remove_page?
>>>>>
>>>>> Why? It's ogfn which gets passed to the function. And it indeed is the
>>>>> prior GFN's mapping that we want to remove here.
>>>>>
>>>>>> Or assuming that ogfn is not invalid can be used to imply a removal?
>>>>>
>>>>> That implication can be (and on x86 is) used for the incoming argument,
>>>>> i.e. "gfn". I don't think "ogfn" can serve this purpose.
>>>>
>>>> I guess I'm confused due to the ogfn checks done on the Arm side that
>>>> are not performed on x86. So on Arm you always need to explicitly
>>>> unhook the previous GFN before attempting to setup a new mapping,
>>>> while on x86 you only need to do this when it's a removal in order to
>>>> clear the entry?
>>>
>>> The difference isn't with guest_physmap_add_entry() (both x86 and
>>> Arm only insert a new mapping there), but with
>>> xenmem_add_to_physmap_one(): Arm's variant doesn't care about prior
>>> mappings. And gnttab_map_frame() gets called only from there. This
>>> is effectively what the first paragraph of the description is about.
>>
>> OK, sorry, it wasn't clear to me from the description. Could you
>> explicitly mention in the description that the removal is moved into
>> gnttab_set_frame_gfn on Arm in order to cope with the fact that
>> xenmem_add_to_physmap_one doesn't perform it.
> 
> Well, it's not really "in order to cope" - that's true for the placement
> prior to this change as well, so not a justification for the change.
> Nevertheless I've tried to make this more clear by changing the 1st
> paragraph to:
> 
> "Without holding appropriate locks, attempting to remove a prior mapping
>   of the underlying page is pointless, as the same (or another) mapping
>   could be re-established by a parallel request on another vCPU. Move the
>   code to Arm's gnttab_set_frame_gfn(); it cannot be dropped there since
>   xenmem_add_to_physmap_one() doesn't call it either (unlike on x86). Of
>   course this new placement doesn't improve things in any way as far as
>   the security of grant status frame mappings goes (see XSA-379). Proper
>   locking would be needed here to allow status frames to be mapped
>   securely."
> 
>> TBH I think it would be in our best interest to try to make
>> xenmem_add_to_physmap_one behave as close as possible between arches.
>> This discrepancy between x86 and Arm regarding page removal is just
>> going to bring more trouble in the long term, and hiding the
>> differences inside gnttab_set_frame_gfn just makes it even more
>> obscure.
> 
> Stefano, Julien?

This would be ideal as I don't particular like the approach taken in 
this patch. But AFAICT, this would require us to implement an M2P. Or is 
there another way to do it?

In another context, I saw the suggestion to bring an M2P on Arm. But I 
am still somewhat split whether this is really worth it for the current use.

Cheers,

-- 
Julien Grall

Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame()

[Jan Beulich]

On 22.09.2021 12:28, Julien Grall wrote:
> Hi,
> 
> On 22/09/2021 14:42, Jan Beulich wrote:
>> On 22.09.2021 11:26, Roger Pau Monné wrote:
>>> On Tue, Sep 21, 2021 at 12:12:05PM +0200, Jan Beulich wrote:
>>>> On 21.09.2021 10:32, Roger Pau Monné wrote:
>>>>> On Mon, Sep 20, 2021 at 05:27:17PM +0200, Jan Beulich wrote:
>>>>>> On 20.09.2021 12:20, Roger Pau Monné wrote:
>>>>>>> On Mon, Sep 13, 2021 at 08:41:47AM +0200, Jan Beulich wrote:
>>>>>>>> --- a/xen/include/asm-arm/grant_table.h
>>>>>>>> +++ b/xen/include/asm-arm/grant_table.h
>>>>>>>> +        if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) ||           \
>>>>>>>
>>>>>>> I'm slightly confused by this checks, don't you need to check for
>>>>>>> gfn_eq(gfn, INVALID_GFN) (not ogfn) in order to call
>>>>>>> guest_physmap_remove_page?
>>>>>>
>>>>>> Why? It's ogfn which gets passed to the function. And it indeed is the
>>>>>> prior GFN's mapping that we want to remove here.
>>>>>>
>>>>>>> Or assuming that ogfn is not invalid can be used to imply a removal?
>>>>>>
>>>>>> That implication can be (and on x86 is) used for the incoming argument,
>>>>>> i.e. "gfn". I don't think "ogfn" can serve this purpose.
>>>>>
>>>>> I guess I'm confused due to the ogfn checks done on the Arm side that
>>>>> are not performed on x86. So on Arm you always need to explicitly
>>>>> unhook the previous GFN before attempting to setup a new mapping,
>>>>> while on x86 you only need to do this when it's a removal in order to
>>>>> clear the entry?
>>>>
>>>> The difference isn't with guest_physmap_add_entry() (both x86 and
>>>> Arm only insert a new mapping there), but with
>>>> xenmem_add_to_physmap_one(): Arm's variant doesn't care about prior
>>>> mappings. And gnttab_map_frame() gets called only from there. This
>>>> is effectively what the first paragraph of the description is about.
>>>
>>> OK, sorry, it wasn't clear to me from the description. Could you
>>> explicitly mention in the description that the removal is moved into
>>> gnttab_set_frame_gfn on Arm in order to cope with the fact that
>>> xenmem_add_to_physmap_one doesn't perform it.
>>
>> Well, it's not really "in order to cope" - that's true for the placement
>> prior to this change as well, so not a justification for the change.
>> Nevertheless I've tried to make this more clear by changing the 1st
>> paragraph to:
>>
>> "Without holding appropriate locks, attempting to remove a prior mapping
>>   of the underlying page is pointless, as the same (or another) mapping
>>   could be re-established by a parallel request on another vCPU. Move the
>>   code to Arm's gnttab_set_frame_gfn(); it cannot be dropped there since
>>   xenmem_add_to_physmap_one() doesn't call it either (unlike on x86). Of
>>   course this new placement doesn't improve things in any way as far as
>>   the security of grant status frame mappings goes (see XSA-379). Proper
>>   locking would be needed here to allow status frames to be mapped
>>   securely."
>>
>>> TBH I think it would be in our best interest to try to make
>>> xenmem_add_to_physmap_one behave as close as possible between arches.
>>> This discrepancy between x86 and Arm regarding page removal is just
>>> going to bring more trouble in the long term, and hiding the
>>> differences inside gnttab_set_frame_gfn just makes it even more
>>> obscure.
>>
>> Stefano, Julien?
> 
> This would be ideal as I don't particular like the approach taken in 
> this patch. But AFAICT, this would require us to implement an M2P. Or is 
> there another way to do it?

For the purpose of just XENMAPSPACE_grant_table the "restricted" M2P
(to just grant table pages), as introduced by an on-list patch, would
do afaict. That wouldn't remove all the differences between the two
implementations, but at least the one affecting the difference in how
gnttab_map_frame() needs to behave.

Jan

Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame()

[Julien Grall]

Hi Jan,

On 22/09/2021 15:47, Jan Beulich wrote:
> On 22.09.2021 12:28, Julien Grall wrote:
>> Hi,
>>
>> On 22/09/2021 14:42, Jan Beulich wrote:
>>> On 22.09.2021 11:26, Roger Pau Monné wrote:
>>>> On Tue, Sep 21, 2021 at 12:12:05PM +0200, Jan Beulich wrote:
>>>>> On 21.09.2021 10:32, Roger Pau Monné wrote:
>>>>>> On Mon, Sep 20, 2021 at 05:27:17PM +0200, Jan Beulich wrote:
>>>>>>> On 20.09.2021 12:20, Roger Pau Monné wrote:
>>>>>>>> On Mon, Sep 13, 2021 at 08:41:47AM +0200, Jan Beulich wrote:
>>>>>>>>> --- a/xen/include/asm-arm/grant_table.h
>>>>>>>>> +++ b/xen/include/asm-arm/grant_table.h
>>>>>>>>> +        if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) ||           \
>>>>>>>>
>>>>>>>> I'm slightly confused by this checks, don't you need to check for
>>>>>>>> gfn_eq(gfn, INVALID_GFN) (not ogfn) in order to call
>>>>>>>> guest_physmap_remove_page?
>>>>>>>
>>>>>>> Why? It's ogfn which gets passed to the function. And it indeed is the
>>>>>>> prior GFN's mapping that we want to remove here.
>>>>>>>
>>>>>>>> Or assuming that ogfn is not invalid can be used to imply a removal?
>>>>>>>
>>>>>>> That implication can be (and on x86 is) used for the incoming argument,
>>>>>>> i.e. "gfn". I don't think "ogfn" can serve this purpose.
>>>>>>
>>>>>> I guess I'm confused due to the ogfn checks done on the Arm side that
>>>>>> are not performed on x86. So on Arm you always need to explicitly
>>>>>> unhook the previous GFN before attempting to setup a new mapping,
>>>>>> while on x86 you only need to do this when it's a removal in order to
>>>>>> clear the entry?
>>>>>
>>>>> The difference isn't with guest_physmap_add_entry() (both x86 and
>>>>> Arm only insert a new mapping there), but with
>>>>> xenmem_add_to_physmap_one(): Arm's variant doesn't care about prior
>>>>> mappings. And gnttab_map_frame() gets called only from there. This
>>>>> is effectively what the first paragraph of the description is about.
>>>>
>>>> OK, sorry, it wasn't clear to me from the description. Could you
>>>> explicitly mention in the description that the removal is moved into
>>>> gnttab_set_frame_gfn on Arm in order to cope with the fact that
>>>> xenmem_add_to_physmap_one doesn't perform it.
>>>
>>> Well, it's not really "in order to cope" - that's true for the placement
>>> prior to this change as well, so not a justification for the change.
>>> Nevertheless I've tried to make this more clear by changing the 1st
>>> paragraph to:
>>>
>>> "Without holding appropriate locks, attempting to remove a prior mapping
>>>    of the underlying page is pointless, as the same (or another) mapping
>>>    could be re-established by a parallel request on another vCPU. Move the
>>>    code to Arm's gnttab_set_frame_gfn(); it cannot be dropped there since
>>>    xenmem_add_to_physmap_one() doesn't call it either (unlike on x86). Of
>>>    course this new placement doesn't improve things in any way as far as
>>>    the security of grant status frame mappings goes (see XSA-379). Proper
>>>    locking would be needed here to allow status frames to be mapped
>>>    securely."
>>>
>>>> TBH I think it would be in our best interest to try to make
>>>> xenmem_add_to_physmap_one behave as close as possible between arches.
>>>> This discrepancy between x86 and Arm regarding page removal is just
>>>> going to bring more trouble in the long term, and hiding the
>>>> differences inside gnttab_set_frame_gfn just makes it even more
>>>> obscure.
>>>
>>> Stefano, Julien?
>>
>> This would be ideal as I don't particular like the approach taken in
>> this patch. But AFAICT, this would require us to implement an M2P. Or is
>> there another way to do it?
> 
> For the purpose of just XENMAPSPACE_grant_table the "restricted" M2P
> (to just grant table pages), as introduced by an on-list patch, would
> do afaict. That wouldn't remove all the differences between the two
> implementations, but at least the one affecting the difference in how
> gnttab_map_frame() needs to behave.

This would work. I will add it in my todo list but I am not such when I 
will have time to look at it. I am happy if someone else wants to look 
at it.

Cheers,

-- 
Julien Grall

Re: [PATCH 0/2] grant table and add-to-physmap adjustments on top of XSAs 379 and 384

[Oleksandr]

On 13.09.21 09:39, Jan Beulich wrote:

Hi Jan

> I'm prepared for the "how" aspect of the 1st patch here to end up
> controversial. Since the observed quirk will imo want dealing with,
> I'd appreciate any objection to the proposed change to be accompanied
> by an alternative suggestion. An intention of mine was to not further
> increase the number of arch hooks needed. I further realize that this
> change conflicts with Oleksandr's "xen/gnttab: Store frame GFN in
> struct page_info on Arm", at the very least contextually.
FYI, I have a rebased version of my patch on top of your patch #1 
locally. I preliminary checked that combination on my setup (Arm64) and 
didn't see any issues.


>
> 1: gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame()
> 2: memory: XENMEM_add_to_physmap (almost) wrapping checks
>
> Jan
>
>
-- 
Regards,

Oleksandr Tyshchenko

Ping: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame()

[Jan Beulich]

On 22.09.2021 12:00, Roger Pau Monné wrote:
> On Wed, Sep 22, 2021 at 11:42:30AM +0200, Jan Beulich wrote:
>> On 22.09.2021 11:26, Roger Pau Monné wrote:
>>> On Tue, Sep 21, 2021 at 12:12:05PM +0200, Jan Beulich wrote:
>>>> On 21.09.2021 10:32, Roger Pau Monné wrote:
>>>>> On Mon, Sep 20, 2021 at 05:27:17PM +0200, Jan Beulich wrote:
>>>>>> On 20.09.2021 12:20, Roger Pau Monné wrote:
>>>>>>> On Mon, Sep 13, 2021 at 08:41:47AM +0200, Jan Beulich wrote:
>>>>>>>> --- a/xen/include/asm-arm/grant_table.h
>>>>>>>> +++ b/xen/include/asm-arm/grant_table.h
>>>>>>>> +        if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) ||           \
>>>>>>>
>>>>>>> I'm slightly confused by this checks, don't you need to check for
>>>>>>> gfn_eq(gfn, INVALID_GFN) (not ogfn) in order to call
>>>>>>> guest_physmap_remove_page?
>>>>>>
>>>>>> Why? It's ogfn which gets passed to the function. And it indeed is the
>>>>>> prior GFN's mapping that we want to remove here.
>>>>>>
>>>>>>> Or assuming that ogfn is not invalid can be used to imply a removal?
>>>>>>
>>>>>> That implication can be (and on x86 is) used for the incoming argument,
>>>>>> i.e. "gfn". I don't think "ogfn" can serve this purpose.
>>>>>
>>>>> I guess I'm confused due to the ogfn checks done on the Arm side that
>>>>> are not performed on x86. So on Arm you always need to explicitly
>>>>> unhook the previous GFN before attempting to setup a new mapping,
>>>>> while on x86 you only need to do this when it's a removal in order to
>>>>> clear the entry?
>>>>
>>>> The difference isn't with guest_physmap_add_entry() (both x86 and
>>>> Arm only insert a new mapping there), but with
>>>> xenmem_add_to_physmap_one(): Arm's variant doesn't care about prior
>>>> mappings. And gnttab_map_frame() gets called only from there. This
>>>> is effectively what the first paragraph of the description is about.
>>>
>>> OK, sorry, it wasn't clear to me from the description. Could you
>>> explicitly mention in the description that the removal is moved into
>>> gnttab_set_frame_gfn on Arm in order to cope with the fact that
>>> xenmem_add_to_physmap_one doesn't perform it.
>>
>> Well, it's not really "in order to cope" - that's true for the placement
>> prior to this change as well, so not a justification for the change.
>> Nevertheless I've tried to make this more clear by changing the 1st
>> paragraph to:
>>
>> "Without holding appropriate locks, attempting to remove a prior mapping
>>  of the underlying page is pointless, as the same (or another) mapping
>>  could be re-established by a parallel request on another vCPU. Move the
>>  code to Arm's gnttab_set_frame_gfn(); it cannot be dropped there since
>>  xenmem_add_to_physmap_one() doesn't call it either (unlike on x86). Of
>>  course this new placement doesn't improve things in any way as far as
>>  the security of grant status frame mappings goes (see XSA-379). Proper
>>  locking would be needed here to allow status frames to be mapped
>>  securely."
> 
> Thanks, this is indeed much clearer IMO:
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Any chance of an Arm ack (or otherwise) here?

Thanks, Jan

> Albeit I still think we need to fix Arm side to do the removal in
> xenmem_add_to_physmap_one (or the x86 side to not do it).
> 
> Thanks, Roger.
> 

Re: [PATCH 2/2] memory: XENMEM_add_to_physmap (almost) wrapping checks

[Julien Grall]

Hi Jan,

On 13/09/2021 07:42, Jan Beulich wrote:
> Determining that behavior is correct (i.e. results in failure) for a
> passed in GFN equaling INVALID_GFN is non-trivial. Make this quite a
> bit more obvious by checking input in generic code - both for singular
> requests to not match the value and for range ones to not pass / wrap
> through it.
> 
> For Arm similarly make more obvious that no wrapping of MFNs passed
> for XENMAPSPACE_dev_mmio and thus to map_dev_mmio_region() can occur:
> Drop the "nr" parameter of the function to avoid future callers
> appearing which might not themselves check for wrapping. Otherwise
> the respective ASSERT() in rangeset_contains_range() could trigger.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> I find it odd that map_dev_mmio_region() returns success upon
> iomem_access_permitted() indicating failure - is this really intended?

AFAIR yes. The hypercall is not used as "Map the region" but instead 
"Make sure the region is mapped if the IOMEM region is accessible".

It is necessary to return 0 because dom0 OS cannot distinguished between 
emulated and non-emulated. So we may report error when there is none.

> As per commit 102984bb1987 introducing it this also was added for ACPI
> only - any reason XENMAPSPACE_dev_mmio isn't restricted to CONFIG_ACPI
> builds?

There is nothing specific to ACPI in the implementation. So I don't 
really see the reason to restrict to CONFIG_ACPI.

However, it is still possible to boot using DT when Xen is built with 
CONFIG_ACPI. So if the restriction was desirable, then I think it should 
be using !acpi_disabled.

> 
> I'd be happy to take suggestions towards avoiding the need to #define
> _gfn() around the BUILD_BUG_ON() being added. I couldn't think of
> anything prettier.
> 
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1479,7 +1479,7 @@ int xenmem_add_to_physmap_one(
>           break;
>       }
>       case XENMAPSPACE_dev_mmio:
> -        rc = map_dev_mmio_region(d, gfn, 1, _mfn(idx));
> +        rc = map_dev_mmio_region(d, gfn, _mfn(idx));
>           return rc;
>   
>       default:
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1360,19 +1360,18 @@ int unmap_mmio_regions(struct domain *d,
>   
>   int map_dev_mmio_region(struct domain *d,
>                           gfn_t gfn,
> -                        unsigned long nr,
>                           mfn_t mfn)
>   {
>       int res;
>   
> -    if ( !(nr && iomem_access_permitted(d, mfn_x(mfn), mfn_x(mfn) + nr - 1)) )
> +    if ( !iomem_access_permitted(d, mfn_x(mfn), mfn_x(mfn)) )
>           return 0;
>   
> -    res = p2m_insert_mapping(d, gfn, nr, mfn, p2m_mmio_direct_c);
> +    res = p2m_insert_mapping(d, gfn, 1, mfn, p2m_mmio_direct_c);
>       if ( res < 0 )
>       {
> -        printk(XENLOG_G_ERR "Unable to map MFNs [%#"PRI_mfn" - %#"PRI_mfn" in Dom%d\n",
> -               mfn_x(mfn), mfn_x(mfn) + nr - 1, d->domain_id);
> +        printk(XENLOG_G_ERR "Unable to map MFN %#"PRI_mfn" in %pd\n",
> +               mfn_x(mfn), d);
>           return res;
>       }
>   
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -4132,7 +4132,10 @@ int gnttab_map_frame(struct domain *d, u
>       bool status = false;
>   
>       if ( gfn_eq(gfn, INVALID_GFN) )
> +    {
> +        ASSERT_UNREACHABLE();
>           return -EINVAL;
> +    }
>   
>       grant_write_lock(gt);
>   
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -831,6 +831,9 @@ int xenmem_add_to_physmap(struct domain
>           return -EACCES;
>       }
>   
> +    if ( gfn_eq(_gfn(xatp->gpfn), INVALID_GFN) )
> +        return -EINVAL;
> +
>       if ( xatp->space == XENMAPSPACE_gmfn_foreign )
>           extra.foreign_domid = DOMID_INVALID;
>   
> @@ -841,6 +844,15 @@ int xenmem_add_to_physmap(struct domain
>       if ( xatp->size < start )
>           return -EILSEQ;
>   
> +    if ( xatp->gpfn + xatp->size < xatp->gpfn ||
> +         xatp->idx + xatp->size < xatp->idx )
> +    {
> +#define _gfn(x) (x)

AFAICT, _gfn() will already be defined. So some compiler may complain 
because will be defined differently on debug build. However...

> +        BUILD_BUG_ON(INVALID_GFN + 1);

... I might be missing something... but why can't use gfn_x(INVALID_GFN) 
+ 1 here?

In fact, I am not entirely sure what's the purpose of this 
BUILD_BUG_ON(). Could you give more details?

> +#undef _gfn
> +        return -EOVERFLOW;
> +    }
> +
>       xatp->idx += start;
>       xatp->gpfn += start;
>       xatp->size -= start;
> @@ -961,6 +973,9 @@ static int xenmem_add_to_physmap_batch(s
>                                                  extent, 1)) )
>               return -EFAULT;
>   
> +        if ( gfn_eq(_gfn(gpfn), INVALID_GFN) )
> +            return -EINVAL;
> +
>           rc = xenmem_add_to_physmap_one(d, xatpb->space, extra,
>                                          idx, _gfn(gpfn));
>   
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -297,7 +297,6 @@ int unmap_regions_p2mt(struct domain *d,
>   
>   int map_dev_mmio_region(struct domain *d,
>                           gfn_t gfn,
> -                        unsigned long nr,
>                           mfn_t mfn);
>   
>   int guest_physmap_add_entry(struct domain *d,
> 

Cheers,

-- 
Julien Grall

Re: [PATCH 2/2] memory: XENMEM_add_to_physmap (almost) wrapping checks

[Jan Beulich]

On 14.10.2021 13:29, Julien Grall wrote:
> On 13/09/2021 07:42, Jan Beulich wrote:
>> Determining that behavior is correct (i.e. results in failure) for a
>> passed in GFN equaling INVALID_GFN is non-trivial. Make this quite a
>> bit more obvious by checking input in generic code - both for singular
>> requests to not match the value and for range ones to not pass / wrap
>> through it.
>>
>> For Arm similarly make more obvious that no wrapping of MFNs passed
>> for XENMAPSPACE_dev_mmio and thus to map_dev_mmio_region() can occur:
>> Drop the "nr" parameter of the function to avoid future callers
>> appearing which might not themselves check for wrapping. Otherwise
>> the respective ASSERT() in rangeset_contains_range() could trigger.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> I find it odd that map_dev_mmio_region() returns success upon
>> iomem_access_permitted() indicating failure - is this really intended?
> 
> AFAIR yes. The hypercall is not used as "Map the region" but instead 
> "Make sure the region is mapped if the IOMEM region is accessible".
> 
> It is necessary to return 0 because dom0 OS cannot distinguished between 
> emulated and non-emulated. So we may report error when there is none.

Odd, but I clearly don't understand all the aspects here.

>> As per commit 102984bb1987 introducing it this also was added for ACPI
>> only - any reason XENMAPSPACE_dev_mmio isn't restricted to CONFIG_ACPI
>> builds?
> 
> There is nothing specific to ACPI in the implementation. So I don't 
> really see the reason to restrict to CONFIG_ACPI.
> 
> However, it is still possible to boot using DT when Xen is built with 
> CONFIG_ACPI. So if the restriction was desirable, then I think it should 
> be using !acpi_disabled.

My point was rather about this potentially being dead code in non-ACPI
builds (i.e. in particular uniformly on 32-bit).

>> @@ -841,6 +844,15 @@ int xenmem_add_to_physmap(struct domain
>>       if ( xatp->size < start )
>>           return -EILSEQ;
>>   
>> +    if ( xatp->gpfn + xatp->size < xatp->gpfn ||
>> +         xatp->idx + xatp->size < xatp->idx )
>> +    {
>> +#define _gfn(x) (x)
> 
> AFAICT, _gfn() will already be defined. So some compiler may complain 
> because will be defined differently on debug build.

No - _gfn() is an inline function as per typesafe.h. (Or else it
wouldn't be just "some" compiler, but gcc at least would have
complained to me.)

> However...
> 
>> +        BUILD_BUG_ON(INVALID_GFN + 1);
> 
> ... I might be missing something... but why can't use gfn_x(INVALID_GFN) 
> + 1 here?

Because gfn_x() also is an inline function, and that's not suitable
for a compile-time constant expression.

> In fact, I am not entirely sure what's the purpose of this 
> BUILD_BUG_ON(). Could you give more details?

The expression in the surrounding if() relies on INVALID_GFN being the
largest representable value, i.e. this ensures that INVALID_GFN doesn't
sit anywhere in [xatp->gpfn, xatp->gpfn + xatp->size).

Jan

Re: [PATCH 2/2] memory: XENMEM_add_to_physmap (almost) wrapping checks

[Julien Grall]

Hi,

On 14/10/2021 15:10, Jan Beulich wrote:
> On 14.10.2021 13:29, Julien Grall wrote:
>> On 13/09/2021 07:42, Jan Beulich wrote:
>>> Determining that behavior is correct (i.e. results in failure) for a
>>> passed in GFN equaling INVALID_GFN is non-trivial. Make this quite a
>>> bit more obvious by checking input in generic code - both for singular
>>> requests to not match the value and for range ones to not pass / wrap
>>> through it.
>>>
>>> For Arm similarly make more obvious that no wrapping of MFNs passed
>>> for XENMAPSPACE_dev_mmio and thus to map_dev_mmio_region() can occur:
>>> Drop the "nr" parameter of the function to avoid future callers
>>> appearing which might not themselves check for wrapping. Otherwise
>>> the respective ASSERT() in rangeset_contains_range() could trigger.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> ---
>>> I find it odd that map_dev_mmio_region() returns success upon
>>> iomem_access_permitted() indicating failure - is this really intended?
>>
>> AFAIR yes. The hypercall is not used as "Map the region" but instead
>> "Make sure the region is mapped if the IOMEM region is accessible".
>>
>> It is necessary to return 0 because dom0 OS cannot distinguished between
>> emulated and non-emulated. So we may report error when there is none.
> 
> Odd, but I clearly don't understand all the aspects here.
> 
>>> As per commit 102984bb1987 introducing it this also was added for ACPI
>>> only - any reason XENMAPSPACE_dev_mmio isn't restricted to CONFIG_ACPI
>>> builds?
>>
>> There is nothing specific to ACPI in the implementation. So I don't
>> really see the reason to restrict to CONFIG_ACPI.
>>
>> However, it is still possible to boot using DT when Xen is built with
>> CONFIG_ACPI. So if the restriction was desirable, then I think it should
>> be using !acpi_disabled.
> 
> My point was rather about this potentially being dead code in non-ACPI
> builds (i.e. in particular uniformly on 32-bit).

The hypercall is already wired and a dom0 OS can use it today even on 
non-ACPI. Whether a dom0 OS will use it is a different question. I know 
that Linux will limit it to ACPI. It is likely not used by other OS, but 
I can't guarantee it.

In this case, the hypercall is only a few lines and already restricted 
to dom0 only (see xapt_permission_check()). So to me, the #ifdef here is 
not worth it.

>>> @@ -841,6 +844,15 @@ int xenmem_add_to_physmap(struct domain
>>>        if ( xatp->size < start )
>>>            return -EILSEQ;
>>>    
>>> +    if ( xatp->gpfn + xatp->size < xatp->gpfn ||
>>> +         xatp->idx + xatp->size < xatp->idx )
>>> +    {
>>> +#define _gfn(x) (x)
>>
>> AFAICT, _gfn() will already be defined. So some compiler may complain
>> because will be defined differently on debug build.
> 
> No - _gfn() is an inline function as per typesafe.h. (Or else it
> wouldn't be just "some" compiler, but gcc at least would have
> complained to me.)

Ah. somehow I thought it was a macro. But looking at the implementation, 
it makes sense to be an inline funciton.

Sorry for the noise.

> 
>> However...
>>
>>> +        BUILD_BUG_ON(INVALID_GFN + 1);
>>
>> ... I might be missing something... but why can't use gfn_x(INVALID_GFN)
>> + 1 here?
> 
> Because gfn_x() also is an inline function, and that's not suitable
> for a compile-time constant expression.

Right. How about introduce INVALID_GFN_RAW in mm-frame.h? This could 
also be used to replace the open-code value in INVALID_GFN and 
INVALID_GFN_INITIALIZER?

> 
>> In fact, I am not entirely sure what's the purpose of this
>> BUILD_BUG_ON(). Could you give more details?
> 
> The expression in the surrounding if() relies on INVALID_GFN being the
> largest representable value, i.e. this ensures that INVALID_GFN doesn't
> sit anywhere in [xatp->gpfn, xatp->gpfn + xatp->size).

Thanks the explanation. Can you add the rationale in a comment on top of 
BUILD_BUG_ON()?

Cheers,

-- 
Julien Grall

Re: [PATCH 2/2] memory: XENMEM_add_to_physmap (almost) wrapping checks

[Jan Beulich]

On 15.10.2021 11:26, Julien Grall wrote:
> On 14/10/2021 15:10, Jan Beulich wrote:
>> On 14.10.2021 13:29, Julien Grall wrote:
>>> On 13/09/2021 07:42, Jan Beulich wrote:
>>>> Determining that behavior is correct (i.e. results in failure) for a
>>>> passed in GFN equaling INVALID_GFN is non-trivial. Make this quite a
>>>> bit more obvious by checking input in generic code - both for singular
>>>> requests to not match the value and for range ones to not pass / wrap
>>>> through it.
>>>>
>>>> For Arm similarly make more obvious that no wrapping of MFNs passed
>>>> for XENMAPSPACE_dev_mmio and thus to map_dev_mmio_region() can occur:
>>>> Drop the "nr" parameter of the function to avoid future callers
>>>> appearing which might not themselves check for wrapping. Otherwise
>>>> the respective ASSERT() in rangeset_contains_range() could trigger.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> ---
>>>> I find it odd that map_dev_mmio_region() returns success upon
>>>> iomem_access_permitted() indicating failure - is this really intended?
>>>
>>> AFAIR yes. The hypercall is not used as "Map the region" but instead
>>> "Make sure the region is mapped if the IOMEM region is accessible".
>>>
>>> It is necessary to return 0 because dom0 OS cannot distinguished between
>>> emulated and non-emulated. So we may report error when there is none.
>>
>> Odd, but I clearly don't understand all the aspects here.
>>
>>>> As per commit 102984bb1987 introducing it this also was added for ACPI
>>>> only - any reason XENMAPSPACE_dev_mmio isn't restricted to CONFIG_ACPI
>>>> builds?
>>>
>>> There is nothing specific to ACPI in the implementation. So I don't
>>> really see the reason to restrict to CONFIG_ACPI.
>>>
>>> However, it is still possible to boot using DT when Xen is built with
>>> CONFIG_ACPI. So if the restriction was desirable, then I think it should
>>> be using !acpi_disabled.
>>
>> My point was rather about this potentially being dead code in non-ACPI
>> builds (i.e. in particular uniformly on 32-bit).
> 
> The hypercall is already wired and a dom0 OS can use it today even on 
> non-ACPI. Whether a dom0 OS will use it is a different question. I know 
> that Linux will limit it to ACPI. It is likely not used by other OS, but 
> I can't guarantee it.
> 
> In this case, the hypercall is only a few lines and already restricted 
> to dom0 only (see xapt_permission_check()). So to me, the #ifdef here is 
> not worth it.

Well, okay then - I've removed that remark.

>>>> @@ -841,6 +844,15 @@ int xenmem_add_to_physmap(struct domain
>>>>        if ( xatp->size < start )
>>>>            return -EILSEQ;
>>>>    
>>>> +    if ( xatp->gpfn + xatp->size < xatp->gpfn ||
>>>> +         xatp->idx + xatp->size < xatp->idx )
>>>> +    {
>>>> +#define _gfn(x) (x)
>>>
>>> AFAICT, _gfn() will already be defined. So some compiler may complain
>>> because will be defined differently on debug build.
>>
>> No - _gfn() is an inline function as per typesafe.h. (Or else it
>> wouldn't be just "some" compiler, but gcc at least would have
>> complained to me.)
> 
> Ah. somehow I thought it was a macro. But looking at the implementation, 
> it makes sense to be an inline funciton.
> 
> Sorry for the noise.
> 
>>
>>> However...
>>>
>>>> +        BUILD_BUG_ON(INVALID_GFN + 1);
>>>
>>> ... I might be missing something... but why can't use gfn_x(INVALID_GFN)
>>> + 1 here?
>>
>> Because gfn_x() also is an inline function, and that's not suitable
>> for a compile-time constant expression.
> 
> Right. How about introduce INVALID_GFN_RAW in mm-frame.h? This could 
> also be used to replace the open-code value in INVALID_GFN and 
> INVALID_GFN_INITIALIZER?

Can do, but that'll be a prereq patch then also taking care of INVALID_MFN.

>>> In fact, I am not entirely sure what's the purpose of this
>>> BUILD_BUG_ON(). Could you give more details?
>>
>> The expression in the surrounding if() relies on INVALID_GFN being the
>> largest representable value, i.e. this ensures that INVALID_GFN doesn't
>> sit anywhere in [xatp->gpfn, xatp->gpfn + xatp->size).
> 
> Thanks the explanation. Can you add the rationale in a comment on top of 
> BUILD_BUG_ON()?

Sure, done.

Jan

Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame()

[Julien Grall]

Hi Jan,

Sorry for the delay. I was waiting for XSA-387 to go out before answering.

On 13/09/2021 07:41, Jan Beulich wrote:
> Without holding appropriate locks, attempting to remove a prior mapping
> of the underlying page is pointless, as the same (or another) mapping
> could be re-established by a parallel request on another vCPU. Move the
> code to Arm's gnttab_set_frame_gfn(). Of course this new placement
> doesn't improve things in any way as far as the security of grant status
> frame mappings goes (see XSA-379). Proper locking would be needed here
> to allow status frames to be mapped securely.
> 
> In turn this then requires replacing the other use in
> gnttab_unpopulate_status_frames(), which yet in turn requires adjusting
> x86's gnttab_set_frame_gfn(). Note that with proper locking inside
> guest_physmap_remove_page() combined with checking the GFN's mapping
> there against the passed in MFN, there then is no issue with the
> involved multiple gnttab_set_frame_gfn()-s potentially returning varying

Do you mean gnttab_get_frame_gfn()?

> values (due to a racing XENMAPSPACE_grant_table request).
> 
> This, as a side effect, does away with gnttab_map_frame() having a local
> variable "gfn" which shadows a function parameter of the same name.
> 
> Together with XSA-379 this points out that XSA-255's addition to
> gnttab_map_frame() was really useless.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -92,7 +92,7 @@ struct grant_table {
>       struct radix_tree_root maptrack_tree;
>   
>       /* Domain to which this struct grant_table belongs. */
> -    const struct domain *domain;
> +    struct domain *domain;
>   
>       struct grant_table_arch arch;
>   };
> @@ -1795,8 +1795,8 @@ gnttab_unpopulate_status_frames(struct d
>           {
>               int rc = gfn_eq(gfn, INVALID_GFN)
>                        ? 0
> -                     : guest_physmap_remove_page(d, gfn,
> -                                                 page_to_mfn(pg), 0);
> +                     : gnttab_set_frame_gfn(gt, true, i, INVALID_GFN,
> +                                            page_to_mfn(pg));
>   
>               if ( rc )
>               {
> @@ -1806,7 +1806,6 @@ gnttab_unpopulate_status_frames(struct d
>                   domain_crash(d);
>                   return rc;
>               }
> -            gnttab_set_frame_gfn(gt, true, i, INVALID_GFN);
>           }
>   
>           BUG_ON(page_get_owner(pg) != d);
> @@ -4132,6 +4131,9 @@ int gnttab_map_frame(struct domain *d, u
>       struct grant_table *gt = d->grant_table;
>       bool status = false;
>   
> +    if ( gfn_eq(gfn, INVALID_GFN) )
> +        return -EINVAL;
> +
>       grant_write_lock(gt);
>   
>       if ( evaluate_nospec(gt->gt_version == 2) && (idx & XENMAPIDX_grant_table_status) )
> @@ -4144,24 +4146,18 @@ int gnttab_map_frame(struct domain *d, u
>       else
>           rc = gnttab_get_shared_frame_mfn(d, idx, mfn);
>   
> -    if ( !rc && paging_mode_translate(d) )
> -    {
> -        gfn_t gfn = gnttab_get_frame_gfn(gt, status, idx);
> -
> -        if ( !gfn_eq(gfn, INVALID_GFN) )
> -            rc = guest_physmap_remove_page(d, gfn, *mfn, 0);
> -    }
> -
>       if ( !rc )
>       {
> +        struct page_info *pg = mfn_to_page(*mfn);
> +
>           /*
>            * Make sure gnttab_unpopulate_status_frames() won't (successfully)
>            * free the page until our caller has completed its operation.
>            */
> -        if ( get_page(mfn_to_page(*mfn), d) )
> -            gnttab_set_frame_gfn(gt, status, idx, gfn);
> -        else
> +        if ( !get_page(pg, d) )
>               rc = -EBUSY;
> +        else if ( (rc = gnttab_set_frame_gfn(gt, status, idx, gfn, *mfn)) )
> +            put_page(pg);
>       }
>   
>       grant_write_unlock(gt);
> --- a/xen/include/asm-arm/grant_table.h
> +++ b/xen/include/asm-arm/grant_table.h
> @@ -71,11 +71,17 @@ int replace_grant_host_mapping(unsigned
>           XFREE((gt)->arch.status_gfn);                                    \
>       } while ( 0 )
>   
> -#define gnttab_set_frame_gfn(gt, st, idx, gfn)                           \
> -    do {                                                                 \
> -        ((st) ? (gt)->arch.status_gfn : (gt)->arch.shared_gfn)[idx] =    \
> -            (gfn);                                                       \
> -    } while ( 0 )
> +#define gnttab_set_frame_gfn(gt, st, idx, gfn, mfn)                      \
> +    ({                                                                   \
> +        int rc_ = 0;                                                     \
> +        gfn_t ogfn = gnttab_get_frame_gfn(gt, st, idx);                  \
> +        if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) ||           \
> +             (rc_ = guest_physmap_remove_page((gt)->domain, ogfn, mfn,   \
> +                                              0)) == 0 )                 \
> +            ((st) ? (gt)->arch.status_gfn                                \
> +                  : (gt)->arch.shared_gfn)[idx] = (gfn);                 \
> +        rc_;                                                             \
> +    })

I think using a function would make it a bit easier to understand what 
it does.

However... The naming of the function is now quite confusing. The more 
on x86...

>   
>   #define gnttab_get_frame_gfn(gt, st, idx) ({                             \
>      (st) ? gnttab_status_gfn(NULL, gt, idx)                               \
> --- a/xen/include/asm-x86/grant_table.h
> +++ b/xen/include/asm-x86/grant_table.h
> @@ -37,7 +37,12 @@ static inline int replace_grant_host_map
>   
>   #define gnttab_init_arch(gt) 0
>   #define gnttab_destroy_arch(gt) do {} while ( 0 )
> -#define gnttab_set_frame_gfn(gt, st, idx, gfn) do {} while ( 0 )
> +#define gnttab_set_frame_gfn(gt, st, idx, gfn, mfn)                      \
> +    (gfn_eq(gfn, INVALID_GFN)                                            \
> +     ? guest_physmap_remove_page((gt)->domain,                           \
> +                                 gnttab_get_frame_gfn(gt, st, idx),      \
> +                                 mfn, 0)                                 \
> +     : 0 /* Handled in add_to_physmap_one(). */)

... it will end up to remove the mapping. I don't have a better name at 
the moment. However I think this would deserve some documentation in the 
code so one can understand how to implement it for another arch.

>   #define gnttab_get_frame_gfn(gt, st, idx) ({                             \
>       mfn_t mfn_ = (st) ? gnttab_status_mfn(gt, idx)                       \
>                         : gnttab_shared_mfn(gt, idx);                      \
> 

Cheers,

-- 
Julien Grall

Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame()

[Jan Beulich]

On 25.11.2021 17:37, Julien Grall wrote:
> On 13/09/2021 07:41, Jan Beulich wrote:
>> Without holding appropriate locks, attempting to remove a prior mapping
>> of the underlying page is pointless, as the same (or another) mapping
>> could be re-established by a parallel request on another vCPU. Move the
>> code to Arm's gnttab_set_frame_gfn(). Of course this new placement
>> doesn't improve things in any way as far as the security of grant status
>> frame mappings goes (see XSA-379). Proper locking would be needed here
>> to allow status frames to be mapped securely.
>>
>> In turn this then requires replacing the other use in
>> gnttab_unpopulate_status_frames(), which yet in turn requires adjusting
>> x86's gnttab_set_frame_gfn(). Note that with proper locking inside
>> guest_physmap_remove_page() combined with checking the GFN's mapping
>> there against the passed in MFN, there then is no issue with the
>> involved multiple gnttab_set_frame_gfn()-s potentially returning varying
> 
> Do you mean gnttab_get_frame_gfn()?

No, I don't think so; I do mean gnttab_set_frame_gfn(). Its return value
directs the caller to do (or not) certain things.

>> --- a/xen/include/asm-arm/grant_table.h
>> +++ b/xen/include/asm-arm/grant_table.h
>> @@ -71,11 +71,17 @@ int replace_grant_host_mapping(unsigned
>>           XFREE((gt)->arch.status_gfn);                                    \
>>       } while ( 0 )
>>   
>> -#define gnttab_set_frame_gfn(gt, st, idx, gfn)                           \
>> -    do {                                                                 \
>> -        ((st) ? (gt)->arch.status_gfn : (gt)->arch.shared_gfn)[idx] =    \
>> -            (gfn);                                                       \
>> -    } while ( 0 )
>> +#define gnttab_set_frame_gfn(gt, st, idx, gfn, mfn)                      \
>> +    ({                                                                   \
>> +        int rc_ = 0;                                                     \
>> +        gfn_t ogfn = gnttab_get_frame_gfn(gt, st, idx);                  \
>> +        if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) ||           \
>> +             (rc_ = guest_physmap_remove_page((gt)->domain, ogfn, mfn,   \
>> +                                              0)) == 0 )                 \
>> +            ((st) ? (gt)->arch.status_gfn                                \
>> +                  : (gt)->arch.shared_gfn)[idx] = (gfn);                 \
>> +        rc_;                                                             \
>> +    })
> 
> I think using a function would make it a bit easier to understand what 
> it does.

I can convert it, but it's not very likely that it would be possible
to make it an inline one. Not sure whether that's then still desirable.

> However... The naming of the function is now quite confusing. The more 
> on x86...
> 
>>   
>>   #define gnttab_get_frame_gfn(gt, st, idx) ({                             \
>>      (st) ? gnttab_status_gfn(NULL, gt, idx)                               \
>> --- a/xen/include/asm-x86/grant_table.h
>> +++ b/xen/include/asm-x86/grant_table.h
>> @@ -37,7 +37,12 @@ static inline int replace_grant_host_map
>>   
>>   #define gnttab_init_arch(gt) 0
>>   #define gnttab_destroy_arch(gt) do {} while ( 0 )
>> -#define gnttab_set_frame_gfn(gt, st, idx, gfn) do {} while ( 0 )
>> +#define gnttab_set_frame_gfn(gt, st, idx, gfn, mfn)                      \
>> +    (gfn_eq(gfn, INVALID_GFN)                                            \
>> +     ? guest_physmap_remove_page((gt)->domain,                           \
>> +                                 gnttab_get_frame_gfn(gt, st, idx),      \
>> +                                 mfn, 0)                                 \
>> +     : 0 /* Handled in add_to_physmap_one(). */)
> 
> ... it will end up to remove the mapping. I don't have a better name at 
> the moment. However I think this would deserve some documentation in the 
> code so one can understand how to implement it for another arch.

Hmm, perhaps. But wouldn't we better document the final behavior (i.e.
once the remaining Arm issue got addressed)? That may then also lead
to overall simpler code, ideally with more suitable names (if the
present ones are deemed unsuitable, which I'm not convinced of).

Jan

Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame()

[Julien Grall]

Hi Jan,

On 26/11/2021 08:07, Jan Beulich wrote:
> On 25.11.2021 17:37, Julien Grall wrote:
>> On 13/09/2021 07:41, Jan Beulich wrote:
>>> Without holding appropriate locks, attempting to remove a prior mapping
>>> of the underlying page is pointless, as the same (or another) mapping
>>> could be re-established by a parallel request on another vCPU. Move the
>>> code to Arm's gnttab_set_frame_gfn(). Of course this new placement
>>> doesn't improve things in any way as far as the security of grant status
>>> frame mappings goes (see XSA-379). Proper locking would be needed here
>>> to allow status frames to be mapped securely.
>>>
>>> In turn this then requires replacing the other use in
>>> gnttab_unpopulate_status_frames(), which yet in turn requires adjusting
>>> x86's gnttab_set_frame_gfn(). Note that with proper locking inside
>>> guest_physmap_remove_page() combined with checking the GFN's mapping
>>> there against the passed in MFN, there then is no issue with the
>>> involved multiple gnttab_set_frame_gfn()-s potentially returning varying
>>
>> Do you mean gnttab_get_frame_gfn()?
> 
> No, I don't think so; I do mean gnttab_set_frame_gfn(). Its return value
> directs the caller to do (or not) certain things.

Hmmm ok. I misundertood that comment then. Thanks for the clarification!

> 
>>> --- a/xen/include/asm-arm/grant_table.h
>>> +++ b/xen/include/asm-arm/grant_table.h
>>> @@ -71,11 +71,17 @@ int replace_grant_host_mapping(unsigned
>>>            XFREE((gt)->arch.status_gfn);                                    \
>>>        } while ( 0 )
>>>    
>>> -#define gnttab_set_frame_gfn(gt, st, idx, gfn)                           \
>>> -    do {                                                                 \
>>> -        ((st) ? (gt)->arch.status_gfn : (gt)->arch.shared_gfn)[idx] =    \
>>> -            (gfn);                                                       \
>>> -    } while ( 0 )
>>> +#define gnttab_set_frame_gfn(gt, st, idx, gfn, mfn)                      \
>>> +    ({                                                                   \
>>> +        int rc_ = 0;                                                     \
>>> +        gfn_t ogfn = gnttab_get_frame_gfn(gt, st, idx);                  \
>>> +        if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) ||           \
>>> +             (rc_ = guest_physmap_remove_page((gt)->domain, ogfn, mfn,   \
>>> +                                              0)) == 0 )                 \
>>> +            ((st) ? (gt)->arch.status_gfn                                \
>>> +                  : (gt)->arch.shared_gfn)[idx] = (gfn);                 \
>>> +        rc_;                                                             \
>>> +    })
>>
>> I think using a function would make it a bit easier to understand what
>> it does.
> 
> I can convert it, but it's not very likely that it would be possible
> to make it an inline one. Not sure whether that's then still desirable.

So looking at Oleksandr series, I think we should be able to have the 
Arm helper matching the x86 one. At which point, I could deal with this 
version for now.

> 
>> However... The naming of the function is now quite confusing. The more
>> on x86...
>>
>>>    
>>>    #define gnttab_get_frame_gfn(gt, st, idx) ({                             \
>>>       (st) ? gnttab_status_gfn(NULL, gt, idx)                               \
>>> --- a/xen/include/asm-x86/grant_table.h
>>> +++ b/xen/include/asm-x86/grant_table.h
>>> @@ -37,7 +37,12 @@ static inline int replace_grant_host_map
>>>    
>>>    #define gnttab_init_arch(gt) 0
>>>    #define gnttab_destroy_arch(gt) do {} while ( 0 )
>>> -#define gnttab_set_frame_gfn(gt, st, idx, gfn) do {} while ( 0 )
>>> +#define gnttab_set_frame_gfn(gt, st, idx, gfn, mfn)                      \
>>> +    (gfn_eq(gfn, INVALID_GFN)                                            \
>>> +     ? guest_physmap_remove_page((gt)->domain,                           \
>>> +                                 gnttab_get_frame_gfn(gt, st, idx),      \
>>> +                                 mfn, 0)                                 \
>>> +     : 0 /* Handled in add_to_physmap_one(). */)
>>
>> ... it will end up to remove the mapping. I don't have a better name at
>> the moment. However I think this would deserve some documentation in the
>> code so one can understand how to implement it for another arch.
> 
> Hmm, perhaps. But wouldn't we better document the final behavior (i.e.
> once the remaining Arm issue got addressed)?

Fair point. I don't expect Oleksandr's series to be committed long after 
yours.

> That may then also lead
> to overall simpler code, ideally with more suitable names (if the
> present ones are deemed unsuitable, which I'm not convinced of).
I don't have a better name so far. However, without any documentation 
this is very difficult to figure out what it is meant to do (I am not 
only referring to someone implementing it for another arch here).

Cheers,

-- 
Julien Grall

Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame()

[Julien Grall]

Hi Jan,

On 13/09/2021 07:41, Jan Beulich wrote:
> Without holding appropriate locks, attempting to remove a prior mapping
> of the underlying page is pointless, as the same (or another) mapping
> could be re-established by a parallel request on another vCPU. Move the
> code to Arm's gnttab_set_frame_gfn(). Of course this new placement
> doesn't improve things in any way as far as the security of grant status
> frame mappings goes (see XSA-379). Proper locking would be needed here
> to allow status frames to be mapped securely.
> 
> In turn this then requires replacing the other use in
> gnttab_unpopulate_status_frames(), which yet in turn requires adjusting
> x86's gnttab_set_frame_gfn(). Note that with proper locking inside
> guest_physmap_remove_page() combined with checking the GFN's mapping
> there against the passed in MFN, there then is no issue with the
> involved multiple gnttab_set_frame_gfn()-s potentially returning varying
> values (due to a racing XENMAPSPACE_grant_table request).
> 
> This, as a side effect, does away with gnttab_map_frame() having a local
> variable "gfn" which shadows a function parameter of the same name.
> 
> Together with XSA-379 this points out that XSA-255's addition to
> gnttab_map_frame() was really useless.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

As discussed in the thread, I expect the Arm part to be simplified after 
Oleksandr's series. So for the Arm part:

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

Cheers,

-- 
Julien Grall

Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame()

[Jan Beulich]

On 02.12.2021 10:09, Julien Grall wrote:
> Hi Jan,
> 
> On 13/09/2021 07:41, Jan Beulich wrote:
>> Without holding appropriate locks, attempting to remove a prior mapping
>> of the underlying page is pointless, as the same (or another) mapping
>> could be re-established by a parallel request on another vCPU. Move the
>> code to Arm's gnttab_set_frame_gfn(). Of course this new placement
>> doesn't improve things in any way as far as the security of grant status
>> frame mappings goes (see XSA-379). Proper locking would be needed here
>> to allow status frames to be mapped securely.
>>
>> In turn this then requires replacing the other use in
>> gnttab_unpopulate_status_frames(), which yet in turn requires adjusting
>> x86's gnttab_set_frame_gfn(). Note that with proper locking inside
>> guest_physmap_remove_page() combined with checking the GFN's mapping
>> there against the passed in MFN, there then is no issue with the
>> involved multiple gnttab_set_frame_gfn()-s potentially returning varying
>> values (due to a racing XENMAPSPACE_grant_table request).
>>
>> This, as a side effect, does away with gnttab_map_frame() having a local
>> variable "gfn" which shadows a function parameter of the same name.
>>
>> Together with XSA-379 this points out that XSA-255's addition to
>> gnttab_map_frame() was really useless.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> As discussed in the thread, I expect the Arm part to be simplified after 
> Oleksandr's series. So for the Arm part:
> 
> Acked-by: Julien Grall <jgrall@amazon.com>

Thanks, but let me ask for a clarification: Explicitly just the Arm part,
or also the common code change? I ask because from the x86 side I already
have an ack by Roger, but strictly speaking that doesn't cover common
code.

Jan

Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame()

[Julien Grall]

Hi Jan,

On 02/12/2021 10:12, Jan Beulich wrote:
> On 02.12.2021 10:09, Julien Grall wrote:
>> Hi Jan,
>>
>> On 13/09/2021 07:41, Jan Beulich wrote:
>>> Without holding appropriate locks, attempting to remove a prior mapping
>>> of the underlying page is pointless, as the same (or another) mapping
>>> could be re-established by a parallel request on another vCPU. Move the
>>> code to Arm's gnttab_set_frame_gfn(). Of course this new placement
>>> doesn't improve things in any way as far as the security of grant status
>>> frame mappings goes (see XSA-379). Proper locking would be needed here
>>> to allow status frames to be mapped securely.
>>>
>>> In turn this then requires replacing the other use in
>>> gnttab_unpopulate_status_frames(), which yet in turn requires adjusting
>>> x86's gnttab_set_frame_gfn(). Note that with proper locking inside
>>> guest_physmap_remove_page() combined with checking the GFN's mapping
>>> there against the passed in MFN, there then is no issue with the
>>> involved multiple gnttab_set_frame_gfn()-s potentially returning varying
>>> values (due to a racing XENMAPSPACE_grant_table request).
>>>
>>> This, as a side effect, does away with gnttab_map_frame() having a local
>>> variable "gfn" which shadows a function parameter of the same name.
>>>
>>> Together with XSA-379 this points out that XSA-255's addition to
>>> gnttab_map_frame() was really useless.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> As discussed in the thread, I expect the Arm part to be simplified after
>> Oleksandr's series. So for the Arm part:
>>
>> Acked-by: Julien Grall <jgrall@amazon.com>
> 
> Thanks, but let me ask for a clarification: Explicitly just the Arm part,
> or also the common code change? I ask because from the x86 side I already
> have an ack by Roger, but strictly speaking that doesn't cover common
> code.

Ah, I thought Roger gave a reviewed-by and therefore from the check-in 
policy be sufficient. The common parts look also fine to me. So feel 
free to extend my acked-by to include the common code.

Cheers,

-- 
Julien Grall

Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame()

[Oleksandr]

On 02.12.21 11:09, Julien Grall wrote:

Hi Julien, Jan

> Hi Jan,
>
> On 13/09/2021 07:41, Jan Beulich wrote:
>> Without holding appropriate locks, attempting to remove a prior mapping
>> of the underlying page is pointless, as the same (or another) mapping
>> could be re-established by a parallel request on another vCPU. Move the
>> code to Arm's gnttab_set_frame_gfn(). Of course this new placement
>> doesn't improve things in any way as far as the security of grant status
>> frame mappings goes (see XSA-379). Proper locking would be needed here
>> to allow status frames to be mapped securely.
>>
>> In turn this then requires replacing the other use in
>> gnttab_unpopulate_status_frames(), which yet in turn requires adjusting
>> x86's gnttab_set_frame_gfn(). Note that with proper locking inside
>> guest_physmap_remove_page() combined with checking the GFN's mapping
>> there against the passed in MFN, there then is no issue with the
>> involved multiple gnttab_set_frame_gfn()-s potentially returning varying
>> values (due to a racing XENMAPSPACE_grant_table request).
>>
>> This, as a side effect, does away with gnttab_map_frame() having a local
>> variable "gfn" which shadows a function parameter of the same name.
>>
>> Together with XSA-379 this points out that XSA-255's addition to
>> gnttab_map_frame() was really useless.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> As discussed in the thread, I expect the Arm part to be simplified 
> after Oleksandr's series.


I assume, current patch is going to be committed soon(?), so I am in the 
process of preparing a rebased version of my patch as both touch Arm's 
gnttab_set_frame_gfn at least (I did a rebase sometime, but I lost these 
changes somehow).

Anyway, according to the recent suggestion how to eliminate the possible 
lock inversion introduced by my patch and taking into the account 
changes in current patch, the Arm's gnttab_set_frame_gfn, I think, needs 
to be updated to the following form:



#define gnttab_set_frame_gfn(gt, st, idx, gfn, mfn)                      \
({ \
         gfn_t ogfn = gnttab_get_frame_gfn(gt, st, idx);                  \
         (!gfn_eq(ogfn, INVALID_GFN) && !gfn_eq(ogfn, gfn))               \
          ? guest_physmap_remove_page((gt)->domain, ogfn, mfn, 0)         \
          : 0;                                                            \
     })


> So for the Arm part:
>
> Acked-by: Julien Grall <jgrall@amazon.com>
>
> Cheers,
>
-- 
Regards,

Oleksandr Tyshchenko