[PATCH] x86/pv: Fill all Xen slots in init_guest_l4_table()

[PATCH] x86/pv: Fill all Xen slots in init_guest_l4_table()

[Andrew Cooper]

There is a bug when using highmem-start= where some L4 directmap slots are not
audited in alloc_l4_table(), and not overwritten by init_guest_l4_table().

As highmem_start is only available in debug builds of the hypervisor, this
does not constitute a security issue.

Ensure that init_guest_l4_table() writes to all of the Xen slots.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>

highmem-start= has actually bitrotten and is unusable.  This was tested by
hacking up a similar situation.
---
 xen/arch/x86/mm.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index b6d6ae3..791e526 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1596,6 +1596,10 @@ static int alloc_l3_table(struct page_info *page)
     return rc > 0 ? 0 : rc;
 }
 
+/*
+ * This function must write all ROOT_PAGETABLE_PV_XEN_SLOTS, to clobber any
+ * values a guest may have left there from alloc_l4_table().
+ */
 void init_guest_l4_table(l4_pgentry_t l4tab[], const struct domain *d,
                          bool zap_ro_mpt)
 {
@@ -1604,9 +1608,19 @@ void init_guest_l4_table(l4_pgentry_t l4tab[], const struct domain *d,
            &idle_pg_table[ROOT_PAGETABLE_FIRST_XEN_SLOT],
            root_pgt_pv_xen_slots * sizeof(l4_pgentry_t));
 #ifndef NDEBUG
-    if ( l4e_get_intpte(split_l4e) )
-        l4tab[ROOT_PAGETABLE_FIRST_XEN_SLOT + root_pgt_pv_xen_slots] =
-            split_l4e;
+    if ( unlikely(root_pgt_pv_xen_slots < ROOT_PAGETABLE_PV_XEN_SLOTS) )
+    {
+        l4_pgentry_t *next = &l4tab[ROOT_PAGETABLE_FIRST_XEN_SLOT +
+                                    root_pgt_pv_xen_slots];
+
+        if ( l4e_get_intpte(split_l4e) )
+            *next++ = split_l4e;
+
+        memset(next, 0,
+               _p(&l4tab[ROOT_PAGETABLE_LAST_XEN_SLOT + 1]) - _p(next));
+    }
+#else
+    BUILD_BUG_ON(root_pgt_pv_xen_slots != ROOT_PAGETABLE_PV_XEN_SLOTS);
 #endif
     l4tab[l4_table_offset(LINEAR_PT_VIRT_START)] =
         l4e_from_pfn(domain_page_map_to_mfn(l4tab), __PAGE_HYPERVISOR_RW);
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] x86/pv: Fill all Xen slots in init_guest_l4_table()

[Jan Beulich]

>>> On 29.08.17 at 12:24, <andrew.cooper3@citrix.com> wrote:
> There is a bug when using highmem-start= where some L4 directmap slots are not
> audited in alloc_l4_table(), and not overwritten by init_guest_l4_table().
> 
> As highmem_start is only available in debug builds of the hypervisor, this
> does not constitute a security issue.
> 
> Ensure that init_guest_l4_table() writes to all of the Xen slots.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
albeit ...

> @@ -1604,9 +1608,19 @@ void init_guest_l4_table(l4_pgentry_t l4tab[], const struct domain *d,
>             &idle_pg_table[ROOT_PAGETABLE_FIRST_XEN_SLOT],
>             root_pgt_pv_xen_slots * sizeof(l4_pgentry_t));
>  #ifndef NDEBUG
> -    if ( l4e_get_intpte(split_l4e) )
> -        l4tab[ROOT_PAGETABLE_FIRST_XEN_SLOT + root_pgt_pv_xen_slots] =
> -            split_l4e;
> +    if ( unlikely(root_pgt_pv_xen_slots < ROOT_PAGETABLE_PV_XEN_SLOTS) )
> +    {
> +        l4_pgentry_t *next = &l4tab[ROOT_PAGETABLE_FIRST_XEN_SLOT +
> +                                    root_pgt_pv_xen_slots];
> +
> +        if ( l4e_get_intpte(split_l4e) )
> +            *next++ = split_l4e;
> +
> +        memset(next, 0,
> +               _p(&l4tab[ROOT_PAGETABLE_LAST_XEN_SLOT + 1]) - _p(next));

... these disguised casts. But all alternatives I can think of make the
code more difficult to read.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel