Xen Security Advisory 388 v3 (CVE-2021-28704,CVE-2021-28707,CVE-2021-28708) - PoD operations on misaligned GFNs

From: Jan Beulich <[email protected]>
Subject: x86/PoD: deal with misaligned GFNs

Users of XENMEM_decrease_reservation and XENMEM_populate_physmap aren't
required to pass in order-aligned GFN values. (While I consider this
bogus, I don't think we can fix this there, as that might break existing
code, e.g Linux'es swiotlb, which - while affecting PV only - until
recently had been enforcing only page alignment on the original
allocation.) Only non-PoD code paths (guest_physmap_{add,remove}_page(),
p2m_set_entry()) look to be dealing with this properly (in part by being
implemented inefficiently, handling every 4k page separately).

Introduce wrappers taking care of splitting the incoming request into
aligned chunks, without putting much effort in trying to determine the
largest possible chunk at every iteration.

Also "handle" p2m_set_entry() failure for non-order-0 requests by
crashing the domain in one more place. Alongside putting a log message
there, also add one to the other similar path.

Note regarding locking: This is left in the actual worker functions on
the assumption that callers aren't guaranteed atomicity wrt acting on
multiple pages at a time. For mis-aligned GFNs gfn_lock() wouldn't have
locked the correct GFN range anyway, if it didn't simply resolve to
p2m_lock(), and for well-behaved callers there continues to be only a
single iteration, i.e. behavior is unchanged for them. (FTAOD pulling
out just pod_lock() into p2m_pod_decrease_reservation() would result in
a lock order violation.)

This is CVE-2021-28704 and CVE-2021-28707 / part of XSA-388.

Fixes: 3c352011c0d3 ("x86/PoD: shorten certain operations on higher order ranges")
Signed-off-by: Jan Beulich <[email protected]>
Reviewed-by: Roger Pau Monné <[email protected]>
---
The security aspect here stems from the various BUG_ON()s we have
checking
- p2m->pod.entry_count,
- the alignment of the MFN p2m_pod_cache_get() returns.
---
v3: Re-do using wrappers.
v2: Prefix domain_crash() by printk(XENLOG_G_ERR ...) (including the
    pre-existing similar one).

--- a/xen/arch/x86/mm/p2m-pod.c
+++ b/xen/arch/x86/mm/p2m-pod.c
@@ -504,7 +504,7 @@ static void pod_unlock_and_flush(struct
 }
 
 /*
- * This function is needed for two reasons:
+ * This pair of functions is needed for two reasons:
  * + To properly handle clearing of PoD entries
  * + To "steal back" memory being freed for the PoD cache, rather than
  *   releasing it.
@@ -512,8 +512,8 @@ static void pod_unlock_and_flush(struct
  * Once both of these functions have been completed, we can return and
  * allow decrease_reservation() to handle everything else.
  */
-unsigned long
-p2m_pod_decrease_reservation(struct domain *d, gfn_t gfn, unsigned int order)
+static unsigned long
+decrease_reservation(struct domain *d, gfn_t gfn, unsigned int order)
 {
     unsigned long ret = 0, i, n;
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
@@ -561,8 +561,10 @@ p2m_pod_decrease_reservation(struct doma
          * All PoD: Mark the whole region invalid and tell caller
          * we're done.
          */
-        if ( p2m_set_entry(p2m, gfn, INVALID_MFN, order, p2m_invalid,
-                           p2m->default_access) )
+        int rc = p2m_set_entry(p2m, gfn, INVALID_MFN, order, p2m_invalid,
+                               p2m->default_access);
+
+        if ( rc )
         {
             /*
              * If this fails, we can't tell how much of the range was changed.
@@ -570,7 +572,12 @@ p2m_pod_decrease_reservation(struct doma
              * impossible.
              */
             if ( order != 0 )
+            {
+                printk(XENLOG_G_ERR
+                       "%pd: marking GFN %#lx (order %u) as non-PoD failed: %d\n",
+                       d, gfn_x(gfn), order, rc);
                 domain_crash(d);
+            }
             goto out_unlock;
         }
         ret = 1UL << order;
@@ -679,6 +686,22 @@ out_unlock:
     return ret;
 }
 
+unsigned long
+p2m_pod_decrease_reservation(struct domain *d, gfn_t gfn, unsigned int order)
+{
+    unsigned long left = 1UL << order, ret = 0;
+    unsigned int chunk_order = find_first_set_bit(gfn_x(gfn) | left);
+
+    do {
+        ret += decrease_reservation(d, gfn, chunk_order);
+
+        left -= 1UL << chunk_order;
+        gfn = gfn_add(gfn, 1UL << chunk_order);
+    } while ( left );
+
+    return ret;
+}
+
 void p2m_pod_dump_data(struct domain *d)
 {
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
@@ -1289,19 +1312,15 @@ remap_and_retry:
     return true;
 }
 
-
-int
-guest_physmap_mark_populate_on_demand(struct domain *d, unsigned long gfn_l,
-                                      unsigned int order)
+static int
+mark_populate_on_demand(struct domain *d, unsigned long gfn_l,
+                        unsigned int order)
 {
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
     gfn_t gfn = _gfn(gfn_l);
     unsigned long i, n, pod_count = 0;
     int rc = 0;
 
-    if ( !paging_mode_translate(d) )
-        return -EINVAL;
-
     gfn_lock(p2m, gfn, order);
 
     P2M_DEBUG("mark pod gfn=%#lx\n", gfn_l);
@@ -1341,12 +1360,44 @@ guest_physmap_mark_populate_on_demand(st
 
         ioreq_request_mapcache_invalidate(d);
     }
+    else if ( order )
+    {
+        /*
+         * If this failed, we can't tell how much of the range was changed.
+         * Best to crash the domain.
+         */
+        printk(XENLOG_G_ERR
+               "%pd: marking GFN %#lx (order %u) as PoD failed: %d\n",
+               d, gfn_l, order, rc);
+        domain_crash(d);
+    }
 
 out:
     gfn_unlock(p2m, gfn, order);
 
     return rc;
 }
+
+int
+guest_physmap_mark_populate_on_demand(struct domain *d, unsigned long gfn,
+                                      unsigned int order)
+{
+    unsigned long left = 1UL << order;
+    unsigned int chunk_order = find_first_set_bit(gfn | left);
+    int rc;
+
+    if ( !paging_mode_translate(d) )
+        return -EINVAL;
+
+    do {
+        rc = mark_populate_on_demand(d, gfn, chunk_order);
+
+        left -= 1UL << chunk_order;
+        gfn += 1UL << chunk_order;
+    } while ( !rc && left );
+
+    return rc;
+}
 
 void p2m_pod_init(struct p2m_domain *p2m)
 {

From: Jan Beulich <[email protected]>
Subject: x86/PoD: handle intermediate page orders in p2m_pod_cache_add()

p2m_pod_decrease_reservation() may pass pages to the function which
aren't 4k, 2M, or 1G. Handle all intermediate orders as well, to avoid
hitting the BUG() at the switch() statement's "default" case.

This is CVE-2021-28708 / part of XSA-388.

Fixes: 3c352011c0d3 ("x86/PoD: shorten certain operations on higher order ranges")
Signed-off-by: Jan Beulich <[email protected]>
Reviewed-by: Roger Pau Monné <[email protected]>

--- a/xen/arch/x86/mm/p2m-pod.c
+++ b/xen/arch/x86/mm/p2m-pod.c
@@ -114,15 +114,13 @@ p2m_pod_cache_add(struct p2m_domain *p2m
     /* Then add to the appropriate populate-on-demand list. */
     switch ( order )
     {
-    case PAGE_ORDER_1G:
-        for ( i = 0; i < (1UL << PAGE_ORDER_1G); i += 1UL << PAGE_ORDER_2M )
+    case PAGE_ORDER_2M ... PAGE_ORDER_1G:
+        for ( i = 0; i < (1UL << order); i += 1UL << PAGE_ORDER_2M )
             page_list_add_tail(page + i, &p2m->pod.super);
         break;
-    case PAGE_ORDER_2M:
-        page_list_add_tail(page, &p2m->pod.super);
-        break;
-    case PAGE_ORDER_4K:
-        page_list_add_tail(page, &p2m->pod.single);
+    case PAGE_ORDER_4K ... PAGE_ORDER_2M - 1:
+        for ( i = 0; i < (1UL << order); i += 1UL << PAGE_ORDER_4K )
+            page_list_add_tail(page + i, &p2m->pod.single);
         break;
     default:
         BUG();

From: Jan Beulich <[email protected]>
Subject: x86/PoD: deal with misaligned GFNs

Users of XENMEM_decrease_reservation and XENMEM_populate_physmap aren't
required to pass in order-aligned GFN values. (While I consider this
bogus, I don't think we can fix this there, as that might break existing
code, e.g Linux'es swiotlb, which - while affecting PV only - until
recently had been enforcing only page alignment on the original
allocation.) Only non-PoD code paths (guest_physmap_{add,remove}_page(),
p2m_set_entry()) look to be dealing with this properly (in part by being
implemented inefficiently, handling every 4k page separately).

Introduce wrappers taking care of splitting the incoming request into
aligned chunks, without putting much effort in trying to determine the
largest possible chunk at every iteration.

Also "handle" p2m_set_entry() failure for non-order-0 requests by
crashing the domain in one more place. Alongside putting a log message
there, also add one to the other similar path.

Note regarding locking: This is left in the actual worker functions on
the assumption that callers aren't guaranteed atomicity wrt acting on
multiple pages at a time. For mis-aligned GFNs gfn_lock() wouldn't have
locked the correct GFN range anyway, if it didn't simply resolve to
p2m_lock(), and for well-behaved callers there continues to be only a
single iteration, i.e. behavior is unchanged for them. (FTAOD pulling
out just pod_lock() into p2m_pod_decrease_reservation() would result in
a lock order violation.)

This is CVE-2021-28704 and CVE-2021-28707 / part of XSA-388.

Fixes: 3c352011c0d3 ("x86/PoD: shorten certain operations on higher order ranges")
Signed-off-by: Jan Beulich <[email protected]>
Reviewed-by: Roger Pau Monné <[email protected]>

--- a/xen/arch/x86/mm/p2m-pod.c
+++ b/xen/arch/x86/mm/p2m-pod.c
@@ -495,7 +495,7 @@ p2m_pod_zero_check_superpage(struct p2m_
 
 
 /*
- * This function is needed for two reasons:
+ * This pair of functions is needed for two reasons:
  * + To properly handle clearing of PoD entries
  * + To "steal back" memory being freed for the PoD cache, rather than
  *   releasing it.
@@ -503,8 +503,8 @@ p2m_pod_zero_check_superpage(struct p2m_
  * Once both of these functions have been completed, we can return and
  * allow decrease_reservation() to handle everything else.
  */
-unsigned long
-p2m_pod_decrease_reservation(struct domain *d, gfn_t gfn, unsigned int order)
+static unsigned long
+decrease_reservation(struct domain *d, gfn_t gfn, unsigned int order)
 {
     unsigned long ret = 0, i, n;
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
@@ -551,8 +551,10 @@ p2m_pod_decrease_reservation(struct doma
          * All PoD: Mark the whole region invalid and tell caller
          * we're done.
          */
-        if ( p2m_set_entry(p2m, gfn, INVALID_MFN, order, p2m_invalid,
-                           p2m->default_access) )
+        int rc = p2m_set_entry(p2m, gfn, INVALID_MFN, order, p2m_invalid,
+                               p2m->default_access);
+
+        if ( rc )
         {
             /*
              * If this fails, we can't tell how much of the range was changed.
@@ -560,7 +562,12 @@ p2m_pod_decrease_reservation(struct doma
              * impossible.
              */
             if ( order != 0 )
+            {
+                printk(XENLOG_G_ERR
+                       "%pd: marking GFN %#lx (order %u) as non-PoD failed: %d\n",
+                       d, gfn_x(gfn), order, rc);
                 domain_crash(d);
+            }
             goto out_unlock;
         }
         ret = 1UL << order;
@@ -667,6 +674,22 @@ out_unlock:
     return ret;
 }
 
+unsigned long
+p2m_pod_decrease_reservation(struct domain *d, gfn_t gfn, unsigned int order)
+{
+    unsigned long left = 1UL << order, ret = 0;
+    unsigned int chunk_order = find_first_set_bit(gfn_x(gfn) | left);
+
+    do {
+        ret += decrease_reservation(d, gfn, chunk_order);
+
+        left -= 1UL << chunk_order;
+        gfn = gfn_add(gfn, 1UL << chunk_order);
+    } while ( left );
+
+    return ret;
+}
+
 void p2m_pod_dump_data(struct domain *d)
 {
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
@@ -1266,19 +1289,15 @@ remap_and_retry:
     return true;
 }
 
-
-int
-guest_physmap_mark_populate_on_demand(struct domain *d, unsigned long gfn_l,
-                                      unsigned int order)
+static int
+mark_populate_on_demand(struct domain *d, unsigned long gfn_l,
+                        unsigned int order)
 {
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
     gfn_t gfn = _gfn(gfn_l);
     unsigned long i, n, pod_count = 0;
     int rc = 0;
 
-    if ( !paging_mode_translate(d) )
-        return -EINVAL;
-
     gfn_lock(p2m, gfn, order);
 
     P2M_DEBUG("mark pod gfn=%#lx\n", gfn_l);
@@ -1316,12 +1335,44 @@ guest_physmap_mark_populate_on_demand(st
         BUG_ON(p2m->pod.entry_count < 0);
         pod_unlock(p2m);
     }
+    else if ( order )
+    {
+        /*
+         * If this failed, we can't tell how much of the range was changed.
+         * Best to crash the domain.
+         */
+        printk(XENLOG_G_ERR
+               "%pd: marking GFN %#lx (order %u) as PoD failed: %d\n",
+               d, gfn_l, order, rc);
+        domain_crash(d);
+    }
 
 out:
     gfn_unlock(p2m, gfn, order);
 
     return rc;
 }
+
+int
+guest_physmap_mark_populate_on_demand(struct domain *d, unsigned long gfn,
+                                      unsigned int order)
+{
+    unsigned long left = 1UL << order;
+    unsigned int chunk_order = find_first_set_bit(gfn | left);
+    int rc;
+
+    if ( !paging_mode_translate(d) )
+        return -EINVAL;
+
+    do {
+        rc = mark_populate_on_demand(d, gfn, chunk_order);
+
+        left -= 1UL << chunk_order;
+        gfn += 1UL << chunk_order;
+    } while ( !rc && left );
+
+    return rc;
+}
 
 void p2m_pod_init(struct p2m_domain *p2m)
 {

From: Jan Beulich <[email protected]>
Subject: x86/PoD: handle intermediate page orders in p2m_pod_cache_add()

p2m_pod_decrease_reservation() may pass pages to the function which
aren't 4k, 2M, or 1G. Handle all intermediate orders as well, to avoid
hitting the BUG() at the switch() statement's "default" case.

This is CVE-2021-28708 / part of XSA-388.

Fixes: 3c352011c0d3 ("x86/PoD: shorten certain operations on higher order ranges")
Signed-off-by: Jan Beulich <[email protected]>
Reviewed-by: Roger Pau Monné <[email protected]>

--- a/xen/arch/x86/mm/p2m-pod.c
+++ b/xen/arch/x86/mm/p2m-pod.c
@@ -111,15 +111,13 @@ p2m_pod_cache_add(struct p2m_domain *p2m
     /* Then add to the appropriate populate-on-demand list. */
     switch ( order )
     {
-    case PAGE_ORDER_1G:
-        for ( i = 0; i < (1UL << PAGE_ORDER_1G); i += 1UL << PAGE_ORDER_2M )
+    case PAGE_ORDER_2M ... PAGE_ORDER_1G:
+        for ( i = 0; i < (1UL << order); i += 1UL << PAGE_ORDER_2M )
             page_list_add_tail(page + i, &p2m->pod.super);
         break;
-    case PAGE_ORDER_2M:
-        page_list_add_tail(page, &p2m->pod.super);
-        break;
-    case PAGE_ORDER_4K:
-        page_list_add_tail(page, &p2m->pod.single);
+    case PAGE_ORDER_4K ... PAGE_ORDER_2M - 1:
+        for ( i = 0; i < (1UL << order); i += 1UL << PAGE_ORDER_4K )
+            page_list_add_tail(page + i, &p2m->pod.single);
         break;
     default:
         BUG();

From: Jan Beulich <[email protected]>
Subject: x86/PoD: deal with misaligned GFNs

Users of XENMEM_decrease_reservation and XENMEM_populate_physmap aren't
required to pass in order-aligned GFN values. (While I consider this
bogus, I don't think we can fix this there, as that might break existing
code, e.g Linux'es swiotlb, which - while affecting PV only - until
recently had been enforcing only page alignment on the original
allocation.) Only non-PoD code paths (guest_physmap_{add,remove}_page(),
p2m_set_entry()) look to be dealing with this properly (in part by being
implemented inefficiently, handling every 4k page separately).

Introduce wrappers taking care of splitting the incoming request into
aligned chunks, without putting much effort in trying to determine the
largest possible chunk at every iteration.

Also "handle" p2m_set_entry() failure for non-order-0 requests by
crashing the domain in one more place. Alongside putting a log message
there, also add one to the other similar path.

Note regarding locking: This is left in the actual worker functions on
the assumption that callers aren't guaranteed atomicity wrt acting on
multiple pages at a time. For mis-aligned GFNs gfn_lock() wouldn't have
locked the correct GFN range anyway, if it didn't simply resolve to
p2m_lock(), and for well-behaved callers there continues to be only a
single iteration, i.e. behavior is unchanged for them. (FTAOD pulling
out just pod_lock() into p2m_pod_decrease_reservation() would result in
a lock order violation.)

This is CVE-2021-28704 and CVE-2021-28707 / part of XSA-388.

Fixes: 3c352011c0d3 ("x86/PoD: shorten certain operations on higher order ranges")
Signed-off-by: Jan Beulich <[email protected]>
Reviewed-by: Roger Pau Monné <[email protected]>

--- a/xen/arch/x86/mm/p2m-pod.c
+++ b/xen/arch/x86/mm/p2m-pod.c
@@ -496,7 +496,7 @@ p2m_pod_zero_check_superpage(struct p2m_
 
 
 /*
- * This function is needed for two reasons:
+ * This pair of functions is needed for two reasons:
  * + To properly handle clearing of PoD entries
  * + To "steal back" memory being freed for the PoD cache, rather than
  *   releasing it.
@@ -504,8 +504,8 @@ p2m_pod_zero_check_superpage(struct p2m_
  * Once both of these functions have been completed, we can return and
  * allow decrease_reservation() to handle everything else.
  */
-unsigned long
-p2m_pod_decrease_reservation(struct domain *d, gfn_t gfn, unsigned int order)
+static unsigned long
+decrease_reservation(struct domain *d, gfn_t gfn, unsigned int order)
 {
     unsigned long ret = 0, i, n;
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
@@ -552,8 +552,10 @@ p2m_pod_decrease_reservation(struct doma
          * All PoD: Mark the whole region invalid and tell caller
          * we're done.
          */
-        if ( p2m_set_entry(p2m, gfn, INVALID_MFN, order, p2m_invalid,
-                           p2m->default_access) )
+        int rc = p2m_set_entry(p2m, gfn, INVALID_MFN, order, p2m_invalid,
+                               p2m->default_access);
+
+        if ( rc )
         {
             /*
              * If this fails, we can't tell how much of the range was changed.
@@ -561,7 +563,12 @@ p2m_pod_decrease_reservation(struct doma
              * impossible.
              */
             if ( order != 0 )
+            {
+                printk(XENLOG_G_ERR
+                       "%pd: marking GFN %#lx (order %u) as non-PoD failed: %d\n",
+                       d, gfn_x(gfn), order, rc);
                 domain_crash(d);
+            }
             goto out_unlock;
         }
         ret = 1UL << order;
@@ -670,6 +677,22 @@ out_unlock:
     return ret;
 }
 
+unsigned long
+p2m_pod_decrease_reservation(struct domain *d, gfn_t gfn, unsigned int order)
+{
+    unsigned long left = 1UL << order, ret = 0;
+    unsigned int chunk_order = find_first_set_bit(gfn_x(gfn) | left);
+
+    do {
+        ret += decrease_reservation(d, gfn, chunk_order);
+
+        left -= 1UL << chunk_order;
+        gfn = gfn_add(gfn, 1UL << chunk_order);
+    } while ( left );
+
+    return ret;
+}
+
 void p2m_pod_dump_data(struct domain *d)
 {
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
@@ -1273,19 +1296,15 @@ remap_and_retry:
     return true;
 }
 
-
-int
-guest_physmap_mark_populate_on_demand(struct domain *d, unsigned long gfn_l,
-                                      unsigned int order)
+static int
+mark_populate_on_demand(struct domain *d, unsigned long gfn_l,
+                        unsigned int order)
 {
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
     gfn_t gfn = _gfn(gfn_l);
     unsigned long i, n, pod_count = 0;
     int rc = 0;
 
-    if ( !paging_mode_translate(d) )
-        return -EINVAL;
-
     gfn_lock(p2m, gfn, order);
 
     P2M_DEBUG("mark pod gfn=%#lx\n", gfn_l);
@@ -1325,12 +1344,44 @@ guest_physmap_mark_populate_on_demand(st
 
         ioreq_request_mapcache_invalidate(d);
     }
+    else if ( order )
+    {
+        /*
+         * If this failed, we can't tell how much of the range was changed.
+         * Best to crash the domain.
+         */
+        printk(XENLOG_G_ERR
+               "%pd: marking GFN %#lx (order %u) as PoD failed: %d\n",
+               d, gfn_l, order, rc);
+        domain_crash(d);
+    }
 
 out:
     gfn_unlock(p2m, gfn, order);
 
     return rc;
 }
+
+int
+guest_physmap_mark_populate_on_demand(struct domain *d, unsigned long gfn,
+                                      unsigned int order)
+{
+    unsigned long left = 1UL << order;
+    unsigned int chunk_order = find_first_set_bit(gfn | left);
+    int rc;
+
+    if ( !paging_mode_translate(d) )
+        return -EINVAL;
+
+    do {
+        rc = mark_populate_on_demand(d, gfn, chunk_order);
+
+        left -= 1UL << chunk_order;
+        gfn += 1UL << chunk_order;
+    } while ( !rc && left );
+
+    return rc;
+}
 
 void p2m_pod_init(struct p2m_domain *p2m)
 {

From: Jan Beulich <[email protected]>
Subject: x86/PoD: handle intermediate page orders in p2m_pod_cache_add()

p2m_pod_decrease_reservation() may pass pages to the function which
aren't 4k, 2M, or 1G. Handle all intermediate orders as well, to avoid
hitting the BUG() at the switch() statement's "default" case.

This is CVE-2021-28708 / part of XSA-388.

Fixes: 3c352011c0d3 ("x86/PoD: shorten certain operations on higher order ranges")
Signed-off-by: Jan Beulich <[email protected]>
Reviewed-by: Roger Pau Monné <[email protected]>

--- a/xen/arch/x86/mm/p2m-pod.c
+++ b/xen/arch/x86/mm/p2m-pod.c
@@ -112,15 +112,13 @@ p2m_pod_cache_add(struct p2m_domain *p2m
     /* Then add to the appropriate populate-on-demand list. */
     switch ( order )
     {
-    case PAGE_ORDER_1G:
-        for ( i = 0; i < (1UL << PAGE_ORDER_1G); i += 1UL << PAGE_ORDER_2M )
+    case PAGE_ORDER_2M ... PAGE_ORDER_1G:
+        for ( i = 0; i < (1UL << order); i += 1UL << PAGE_ORDER_2M )
             page_list_add_tail(page + i, &p2m->pod.super);
         break;
-    case PAGE_ORDER_2M:
-        page_list_add_tail(page, &p2m->pod.super);
-        break;
-    case PAGE_ORDER_4K:
-        page_list_add_tail(page, &p2m->pod.single);
+    case PAGE_ORDER_4K ... PAGE_ORDER_2M - 1:
+        for ( i = 0; i < (1UL << order); i += 1UL << PAGE_ORDER_4K )
+            page_list_add_tail(page + i, &p2m->pod.single);
         break;
     default:
         BUG();