Xen Security Advisory 385 v2 (CVE-2021-28706) - guests may exceed their designated memory limit

From: Julien Grall <[email protected]>
Subject: xen/page_alloc: Harden assign_pages()

domain_tot_pages() and d->max_pages are 32-bit values. While the order
should always be quite small, it would still be possible to overflow
if domain_tot_pages() is near to (2^32 - 1).

As this code may be called by a guest via XENMEM_increase_reservation
and XENMEM_populate_physmap, we want to make sure the guest is not going
to be able to allocate more than it is allowed.

Rework the allocation check to avoid any possible overflow. While the
check domain_tot_pages() < d->max_pages should technically not be
necessary, it is probably best to have it to catch any possible
inconsistencies in the future.

This is CVE-2021-28706 / part of XSA-385.

Signed-off-by: Julien Grall <[email protected]>
Signed-off-by: Jan Beulich <[email protected]>
Reviewed-by: Roger Pau Monné <[email protected]>
---
v3: Re-base.
v2: Limit to just the security relevant part. Use tot_pages (not
    d->tot_pages) as necessary. Also account for overflow from PGC_extra
    allocations. Use distinct error codes for the two cases. Avoid
    logging a bogus (wrapped) very small value. Also adjust
    gnttab_transfer().
---
There's yet another similar case in mem-sharing, but that's not security
supported, so doesn't get adjusted here.

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -2324,7 +2324,8 @@ gnttab_transfer(
          * pages when it is dying.
          */
         if ( unlikely(e->is_dying) ||
-             unlikely(domain_tot_pages(e) >= e->max_pages) )
+             unlikely(domain_tot_pages(e) >= e->max_pages) ||
+             unlikely(!(e->tot_pages + 1)) )
         {
             spin_unlock(&e->page_alloc_lock);
 
@@ -2333,8 +2334,8 @@ gnttab_transfer(
                          e->domain_id);
             else
                 gdprintk(XENLOG_INFO,
-                         "Transferee d%d has no headroom (tot %u, max %u)\n",
-                         e->domain_id, domain_tot_pages(e), e->max_pages);
+                         "Transferee %pd has no headroom (tot %u, max %u, ex %u)\n",
+                         e, domain_tot_pages(e), e->max_pages, e->extra_pages);
 
             gop.status = GNTST_general_error;
             goto unlock_and_copyback;
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2305,20 +2305,41 @@ int assign_pages(
     }
     else if ( !(memflags & MEMF_no_refcount) )
     {
-        unsigned int tot_pages = domain_tot_pages(d) + nr;
+        unsigned int tot_pages = domain_tot_pages(d);
 
         if ( unlikely(tot_pages > d->max_pages) )
         {
-            gprintk(XENLOG_INFO, "Over-allocation for domain %u: "
-                    "%u > %u\n", d->domain_id, tot_pages, d->max_pages);
+            gprintk(XENLOG_INFO, "Inconsistent allocation for %pd: %u > %u\n",
+                    d, tot_pages, d->max_pages);
+            rc = -EPERM;
+            goto out;
+        }
+
+        if ( unlikely(nr > d->max_pages - tot_pages) )
+        {
+            gprintk(XENLOG_INFO, "Over-allocation for %pd: %Lu > %u\n",
+                    d, tot_pages + 0ull + nr, d->max_pages);
             rc = -E2BIG;
             goto out;
         }
     }
 
-    if ( !(memflags & MEMF_no_refcount) &&
-         unlikely(domain_adjust_tot_pages(d, nr) == nr) )
-        get_knownalive_domain(d);
+    if ( !(memflags & MEMF_no_refcount) )
+    {
+        if ( unlikely(d->tot_pages + nr < nr) )
+        {
+            gprintk(XENLOG_INFO,
+                    "Excess allocation for %pd: %Lu (%u extra)\n",
+                    d, d->tot_pages + 0ull + nr, d->extra_pages);
+            if ( pg[0].count_info & PGC_extra )
+                d->extra_pages -= nr;
+            rc = -E2BIG;
+            goto out;
+        }
+
+        if ( unlikely(domain_adjust_tot_pages(d, nr) == nr) )
+            get_knownalive_domain(d);
+    }
 
     for ( i = 0; i < nr; i++ )
     {

From: Jan Beulich <[email protected]>
Subject: SUPPORT.md: limit security support for hosts with very much memory

Sufficient and in particular regular testing on very large hosts cannot
currently be guaranteed. Anyone wanting us to support larger hosts is
free to propose so, but will need to supply not only test results, but
also a test plan.

This is part of XSA-385.

Signed-off-by: Jan Beulich <[email protected]>

--- a/SUPPORT.md
+++ b/SUPPORT.md
@@ -48,6 +48,12 @@ For the Cortex A57 r0p0 - r1p1, see Erra
 
     Status, x86: Supported
 
+### Physical Memory
+
+    Status: Supported up to 8 TiB
+
+Hosts with more memory are supported, but not security supported.
+
 ### Physical Memory Hotplug
 
     Status, x86: Supported

From: Julien Grall <[email protected]>
Subject: xen/page_alloc: Harden assign_pages()

domain_tot_pages() and d->max_pages are 32-bit values. While the order
should always be quite small, it would still be possible to overflow
if domain_tot_pages() is near to (2^32 - 1).

As this code may be called by a guest via XENMEM_increase_reservation
and XENMEM_populate_physmap, we want to make sure the guest is not going
to be able to allocate more than it is allowed.

Rework the allocation check to avoid any possible overflow. While the
check domain_tot_pages() < d->max_pages should technically not be
necessary, it is probably best to have it to catch any possible
inconsistencies in the future.

This is CVE-2021-28706 / XSA-385.

Signed-off-by: Julien Grall <[email protected]>
Signed-off-by: Jan Beulich <[email protected]>
Reviewed-by: Roger Pau Monné <[email protected]>

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -2239,7 +2239,8 @@ gnttab_transfer(
          * pages when it is dying.
          */
         if ( unlikely(e->is_dying) ||
-             unlikely(e->tot_pages >= e->max_pages) )
+             unlikely(e->tot_pages >= e->max_pages) ||
+             unlikely(!(e->tot_pages + 1)) )
         {
             spin_unlock(&e->page_alloc_lock);
 
@@ -2248,8 +2249,8 @@ gnttab_transfer(
                          e->domain_id);
             else
                 gdprintk(XENLOG_INFO,
-                         "Transferee d%d has no headroom (tot %u, max %u)\n",
-                         e->domain_id, e->tot_pages, e->max_pages);
+                         "Transferee %pd has no headroom (tot %u, max %u)\n",
+                         e, e->tot_pages, e->max_pages);
 
             gop.status = GNTST_general_error;
             goto unlock_and_copyback;
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2278,17 +2278,26 @@ int assign_pages(
 
     if ( !(memflags & MEMF_no_refcount) )
     {
-        if ( unlikely((d->tot_pages + (1 << order)) > d->max_pages) )
+        unsigned int nr = 1u << order;
+
+        if ( unlikely(d->tot_pages > d->max_pages) )
+        {
+            gprintk(XENLOG_INFO, "Inconsistent allocation for %pd: %u > %u\n",
+                    d, d->tot_pages, d->max_pages);
+            rc = -EPERM;
+            goto out;
+        }
+
+        if ( unlikely(nr > d->max_pages - d->tot_pages) )
         {
             if ( !tmem_enabled() || order != 0 || d->tot_pages != d->max_pages )
-                gprintk(XENLOG_INFO, "Over-allocation for domain %u: "
-                        "%u > %u\n", d->domain_id,
-                        d->tot_pages + (1 << order), d->max_pages);
+                gprintk(XENLOG_INFO, "Over-allocation for %pd: %Lu > %u\n",
+                        d, d->tot_pages + 0ull + nr, d->max_pages);
             rc = -E2BIG;
             goto out;
         }
 
-        if ( unlikely(domain_adjust_tot_pages(d, 1 << order) == (1 << order)) )
+        if ( unlikely(domain_adjust_tot_pages(d, nr) == nr) )
             get_knownalive_domain(d);
     }

From: Julien Grall <[email protected]>
Subject: xen/page_alloc: Harden assign_pages()

domain_tot_pages() and d->max_pages are 32-bit values. While the order
should always be quite small, it would still be possible to overflow
if domain_tot_pages() is near to (2^32 - 1).

As this code may be called by a guest via XENMEM_increase_reservation
and XENMEM_populate_physmap, we want to make sure the guest is not going
to be able to allocate more than it is allowed.

Rework the allocation check to avoid any possible overflow. While the
check domain_tot_pages() < d->max_pages should technically not be
necessary, it is probably best to have it to catch any possible
inconsistencies in the future.

This is CVE-2021-28706 / XSA-385.

Signed-off-by: Julien Grall <[email protected]>
Signed-off-by: Jan Beulich <[email protected]>
Reviewed-by: Roger Pau Monné <[email protected]>

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -2286,7 +2286,8 @@ gnttab_transfer(
          * pages when it is dying.
          */
         if ( unlikely(e->is_dying) ||
-             unlikely(e->tot_pages >= e->max_pages) )
+             unlikely(e->tot_pages >= e->max_pages) ||
+             unlikely(!(e->tot_pages + 1)) )
         {
             spin_unlock(&e->page_alloc_lock);
 
@@ -2295,8 +2296,8 @@ gnttab_transfer(
                          e->domain_id);
             else
                 gdprintk(XENLOG_INFO,
-                         "Transferee d%d has no headroom (tot %u, max %u)\n",
-                         e->domain_id, e->tot_pages, e->max_pages);
+                         "Transferee %pd has no headroom (tot %u, max %u)\n",
+                         e, e->tot_pages, e->max_pages);
 
             gop.status = GNTST_general_error;
             goto unlock_and_copyback;
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2276,16 +2276,25 @@ int assign_pages(
 
     if ( !(memflags & MEMF_no_refcount) )
     {
-        if ( unlikely((d->tot_pages + (1 << order)) > d->max_pages) )
+        unsigned int nr = 1u << order;
+
+        if ( unlikely(d->tot_pages > d->max_pages) )
+        {
+            gprintk(XENLOG_INFO, "Inconsistent allocation for %pd: %u > %u\n",
+                    d, d->tot_pages, d->max_pages);
+            rc = -EPERM;
+            goto out;
+        }
+
+        if ( unlikely(nr > d->max_pages - d->tot_pages) )
         {
-            gprintk(XENLOG_INFO, "Over-allocation for domain %u: "
-                    "%u > %u\n", d->domain_id,
-                    d->tot_pages + (1 << order), d->max_pages);
+            gprintk(XENLOG_INFO, "Over-allocation for %pd: %Lu > %u\n",
+                    d, d->tot_pages + 0ull + nr, d->max_pages);
             rc = -E2BIG;
             goto out;
         }
 
-        if ( unlikely(domain_adjust_tot_pages(d, 1 << order) == (1 << order)) )
+        if ( unlikely(domain_adjust_tot_pages(d, nr) == nr) )
             get_knownalive_domain(d);
     }

From: Julien Grall <[email protected]>
Subject: xen/page_alloc: Harden assign_pages()

domain_tot_pages() and d->max_pages are 32-bit values. While the order
should always be quite small, it would still be possible to overflow
if domain_tot_pages() is near to (2^32 - 1).

As this code may be called by a guest via XENMEM_increase_reservation
and XENMEM_populate_physmap, we want to make sure the guest is not going
to be able to allocate more than it is allowed.

Rework the allocation check to avoid any possible overflow. While the
check domain_tot_pages() < d->max_pages should technically not be
necessary, it is probably best to have it to catch any possible
inconsistencies in the future.

This is CVE-2021-28706 / XSA-385.

Signed-off-by: Julien Grall <[email protected]>
Signed-off-by: Jan Beulich <[email protected]>

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -2336,7 +2336,8 @@ gnttab_transfer(
          * pages when it is dying.
          */
         if ( unlikely(e->is_dying) ||
-             unlikely(domain_tot_pages(e) >= e->max_pages) )
+             unlikely(domain_tot_pages(e) >= e->max_pages) ||
+             unlikely(!(e->tot_pages + 1)) )
         {
             spin_unlock(&e->page_alloc_lock);
 
@@ -2345,8 +2346,8 @@ gnttab_transfer(
                          e->domain_id);
             else
                 gdprintk(XENLOG_INFO,
-                         "Transferee d%d has no headroom (tot %u, max %u)\n",
-                         e->domain_id, domain_tot_pages(e), e->max_pages);
+                         "Transferee %pd has no headroom (tot %u, max %u, ex %u)\n",
+                         e, domain_tot_pages(e), e->max_pages, e->extra_pages);
 
             gop.status = GNTST_general_error;
             goto unlock_and_copyback;
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2298,20 +2298,43 @@ int assign_pages(
     }
     else if ( !(memflags & MEMF_no_refcount) )
     {
-        unsigned int tot_pages = domain_tot_pages(d) + (1 << order);
+        unsigned int tot_pages = domain_tot_pages(d), nr = 1u << order;
 
         if ( unlikely(tot_pages > d->max_pages) )
         {
-            gprintk(XENLOG_INFO, "Over-allocation for domain %u: "
-                    "%u > %u\n", d->domain_id, tot_pages, d->max_pages);
+            gprintk(XENLOG_INFO, "Inconsistent allocation for %pd: %u > %u\n",
+                    d, tot_pages, d->max_pages);
+            rc = -EPERM;
+            goto out;
+        }
+
+        if ( unlikely(nr > d->max_pages - tot_pages) )
+        {
+            gprintk(XENLOG_INFO, "Over-allocation for %pd: %Lu > %u\n",
+                    d, tot_pages + 0ull + nr, d->max_pages);
             rc = -E2BIG;
             goto out;
         }
     }
 
-    if ( !(memflags & MEMF_no_refcount) &&
-         unlikely(domain_adjust_tot_pages(d, 1 << order) == (1 << order)) )
-        get_knownalive_domain(d);
+    if ( !(memflags & MEMF_no_refcount) )
+    {
+        unsigned int nr = 1u << order;
+
+        if ( unlikely(d->tot_pages + nr < nr) )
+        {
+            gprintk(XENLOG_INFO,
+                    "Excess allocation for %pd: %Lu (%u extra)\n",
+                    d, d->tot_pages + 0ull + nr, d->extra_pages);
+            if ( pg[0].count_info & PGC_extra )
+                d->extra_pages -= nr;
+            rc = -E2BIG;
+            goto out;
+        }
+
+        if ( unlikely(domain_adjust_tot_pages(d, nr) == nr) )
+            get_knownalive_domain(d);
+    }
 
     for ( i = 0; i < (1 << order); i++ )
     {