[Xen-devel] [PATCH v4] x86/mm: Add mem access rights to NPT

[Alexandru Isaila <aisaila@xxxxxxxxxxxxxxx>]

From: Isaila Alexandru <aisaila@xxxxxxxxxxxxxxx>

This patch adds access control for NPT mode.

There aren’t enough extra bits to store the access rights in the NPT p2m
table, so we add a radix tree to store the rights.  For efficiency,
remove entries which match the default permissions rather than
continuing to store them.

Modify p2m-pt.c:p2m_type_to_flags() to mirror the ept version: taking an
access, and removing / adding RW or NX flags as appropriate.

Note: It was tested with xen-access write

Signed-off-by: Alexandru Isaila <aisaila@xxxxxxxxxxxxxxx>

---
Changes since V3:
        - Add p2m_pt_check_access() to filter n, w, wx, n2rwx from
          supported page rights
        - Add rights check for the default_access change in the
          IVALID_GFN case
        - Add blank lines
        - Remove cpu_has_svm if from p2m_mem_access_check()
        - Add xfree(msr_bitmap) in case of error on
          xalloc(raxid_tree_root).
---
 xen/arch/x86/mm/mem_access.c     |  17 +++---
 xen/arch/x86/mm/p2m-ept.c        |   7 +++
 xen/arch/x86/mm/p2m-pt.c         | 124 ++++++++++++++++++++++++++++++++++-----
 xen/arch/x86/mm/p2m.c            |   6 ++
 xen/arch/x86/monitor.c           |  15 +++++
 xen/include/asm-x86/mem_access.h |   2 +-
 xen/include/asm-x86/p2m.h        |   7 +++
 7 files changed, 156 insertions(+), 22 deletions(-)

diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index c0cd017..cab72bc 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -221,12 +221,12 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
         {
             req->u.mem_access.flags |= MEM_ACCESS_GLA_VALID;
             req->u.mem_access.gla = gla;
-
-            if ( npfec.kind == npfec_kind_with_gla )
-                req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA;
-            else if ( npfec.kind == npfec_kind_in_gpt )
-                req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT;
         }
+
+        if ( npfec.kind == npfec_kind_with_gla )
+            req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA;
+        else if ( npfec.kind == npfec_kind_in_gpt )
+            req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT;
         req->u.mem_access.flags |= npfec.read_access    ? MEM_ACCESS_R : 0;
         req->u.mem_access.flags |= npfec.write_access   ? MEM_ACCESS_W : 0;
         req->u.mem_access.flags |= npfec.insn_fetch     ? MEM_ACCESS_X : 0;
@@ -366,8 +366,11 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, 
uint32_t nr,
     /* If request to set default access. */
     if ( gfn_eq(gfn, INVALID_GFN) )
     {
-        p2m->default_access = a;
-        return 0;
+        if ( (rc = p2m->check_access(a)) == 0 )
+        {
+            p2m->default_access = a;
+            return 0;
+        }
     }
 
     p2m_lock(p2m);
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 14b5939..de26aa1 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -667,6 +667,12 @@ bool_t ept_handle_misconfig(uint64_t gpa)
     return spurious ? (rc >= 0) : (rc > 0);
 }
 
+int ept_check_access(p2m_access_t p2ma)
+{
+    /* All access is permitted */
+    return 0;
+}
+
 /*
  * ept_set_entry() computes 'need_modify_vtd_table' for itself,
  * by observing whether any gfn->mfn translations are modified.
@@ -1255,6 +1261,7 @@ int ept_p2m_init(struct p2m_domain *p2m)
     p2m->change_entry_type_global = ept_change_entry_type_global;
     p2m->change_entry_type_range = ept_change_entry_type_range;
     p2m->memory_type_changed = ept_memory_type_changed;
+    p2m->check_access = ept_check_access;
     p2m->audit_p2m = NULL;
     p2m->tlb_flush = ept_tlb_flush;
 
diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index b8c5d2e..951cbe5 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -68,7 +68,8 @@
 static unsigned long p2m_type_to_flags(const struct p2m_domain *p2m,
                                        p2m_type_t t,
                                        mfn_t mfn,
-                                       unsigned int level)
+                                       unsigned int level,
+                                       p2m_access_t access)
 {
     unsigned long flags;
     /*
@@ -87,23 +88,27 @@ static unsigned long p2m_type_to_flags(const struct 
p2m_domain *p2m,
     case p2m_ram_paged:
     case p2m_ram_paging_in:
     default:
-        return flags | _PAGE_NX_BIT;
+        flags |= P2M_BASE_FLAGS | _PAGE_NX_BIT;
+        break;
     case p2m_grant_map_ro:
         return flags | P2M_BASE_FLAGS | _PAGE_NX_BIT;
     case p2m_ioreq_server:
         flags |= P2M_BASE_FLAGS | _PAGE_RW | _PAGE_NX_BIT;
         if ( p2m->ioreq.flags & XEN_DMOP_IOREQ_MEM_ACCESS_WRITE )
-            return flags & ~_PAGE_RW;
-        return flags;
+            flags &= ~_PAGE_RW;
+        break;
     case p2m_ram_ro:
     case p2m_ram_logdirty:
     case p2m_ram_shared:
-        return flags | P2M_BASE_FLAGS;
+        flags |= P2M_BASE_FLAGS;
+        break;
     case p2m_ram_rw:
-        return flags | P2M_BASE_FLAGS | _PAGE_RW;
+        flags |= P2M_BASE_FLAGS | _PAGE_RW;
+        break;
     case p2m_grant_map_rw:
     case p2m_map_foreign:
-        return flags | P2M_BASE_FLAGS | _PAGE_RW | _PAGE_NX_BIT;
+        flags |= P2M_BASE_FLAGS | _PAGE_RW | _PAGE_NX_BIT;
+        break;
     case p2m_mmio_direct:
         if ( !rangeset_contains_singleton(mmio_ro_ranges, mfn_x(mfn)) )
             flags |= _PAGE_RW;
@@ -112,8 +117,32 @@ static unsigned long p2m_type_to_flags(const struct 
p2m_domain *p2m,
             flags |= _PAGE_PWT;
             ASSERT(!level);
         }
-        return flags | P2M_BASE_FLAGS | _PAGE_PCD;
+        flags |= P2M_BASE_FLAGS | _PAGE_PCD;
+        break;
+    }
+
+    switch ( access )
+    {
+    case p2m_access_r:
+        flags |= _PAGE_NX_BIT;
+        flags &= ~_PAGE_RW;
+        break;
+    case p2m_access_rw:
+        flags |= _PAGE_NX_BIT;
+        break;
+    case p2m_access_rx:
+    case p2m_access_rx2rw:
+        flags &= ~(_PAGE_NX_BIT | _PAGE_RW);
+        break;
+    case p2m_access_x:
+        flags &= ~_PAGE_RW;
+        break;
+    case p2m_access_rwx:
+    default:
+        break;
     }
+
+    return flags;
 }
 
 
@@ -174,6 +203,44 @@ static void p2m_add_iommu_flags(l1_pgentry_t *p2m_entry,
         l1e_add_flags(*p2m_entry, iommu_nlevel_to_flags(nlevel, flags));
 }
 
+static p2m_access_t p2m_get_access(struct p2m_domain *p2m, unsigned long gfn)
+{
+    void *ptr;
+
+    if ( !p2m->mem_access_settings )
+        return p2m_access_rwx;
+
+    ptr = radix_tree_lookup(p2m->mem_access_settings, gfn);
+    if ( !ptr )
+        return p2m_access_rwx;
+    else
+        return radix_tree_ptr_to_int(ptr);
+}
+
+static void p2m_set_access(struct p2m_domain *p2m, unsigned long gfn,
+                                      p2m_access_t a)
+{
+    int rc;
+
+    if ( !p2m->mem_access_settings )
+        return;
+
+    if ( p2m_access_rwx == a )
+    {
+        radix_tree_delete(p2m->mem_access_settings, gfn);
+        return;
+    }
+
+    rc = radix_tree_insert(p2m->mem_access_settings, gfn,
+                           radix_tree_int_to_ptr(a));
+    if ( rc == -EEXIST )
+        /* If a setting already exists, change it to the new one. */
+        radix_tree_replace_slot(
+            radix_tree_lookup_slot(
+                p2m->mem_access_settings, gfn),
+            radix_tree_int_to_ptr(a));
+}
+
 /* Returns: 0 for success, -errno for failure */
 static int
 p2m_next_level(struct p2m_domain *p2m, void **table,
@@ -201,6 +268,7 @@ p2m_next_level(struct p2m_domain *p2m, void **table,
         new_entry = l1e_from_mfn(mfn, P2M_BASE_FLAGS | _PAGE_RW);
 
         p2m_add_iommu_flags(&new_entry, level, 
IOMMUF_readable|IOMMUF_writable);
+        p2m_set_access(p2m, gfn, p2m->default_access);
         p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry, level + 1);
     }
     else if ( flags & _PAGE_PSE )
@@ -249,6 +317,7 @@ p2m_next_level(struct p2m_domain *p2m, void **table,
         {
             new_entry = l1e_from_pfn(pfn | (i << ((level - 1) * 
PAGETABLE_ORDER)),
                                      flags);
+            p2m_set_access(p2m, gfn, p2m->default_access);
             p2m->write_p2m_entry(p2m, gfn, l1_entry + i, new_entry, level);
         }
 
@@ -256,6 +325,7 @@ p2m_next_level(struct p2m_domain *p2m, void **table,
 
         new_entry = l1e_from_mfn(mfn, P2M_BASE_FLAGS | _PAGE_RW);
         p2m_add_iommu_flags(&new_entry, level, 
IOMMUF_readable|IOMMUF_writable);
+        p2m_set_access(p2m, gfn, p2m->default_access);
         p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry, level + 1);
     }
     else
@@ -420,8 +490,9 @@ static int do_recalc(struct p2m_domain *p2m, unsigned long 
gfn)
         if ( nt != ot )
         {
             unsigned long mfn = l1e_get_pfn(e);
+            p2m_access_t a = p2m_get_access(p2m, gfn);
             unsigned long flags = p2m_type_to_flags(p2m, nt,
-                                                    _mfn(mfn), level);
+                                                    _mfn(mfn), level, a);
 
             if ( level )
             {
@@ -472,6 +543,22 @@ int p2m_pt_handle_deferred_changes(uint64_t gpa)
 }
 
 /* Returns: 0 for success, -errno for failure */
+static int p2m_pt_check_access(p2m_access_t p2ma)
+{
+    switch ( p2ma )
+    {
+    case p2m_access_n:
+    case p2m_access_w:
+    case p2m_access_wx:
+    case p2m_access_n2rwx:
+        return -EINVAL;
+    default:
+        break;
+    }
+    return 0;
+}
+
+/* Returns: 0 for success, -errno for failure */
 static int
 p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn,
                  unsigned int page_order, p2m_type_t p2mt, p2m_access_t p2ma,
@@ -503,6 +590,9 @@ p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t 
mfn,
 
     ASSERT(sve != 0);
 
+    if ( (rc = p2m_pt_check_access(p2ma)) != 0 )
+        return rc;
+
     if ( tb_init_done )
     {
         struct {
@@ -569,13 +659,14 @@ p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_, 
mfn_t mfn,
         ASSERT(!mfn_valid(mfn) || p2mt != p2m_mmio_direct);
         l3e_content = mfn_valid(mfn) || p2m_allows_invalid_mfn(p2mt)
             ? p2m_l3e_from_pfn(mfn_x(mfn),
-                               p2m_type_to_flags(p2m, p2mt, mfn, 2))
+                               p2m_type_to_flags(p2m, p2mt, mfn, 2, p2ma))
             : l3e_empty();
         entry_content.l1 = l3e_content.l3;
 
         if ( entry_content.l1 != 0 )
             p2m_add_iommu_flags(&entry_content, 0, iommu_pte_flags);
 
+        p2m_set_access(p2m, gfn, p2ma);
         p2m->write_p2m_entry(p2m, gfn, p2m_entry, entry_content, 3);
         /* NB: paging_write_p2m_entry() handles tlb flushes properly */
     }
@@ -608,7 +699,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t 
mfn,
 
         if ( mfn_valid(mfn) || p2m_allows_invalid_mfn(p2mt) )
             entry_content = p2m_l1e_from_pfn(mfn_x(mfn),
-                                         p2m_type_to_flags(p2m, p2mt, mfn, 0));
+                                         p2m_type_to_flags(p2m, p2mt, mfn, 0, 
p2ma));
         else
             entry_content = l1e_empty();
 
@@ -630,6 +721,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t 
mfn,
             p2m->ioreq.entry_count--;
         }
 
+        p2m_set_access(p2m, gfn, p2ma);
         /* level 1 entry */
         p2m->write_p2m_entry(p2m, gfn, p2m_entry, entry_content, 1);
         /* NB: paging_write_p2m_entry() handles tlb flushes properly */
@@ -661,13 +753,14 @@ p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_, 
mfn_t mfn,
         ASSERT(!mfn_valid(mfn) || p2mt != p2m_mmio_direct);
         l2e_content = mfn_valid(mfn) || p2m_allows_invalid_mfn(p2mt)
             ? p2m_l2e_from_pfn(mfn_x(mfn),
-                               p2m_type_to_flags(p2m, p2mt, mfn, 1))
+                               p2m_type_to_flags(p2m, p2mt, mfn, 1, p2ma))
             : l2e_empty();
         entry_content.l1 = l2e_content.l2;
 
         if ( entry_content.l1 != 0 )
             p2m_add_iommu_flags(&entry_content, 0, iommu_pte_flags);
 
+        p2m_set_access(p2m, gfn, p2ma);
         p2m->write_p2m_entry(p2m, gfn, p2m_entry, entry_content, 2);
         /* NB: paging_write_p2m_entry() handles tlb flushes properly */
     }
@@ -749,8 +842,7 @@ p2m_pt_get_entry(struct p2m_domain *p2m, gfn_t gfn_,
      * XXX Once we start explicitly registering MMIO regions in the p2m 
      * XXX we will return p2m_invalid for unmapped gfns */
     *t = p2m_mmio_dm;
-    /* Not implemented except with EPT */
-    *a = p2m_access_rwx; 
+    *a = p2m_access_n;
 
     if ( gfn > p2m->max_mapped_pfn )
     {
@@ -813,6 +905,7 @@ pod_retry_l3:
                        l1_table_offset(addr));
             *t = p2m_recalc_type(recalc || _needs_recalc(flags),
                                  p2m_flags_to_type(flags), p2m, gfn);
+            *a = p2m_get_access(p2m, gfn);
             unmap_domain_page(l3e);
 
             ASSERT(mfn_valid(mfn) || !p2m_is_ram(*t));
@@ -852,6 +945,7 @@ pod_retry_l2:
         mfn = _mfn(l2e_get_pfn(*l2e) + l1_table_offset(addr));
         *t = p2m_recalc_type(recalc || _needs_recalc(flags),
                              p2m_flags_to_type(flags), p2m, gfn);
+        *a = p2m_get_access(p2m, gfn);
         unmap_domain_page(l2e);
         
         ASSERT(mfn_valid(mfn) || !p2m_is_ram(*t));
@@ -888,6 +982,7 @@ pod_retry_l1:
     }
     mfn = l1e_get_mfn(*l1e);
     *t = p2m_recalc_type(recalc || _needs_recalc(flags), l1t, p2m, gfn);
+    *a = p2m_get_access(p2m, gfn);
     unmap_domain_page(l1e);
 
     ASSERT(mfn_valid(mfn) || !p2m_is_ram(*t) || p2m_is_paging(*t));
@@ -1127,6 +1222,7 @@ void p2m_pt_init(struct p2m_domain *p2m)
     p2m->change_entry_type_global = p2m_pt_change_entry_type_global;
     p2m->change_entry_type_range = p2m_pt_change_entry_type_range;
     p2m->write_p2m_entry = paging_write_p2m_entry;
+    p2m->check_access = p2m_pt_check_access;
 #if P2M_AUDIT
     p2m->audit_p2m = p2m_pt_audit_p2m;
 #else
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index c53cab4..12e2d24 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -675,6 +675,12 @@ void p2m_teardown(struct p2m_domain *p2m)
 
     d = p2m->domain;
 
+    if ( p2m->mem_access_settings )
+    {
+        radix_tree_destroy(p2m->mem_access_settings, NULL);
+        xfree(p2m->mem_access_settings);
+    }
+
     p2m_lock(p2m);
     ASSERT(atomic_read(&d->shr_pages) == 0);
     p2m->phys_table = pagetable_null();
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 3fb6531..b871411 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -20,10 +20,13 @@
  */
 
 #include <asm/monitor.h>
+#include <asm/p2m.h>
 #include <public/vm_event.h>
 
 int arch_monitor_init_domain(struct domain *d)
 {
+    struct p2m_domain *p2m = p2m_get_hostp2m(d);
+
     if ( !d->arch.monitor.msr_bitmap )
         d->arch.monitor.msr_bitmap = xzalloc_array(struct monitor_msr_bitmap,
                                                    2);
@@ -31,6 +34,18 @@ int arch_monitor_init_domain(struct domain *d)
     if ( !d->arch.monitor.msr_bitmap )
         return -ENOMEM;
 
+    if ( cpu_has_svm && !p2m->mem_access_settings )
+    {
+        p2m->mem_access_settings = xmalloc(struct radix_tree_root);
+
+        if( !p2m->mem_access_settings )
+        {
+            xfree(d->arch.monitor.msr_bitmap);
+            return -ENOMEM;
+        }
+        radix_tree_init(p2m->mem_access_settings);
+    }
+
     return 0;
 }
 
diff --git a/xen/include/asm-x86/mem_access.h b/xen/include/asm-x86/mem_access.h
index 4043c9f..34f2c07 100644
--- a/xen/include/asm-x86/mem_access.h
+++ b/xen/include/asm-x86/mem_access.h
@@ -46,7 +46,7 @@ bool p2m_mem_access_emulate_check(struct vcpu *v,
 /* Sanity check for mem_access hardware support */
 static inline bool p2m_mem_access_sanity_check(struct domain *d)
 {
-    return is_hvm_domain(d) && cpu_has_vmx && hap_enabled(d);
+    return is_hvm_domain(d) && hap_enabled(d);
 }
 
 #endif /*__ASM_X86_MEM_ACCESS_H__ */
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index d4b3cfc..a190151 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -266,6 +266,7 @@ struct p2m_domain {
                                           unsigned long gfn, l1_pgentry_t *p,
                                           l1_pgentry_t new, unsigned int 
level);
     long               (*audit_p2m)(struct p2m_domain *p2m);
+    int                (*check_access)(p2m_access_t p2ma);
 
     /*
      * P2M updates may require TLBs to be flushed (invalidated).
@@ -288,6 +289,12 @@ struct p2m_domain {
      * retyped get this access type.  See definition of p2m_access_t. */
     p2m_access_t default_access;
 
+    /*
+     * Radix tree to store the p2m_access_t settings as the pte's don't have
+     * enough available bits to store this information.
+     */
+    struct radix_tree_root *mem_access_settings;
+
     /* If true, and an access fault comes in and there is no vm_event 
listener, 
      * pause domain.  Otherwise, remove access restrictions. */
     bool_t       access_required;
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel