Xen Security Advisory 301 v3 (CVE-2019-18423) - add-to-physmap can be abused to DoS Arm hosts

From 21dfe8f707febd62869d4ebbaa155736870bebec Mon Sep 17 00:00:00 2001
From: Julien Grall <[email protected]>
Date: Wed, 2 Oct 2019 12:06:50 +0100
Subject: [PATCH 1/3] xen/arm: p2m: Avoid aliasing guest physical frame

The P2M helpers implementation is quite lax and will end up to ignore
the unused top bits of a guest physical frame.

This effectively means that p2m_set_entry() will create a mapping for a
different frame (it is always equal to gfn & (mask unused bits)). Yet
p2m->max_mapped_gfn will be updated using the original frame.

At the moment, p2m_get_entry() and p2m_resolve_translation_fault()
assume that p2m_get_root_pointer() will always return a non-NULL pointer
when the GFN is smaller than p2m->max_mapped_gfn.

Unfortunately, because of the aliasing described above, it would be
possible to set p2m->max_mapped_gfn high enough so it covers frame that
would lead p2m_get_root_pointer() to return NULL.

As we don't sanity check the guest physical frame provided by a guest, a
malicious guest could craft a series of hypercalls that will hit the
BUG_ON() and therefore DoS Xen.

To prevent aliasing, the function p2m_get_root_pointer() is now reworked
to return NULL If any of the unused top bits are not zero. The caller
can then decide what's the appropriate action to do. Since the two paths
(i.e. P2M_ROOT_PAGES == 1 and P2M_ROOT_PAGES != 1) are now very
similarly, take the opportunity to consolidate them making the code a
bit simpler.

With this change, p2m_get_entry() will not try to insert a mapping as
the root pointer is invalid.

Note that root_table is now switch to unsigned long as unsigned int is
not enough to hold part of a GFN.

This is part of XSA-301.

Reported-by: Julien Grall <[email protected]>
Signed-off-by: Julien Grall <[email protected]>
Reviewed-by: Stefano Stabellini <[email protected]>
---
 xen/arch/arm/p2m.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index d43c3aa896..3967ee7306 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -177,21 +177,14 @@ void p2m_tlb_flush_sync(struct p2m_domain *p2m)
 static lpae_t *p2m_get_root_pointer(struct p2m_domain *p2m,
                                     gfn_t gfn)
 {
-    unsigned int root_table;
-
-    if ( P2M_ROOT_PAGES == 1 )
-        return __map_domain_page(p2m->root);
+    unsigned long root_table;
 
     /*
-     * Concatenated root-level tables. The table number will be the
-     * offset at the previous level. It is not possible to
-     * concatenate a level-0 root.
+     * While the root table index is the offset from the previous level,
+     * we can't use (P2M_ROOT_LEVEL - 1) because the root level might be
+     * 0. Yet we still want to check if all the unused bits are zeroed.
      */
-    ASSERT(P2M_ROOT_LEVEL > 0);
-
-    root_table = gfn_x(gfn) >> (level_orders[P2M_ROOT_LEVEL - 1]);
-    root_table &= LPAE_ENTRY_MASK;
-
+    root_table = gfn_x(gfn) >> (level_orders[P2M_ROOT_LEVEL] + LPAE_SHIFT);
     if ( root_table >= P2M_ROOT_PAGES )
         return NULL;
 
-- 
2.11.0

From 4426d993b7ee0966fb39531dc5a269ce8493ca97 Mon Sep 17 00:00:00 2001
From: Julien Grall <[email protected]>
Date: Wed, 2 Oct 2019 12:35:59 +0100
Subject: [PATCH 2/3] xen/arm: p2m: Avoid off-by-one check on
 p2m->max_mapped_gfn

The code base is using inconsistently the field p2m->max_mapped_gfn.
Some of the useres expect that p2m->max_guest_gfn contain the highest
mapped GFN while others expect highest + 1.

p2m->max_guest_gfn is set as highest + 1, because of that the sanity
check on the GFN in p2m_resolved_translation_fault() and
p2m_get_entry() can be bypassed when GFN == p2m->max_guest_gfn.

p2m_get_root_pointer(p2m->max_guest_gfn) may return NULL if it is
outside of address range supported and therefore the BUG_ON() could be
hit.

The current value hold in p2m->max_mapped_gfn is inconsistent with the
expectation of the common code (see domain_get_maximum_gpfn()) and also
the documentation of the field.

Rather than changing the check in p2m_translation_fault() and
p2m_get_entry(), p2m->max_mapped_gfn is now containing the highest
mapped GFN and the callers assuming "highest + 1" are now adjusted.

Take the opportunity to use 1UL rather than 1 as page_order could
theoritically big enough to overflow a 32-bit integer.

Lastly, the documentation of the field max_guest_gfn to reflect how it
is computed.

This is part of XSA-301.

Reported-by: Julien Grall <[email protected]>
Signed-off-by: Julien Grall <[email protected]>
Reviewed-by: Stefano Stabellini <[email protected]>
---
 xen/arch/arm/p2m.c        | 6 +++---
 xen/include/asm-arm/p2m.h | 5 +----
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 3967ee7306..c7e049901d 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -931,7 +931,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
         p2m_write_pte(entry, pte, p2m->clean_pte);
 
         p2m->max_mapped_gfn = gfn_max(p2m->max_mapped_gfn,
-                                      gfn_add(sgfn, 1 << page_order));
+                                      gfn_add(sgfn, (1UL << page_order) - 1));
         p2m->lowest_mapped_gfn = gfn_min(p2m->lowest_mapped_gfn, sgfn);
     }
 
@@ -1291,7 +1291,7 @@ int relinquish_p2m_mapping(struct domain *d)
     p2m_write_lock(p2m);
 
     start = p2m->lowest_mapped_gfn;
-    end = p2m->max_mapped_gfn;
+    end = gfn_add(p2m->max_mapped_gfn, 1);
 
     for ( ; gfn_x(start) < gfn_x(end);
           start = gfn_next_boundary(start, order) )
@@ -1356,7 +1356,7 @@ int p2m_cache_flush(struct domain *d, gfn_t start, unsigned long nr)
     p2m_read_lock(p2m);
 
     start = gfn_max(start, p2m->lowest_mapped_gfn);
-    end = gfn_min(end, p2m->max_mapped_gfn);
+    end = gfn_min(end, gfn_add(p2m->max_mapped_gfn, 1));
 
     for ( ; gfn_x(start) < gfn_x(end); start = next_gfn )
     {
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 8823707c17..7f1f7e9109 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -38,10 +38,7 @@ struct p2m_domain {
     /* Current Translation Table Base Register for the p2m */
     uint64_t vttbr;
 
-    /*
-     * Highest guest frame that's ever been mapped in the p2m
-     * Only takes into account ram and foreign mapping
-     */
+    /* Highest guest frame that's ever been mapped in the p2m */
     gfn_t max_mapped_gfn;
 
     /*
-- 
2.11.0

From 61c73af08b4ede1fc8cfd2cf72661e6c7cfdbeaa Mon Sep 17 00:00:00 2001
From: Julien Grall <[email protected]>
Date: Wed, 2 Oct 2019 10:55:07 +0100
Subject: [PATCH 3/3] xen/arm: p2m: Don't check the return of
 p2m_get_root_pointer() with BUG_ON()

It turns out that the BUG_ON() was actually reachable with well-crafted
hypercalls. The BUG_ON() is here to prevent catch logical error, so
crashing Xen is a bit over the top.

While all the holes should now be fixed, it would be better to downgrade
the BUG_ON() to something less fatal to prevent any more DoS.

The BUG_ON() in p2m_get_entry() is now replaced by ASSERT_UNREACHABLE()
to catch mistake in debug build and return INVALID_MFN for production
build. The interface also requires to set page_order to give an idea of
the size of "hole". So 'level' is now set so we report a hole of size of
the an entry of the root page-table. This stays inline with what happen
when the GFN is higher than p2m->max_mapped_gfn.

This is part of XSA-301.

Reported-by: Julien Grall <[email protected]>
Signed-off-by: Julien Grall <[email protected]>
---
 xen/arch/arm/p2m.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index c7e049901d..af3515df42 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -318,7 +318,12 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
      * the table should always be non-NULL because the gfn is below
      * p2m->max_mapped_gfn and the root table pages are always present.
      */
-    BUG_ON(table == NULL);
+    if ( !table )
+    {
+        ASSERT_UNREACHABLE();
+        level = P2M_ROOT_LEVEL;
+        goto out;
+    }
 
     for ( level = P2M_ROOT_LEVEL; level < 3; level++ )
     {
-- 
2.11.0

From 19d6330f142cb941b6340a88592e8a294de0ff8c Mon Sep 17 00:00:00 2001
From: Julien Grall <[email protected]>
Date: Tue, 15 Oct 2019 17:10:40 +0100
Subject: [PATCH 1/3] xen/arm: p2m: Avoid aliasing guest physical frame

The P2M helpers implementation is quite lax and will end up to ignore
the unused top bits of a guest physical frame.

This effectively means that p2m_set_entry() will create a mapping for a
different frame (it is always equal to gfn & (mask unused bits)). Yet
p2m->max_mapped_gfn will be updated using the original frame.

At the moment, p2m_get_entry() and p2m_resolve_translation_fault()
assume that p2m_get_root_pointer() will always return a non-NULL pointer
when the GFN is smaller than p2m->max_mapped_gfn.

Unfortunately, because of the aliasing described above, it would be
possible to set p2m->max_mapped_gfn high enough so it covers frame that
would lead p2m_get_root_pointer() to return NULL.

As we don't sanity check the guest physical frame provided by a guest, a
malicious guest could craft a series of hypercalls that will hit the
BUG_ON() and therefore DoS Xen.

To prevent aliasing, the function p2m_get_root_pointer() is now reworked
to return NULL If any of the unused top bits are not zero. The caller
can then decide what's the appropriate action to do. Since the two paths
(i.e. P2M_ROOT_PAGES == 1 and P2M_ROOT_PAGES != 1) are now very
similarly, take the opportunity to consolidate them making the code a
bit simpler.

With this change, p2m_get_entry() will not try to insert a mapping as
the root pointer is invalid.

Note that root_table is now switch to unsigned long as unsigned int is
not enough to hold part of a GFN.

This is part of XSA-301.

Reported-by: Julien Grall <[email protected]>
Signed-off-by: Julien Grall <[email protected]>
Reviewed-by: Stefano Stabellini <[email protected]>
---
 xen/arch/arm/p2m.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index a2749d9b6f..d0045a8b28 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -229,21 +229,14 @@ void p2m_tlb_flush_sync(struct p2m_domain *p2m)
 static lpae_t *p2m_get_root_pointer(struct p2m_domain *p2m,
                                     gfn_t gfn)
 {
-    unsigned int root_table;
-
-    if ( P2M_ROOT_PAGES == 1 )
-        return __map_domain_page(p2m->root);
+    unsigned long root_table;
 
     /*
-     * Concatenated root-level tables. The table number will be the
-     * offset at the previous level. It is not possible to
-     * concatenate a level-0 root.
+     * While the root table index is the offset from the previous level,
+     * we can't use (P2M_ROOT_LEVEL - 1) because the root level might be
+     * 0. Yet we still want to check if all the unused bits are zeroed.
      */
-    ASSERT(P2M_ROOT_LEVEL > 0);
-
-    root_table = gfn_x(gfn) >> (level_orders[P2M_ROOT_LEVEL - 1]);
-    root_table &= LPAE_ENTRY_MASK;
-
+    root_table = gfn_x(gfn) >> (level_orders[P2M_ROOT_LEVEL] + LPAE_SHIFT);
     if ( root_table >= P2M_ROOT_PAGES )
         return NULL;
 
-- 
2.23.0

From 3b896936f7505e929dd869d14afcb185d0ee75f8 Mon Sep 17 00:00:00 2001
From: Julien Grall <[email protected]>
Date: Tue, 15 Oct 2019 17:10:41 +0100
Subject: [PATCH 2/3] xen/arm: p2m: Avoid off-by-one check on
 p2m->max_mapped_gfn

The code base is using inconsistently the field p2m->max_mapped_gfn.
Some of the useres expect that p2m->max_guest_gfn contain the highest
mapped GFN while others expect highest + 1.

p2m->max_guest_gfn is set as highest + 1, because of that the sanity
check on the GFN in p2m_resolved_translation_fault() and
p2m_get_entry() can be bypassed when GFN == p2m->max_guest_gfn.

p2m_get_root_pointer(p2m->max_guest_gfn) may return NULL if it is
outside of address range supported and therefore the BUG_ON() could be
hit.

The current value hold in p2m->max_mapped_gfn is inconsistent with the
expectation of the common code (see domain_get_maximum_gpfn()) and also
the documentation of the field.

Rather than changing the check in p2m_translation_fault() and
p2m_get_entry(), p2m->max_mapped_gfn is now containing the highest
mapped GFN and the callers assuming "highest + 1" are now adjusted.

Take the opportunity to use 1UL rather than 1 as page_order could
theoritically big enough to overflow a 32-bit integer.

Lastly, the documentation of the field max_guest_gfn to reflect how it
is computed.

This is part of XSA-301.

Reported-by: Julien Grall <[email protected]>
Signed-off-by: Julien Grall <[email protected]>
Reviewed-by: Stefano Stabellini <[email protected]>
---
 xen/arch/arm/p2m.c        | 6 +++---
 xen/include/asm-arm/p2m.h | 5 +----
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index d0045a8b28..8d20d27961 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1041,7 +1041,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
         p2m_write_pte(entry, pte, p2m->clean_pte);
 
         p2m->max_mapped_gfn = gfn_max(p2m->max_mapped_gfn,
-                                      gfn_add(sgfn, 1 << page_order));
+                                      gfn_add(sgfn, (1UL << page_order) - 1));
         p2m->lowest_mapped_gfn = gfn_min(p2m->lowest_mapped_gfn, sgfn);
     }
 
@@ -1572,7 +1572,7 @@ int relinquish_p2m_mapping(struct domain *d)
     p2m_write_lock(p2m);
 
     start = p2m->lowest_mapped_gfn;
-    end = p2m->max_mapped_gfn;
+    end = gfn_add(p2m->max_mapped_gfn, 1);
 
     for ( ; gfn_x(start) < gfn_x(end);
           start = gfn_next_boundary(start, order) )
@@ -1641,7 +1641,7 @@ int p2m_cache_flush_range(struct domain *d, gfn_t *pstart, gfn_t end)
     p2m_read_lock(p2m);
 
     start = gfn_max(start, p2m->lowest_mapped_gfn);
-    end = gfn_min(end, p2m->max_mapped_gfn);
+    end = gfn_min(end, gfn_add(p2m->max_mapped_gfn, 1));
 
     next_block_gfn = start;
 
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 89f82df380..5fdb6e8183 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -36,10 +36,7 @@ struct p2m_domain {
     /* Current Translation Table Base Register for the p2m */
     uint64_t vttbr;
 
-    /*
-     * Highest guest frame that's ever been mapped in the p2m
-     * Only takes into account ram and foreign mapping
-     */
+    /* Highest guest frame that's ever been mapped in the p2m */
     gfn_t max_mapped_gfn;
 
     /*
-- 
2.23.0

From 060c2dd3b7c2674a019d94afb2b4ebf3663f6c6e Mon Sep 17 00:00:00 2001
From: Julien Grall <[email protected]>
Date: Tue, 15 Oct 2019 17:10:42 +0100
Subject: [PATCH 3/3] xen/arm: p2m: Don't check the return of
 p2m_get_root_pointer() with BUG_ON()

It turns out that the BUG_ON() was actually reachable with well-crafted
hypercalls. The BUG_ON() is here to prevent catch logical error, so
crashing Xen is a bit over the top.

While all the holes should now be fixed, it would be better to downgrade
the BUG_ON() to something less fatal to prevent any more DoS.

The BUG_ON() in p2m_get_entry() is now replaced by ASSERT_UNREACHABLE()
to catch mistake in debug build and return INVALID_MFN for production
build. The interface also requires to set page_order to give an idea of
the size of "hole". So 'level' is now set so we report a hole of size of
the an entry of the root page-table. This stays inline with what happen
when the GFN is higher than p2m->max_mapped_gfn.

The BUG_ON() in p2m_resolve_translation_fault() is now replaced by
ASSERT_UNREACHABLE() to catch mistake in debug build and just report a
fault for producion build.

This is part of XSA-301.

Reported-by: Julien Grall <[email protected]>
Signed-off-by: Julien Grall <[email protected]>
Reviewed-by: Stefano Stabellini <[email protected]>
---
 xen/arch/arm/p2m.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 8d20d27961..ce59f2b503 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -395,7 +395,12 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
      * the table should always be non-NULL because the gfn is below
      * p2m->max_mapped_gfn and the root table pages are always present.
      */
-    BUG_ON(table == NULL);
+    if ( !table )
+    {
+        ASSERT_UNREACHABLE();
+        level = P2M_ROOT_LEVEL;
+        goto out;
+    }
 
     for ( level = P2M_ROOT_LEVEL; level < 3; level++ )
     {
@@ -1196,7 +1201,11 @@ bool p2m_resolve_translation_fault(struct domain *d, gfn_t gfn)
      * The table should always be non-NULL because the gfn is below
      * p2m->max_mapped_gfn and the root table pages are always present.
      */
-    BUG_ON(table == NULL);
+    if ( !table )
+    {
+        ASSERT_UNREACHABLE();
+        goto out;
+    }
 
     /*
      * Go down the page-tables until an entry has the valid bit unset or
-- 
2.23.0