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

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

[Alexandru Isaila]

From: Isaila Alexandru <aisaila@bitdefender.com>

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@bitdefender.com>

---
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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v4] x86/mm: Add mem access rights to NPT

[Jan Beulich]

>>> On 23.07.18 at 15:48, <aisaila@bitdefender.com> wrote:
> --- 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;

Without explanation in the commit message and without comment
this change is a no-go imo: I consider it at least questionable to
have npfec_kind_with_gla without .gla_valid set to true. Perhaps
it's just a naming issue, but that would then still require half a
sentence to explain.

> @@ -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;
> +        }
>      }

This latching into rc makes subsequent code yield inconsistent
behavior.

> --- 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;
> +}

With this I'd rather see the hook omitted here, to avoid the indirect
call. Perhaps you'll want a wrapper around the indirect call, abstracting
away the NULL check for callers.

> +static void p2m_set_access(struct p2m_domain *p2m, unsigned long gfn,
> +                                      p2m_access_t a)
> +{
> +    int rc;
> +
> +    if ( !p2m->mem_access_settings )
> +        return;

No error indication?

> +    if ( p2m_access_rwx == a )
> +    {
> +        radix_tree_delete(p2m->mem_access_settings, gfn);
> +        return;
> +    }

Is it really p2m_access_rwx that you want to special case here, rather
than ->default_access?

> @@ -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 )

Style.

> +        {
> +            xfree(d->arch.monitor.msr_bitmap);
> +            return -ENOMEM;
> +        }
> +        radix_tree_init(p2m->mem_access_settings);
> +    }

What's the SVM connection here? Please don't forget that p2m-pt.c
also serves the shadow case. Perhaps struct p2m_domain should
contain a boolean indicator whether this auxiliary data structure is
needed?

Jan



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

Re: [PATCH v4] x86/mm: Add mem access rights to NPT

[Razvan Cojocaru]

On 07/24/2018 11:55 AM, Jan Beulich wrote:
>> +    if ( cpu_has_svm && !p2m->mem_access_settings )
>> +    {
>> +        p2m->mem_access_settings = xmalloc(struct radix_tree_root);
>> +
>> +        if( !p2m->mem_access_settings )
> Style.
> 
>> +        {
>> +            xfree(d->arch.monitor.msr_bitmap);
>> +            return -ENOMEM;
>> +        }
>> +        radix_tree_init(p2m->mem_access_settings);
>> +    }
> What's the SVM connection here? Please don't forget that p2m-pt.c
> also serves the shadow case. Perhaps struct p2m_domain should
> contain a boolean indicator whether this auxiliary data structure is
> needed?

Would it not work to simply check for "if ( cpu_has_svm &&
p2m_is_hostp2m(p2m) && !p2m->mem_access_settings )" here?

In the shadow case, will not p2m->p2m_class be p2m_nested?


Thanks,
Razvan

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

Re: [PATCH v4] x86/mm: Add mem access rights to NPT

[Jan Beulich]

>>> On 24.07.18 at 11:28, <rcojocaru@bitdefender.com> wrote:
> On 07/24/2018 11:55 AM, Jan Beulich wrote:
>>> +    if ( cpu_has_svm && !p2m->mem_access_settings )
>>> +    {
>>> +        p2m->mem_access_settings = xmalloc(struct radix_tree_root);
>>> +
>>> +        if( !p2m->mem_access_settings )
>> Style.
>> 
>>> +        {
>>> +            xfree(d->arch.monitor.msr_bitmap);
>>> +            return -ENOMEM;
>>> +        }
>>> +        radix_tree_init(p2m->mem_access_settings);
>>> +    }
>> What's the SVM connection here? Please don't forget that p2m-pt.c
>> also serves the shadow case. Perhaps struct p2m_domain should
>> contain a boolean indicator whether this auxiliary data structure is
>> needed?
> 
> Would it not work to simply check for "if ( cpu_has_svm &&
> p2m_is_hostp2m(p2m) && !p2m->mem_access_settings )" here?
> 
> In the shadow case, will not p2m->p2m_class be p2m_nested?

Maybe, but that wasn't the point of my remark. I want to
get rid of the cpu_has_svm here, not have it amended.

Jan



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

Re: [PATCH v4] x86/mm: Add mem access rights to NPT

[George Dunlap]

On 07/24/2018 09:55 AM, Jan Beulich wrote:
>>>> On 23.07.18 at 15:48, <aisaila@bitdefender.com> wrote:
>> --- 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;
> 
> Without explanation in the commit message and without comment
> this change is a no-go imo: I consider it at least questionable to
> have npfec_kind_with_gla without .gla_valid set to true. Perhaps
> it's just a naming issue, but that would then still require half a
> sentence to explain.

The naming here is confusing, but it seems to be based on the AMD manual
naming convention (IIRC from my skim through the manual last week).
"With gla" in this context means, "The nested fault happend while trying
to translate the final guest linear address of the access", as opposed
to "The nested fault happend while trying to translate one of the page
tables, before the guest linear address for the virtual address could be
calculated."  It's a perfectly valid setting on AMD box, in spite of the
fact that AMD doesn't report the virt -> gla translation.

I'd be in favor of renaming the variable, but that shouldn't be
Alexandru's job.

What about adding a comment like this:

"Naming is confusing here; 'with_gla' simply means the fault happened as
the result of a translating the final gla, as opposed to translating one
of the pagetables."

[snip]
>> +        {
>> +            xfree(d->arch.monitor.msr_bitmap);
>> +            return -ENOMEM;
>> +        }
>> +        radix_tree_init(p2m->mem_access_settings);
>> +    }
> 
> What's the SVM connection here? Please don't forget that p2m-pt.c
> also serves the shadow case. Perhaps struct p2m_domain should
> contain a boolean indicator whether this auxiliary data structure is
> needed?

It's basically just "hap_enabled()" isn't it?

 -George

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

Re: [PATCH v4] x86/mm: Add mem access rights to NPT

[Jan Beulich]

>>> On 24.07.18 at 13:26, <george.dunlap@citrix.com> wrote:
> On 07/24/2018 09:55 AM, Jan Beulich wrote:
>>>>> On 23.07.18 at 15:48, <aisaila@bitdefender.com> wrote:
>>> --- 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;
>> 
>> Without explanation in the commit message and without comment
>> this change is a no-go imo: I consider it at least questionable to
>> have npfec_kind_with_gla without .gla_valid set to true. Perhaps
>> it's just a naming issue, but that would then still require half a
>> sentence to explain.
> 
> The naming here is confusing, but it seems to be based on the AMD manual
> naming convention (IIRC from my skim through the manual last week).
> "With gla" in this context means, "The nested fault happend while trying
> to translate the final guest linear address of the access", as opposed
> to "The nested fault happend while trying to translate one of the page
> tables, before the guest linear address for the virtual address could be
> calculated."  It's a perfectly valid setting on AMD box, in spite of the
> fact that AMD doesn't report the virt -> gla translation.
> 
> I'd be in favor of renaming the variable, but that shouldn't be
> Alexandru's job.
> 
> What about adding a comment like this:
> 
> "Naming is confusing here; 'with_gla' simply means the fault happened as
> the result of a translating the final gla, as opposed to translating one
> of the pagetables."

Yes, that would clarify thnigs enough, I think.

>>> +        {
>>> +            xfree(d->arch.monitor.msr_bitmap);
>>> +            return -ENOMEM;
>>> +        }
>>> +        radix_tree_init(p2m->mem_access_settings);
>>> +    }
>> 
>> What's the SVM connection here? Please don't forget that p2m-pt.c
>> also serves the shadow case. Perhaps struct p2m_domain should
>> contain a boolean indicator whether this auxiliary data structure is
>> needed?
> 
> It's basically just "hap_enabled()" isn't it?

Only if we can't make it there when EPT is active.

Jan



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

Re: [PATCH v4] x86/mm: Add mem access rights to NPT

[Isaila Alexandru]

> > 
> > +static void p2m_set_access(struct p2m_domain *p2m, unsigned long
> > gfn,
> > +                                      p2m_access_t a)
> > +{
> > +    int rc;
> > +
> > +    if ( !p2m->mem_access_settings )
> > +        return;
> No error indication?

I would say ASSERT is a better choice if the code got this far and it
could not allocate memory

Alex

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

Re: [PATCH v4] x86/mm: Add mem access rights to NPT

[Jan Beulich]

>>> On 25.07.18 at 10:29, <aisaila@bitdefender.com> wrote:
>> > 
>> > +static void p2m_set_access(struct p2m_domain *p2m, unsigned long
>> > gfn,
>> > +                                      p2m_access_t a)
>> > +{
>> > +    int rc;
>> > +
>> > +    if ( !p2m->mem_access_settings )
>> > +        return;
>> No error indication?
> 
> I would say ASSERT is a better choice if the code got this far and it
> could not allocate memory

For one ASSERT() is a no-op in release builds. And then it is
extremely bad practices to bring down the host when an operation
targeting just a single guest has failed. You either return an error
indicator here (and pass it up the call tree), or if that's really
unfeasible then you crash the affected domain (we do so in quite
a few other situations). But you'd need to make clear (if it's not
obvious) why passing up an error is unacceptable here.

Jan



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

Re: [PATCH v4] x86/mm: Add mem access rights to NPT

[George Dunlap]

On 07/24/2018 01:02 PM, Jan Beulich wrote:
>>>> On 24.07.18 at 13:26, <george.dunlap@citrix.com> wrote:
>> On 07/24/2018 09:55 AM, Jan Beulich wrote:
>>>>>> On 23.07.18 at 15:48, <aisaila@bitdefender.com> wrote:
>>>> --- 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;
>>>
>>> Without explanation in the commit message and without comment
>>> this change is a no-go imo: I consider it at least questionable to
>>> have npfec_kind_with_gla without .gla_valid set to true. Perhaps
>>> it's just a naming issue, but that would then still require half a
>>> sentence to explain.
>>
>> The naming here is confusing, but it seems to be based on the AMD manual
>> naming convention (IIRC from my skim through the manual last week).
>> "With gla" in this context means, "The nested fault happend while trying
>> to translate the final guest linear address of the access", as opposed
>> to "The nested fault happend while trying to translate one of the page
>> tables, before the guest linear address for the virtual address could be
>> calculated."  It's a perfectly valid setting on AMD box, in spite of the
>> fact that AMD doesn't report the virt -> gla translation.
>>
>> I'd be in favor of renaming the variable, but that shouldn't be
>> Alexandru's job.
>>
>> What about adding a comment like this:
>>
>> "Naming is confusing here; 'with_gla' simply means the fault happened as
>> the result of a translating the final gla, as opposed to translating one
>> of the pagetables."
> 
> Yes, that would clarify thnigs enough, I think.
> 
>>>> +        {
>>>> +            xfree(d->arch.monitor.msr_bitmap);
>>>> +            return -ENOMEM;
>>>> +        }
>>>> +        radix_tree_init(p2m->mem_access_settings);
>>>> +    }
>>>
>>> What's the SVM connection here? Please don't forget that p2m-pt.c
>>> also serves the shadow case. Perhaps struct p2m_domain should
>>> contain a boolean indicator whether this auxiliary data structure is
>>> needed?
>>
>> It's basically just "hap_enabled()" isn't it?
> 
> Only if we can't make it there when EPT is active.

It can make it here when VMX is active and shadow is enabled, but it
shouldn't be able to get here when EPT is active.  We could add an
ASSERT() to that effect; it should be safe in production, as the only
side effect should be that we do a small pointless allocation.

 -George

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

Re: [PATCH v4] x86/mm: Add mem access rights to NPT

[Isaila Alexandru]

On Mi, 2018-07-25 at 03:14 -0600, Jan Beulich wrote:
> > 
> > > 
> > > > 
> > > > On 25.07.18 at 10:29, <aisaila@bitdefender.com> wrote:
> > > > 
> > > > +static void p2m_set_access(struct p2m_domain *p2m, unsigned
> > > > long
> > > > gfn,
> > > > +                                      p2m_access_t a)
> > > > +{
> > > > +    int rc;
> > > > +
> > > > +    if ( !p2m->mem_access_settings )
> > > > +        return;
> > > No error indication?
> > I would say ASSERT is a better choice if the code got this far and
> > it
> > could not allocate memory
> For one ASSERT() is a no-op in release builds. And then it is
> extremely bad practices to bring down the host when an operation
> targeting just a single guest has failed. You either return an error
> indicator here (and pass it up the call tree), or if that's really
> unfeasible then you crash the affected domain (we do so in quite
> a few other situations). But you'd need to make clear (if it's not
> obvious) why passing up an error is unacceptable here.
> 
By this time in the code the radix tree should be in place. If it is
not then the domain should crash because something is wrong and the mem
access feature will not function so passing the error up will have a
result of crashing the domain later after checking. 

I will add a domain crash here and a comment regarding it.

Thanks,
Alex

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

Re: [PATCH v4] x86/mm: Add mem access rights to NPT

[Jan Beulich]

>>> On 25.07.18 at 11:25, <george.dunlap@citrix.com> wrote:
> On 07/24/2018 01:02 PM, Jan Beulich wrote:
>>>>> On 24.07.18 at 13:26, <george.dunlap@citrix.com> wrote:
>>> On 07/24/2018 09:55 AM, Jan Beulich wrote:
>>>>>>> On 23.07.18 at 15:48, <aisaila@bitdefender.com> wrote:
>>>>> +        {
>>>>> +            xfree(d->arch.monitor.msr_bitmap);
>>>>> +            return -ENOMEM;
>>>>> +        }
>>>>> +        radix_tree_init(p2m->mem_access_settings);
>>>>> +    }
>>>>
>>>> What's the SVM connection here? Please don't forget that p2m-pt.c
>>>> also serves the shadow case. Perhaps struct p2m_domain should
>>>> contain a boolean indicator whether this auxiliary data structure is
>>>> needed?
>>>
>>> It's basically just "hap_enabled()" isn't it?
>> 
>> Only if we can't make it there when EPT is active.
> 
> It can make it here when VMX is active and shadow is enabled, but it
> shouldn't be able to get here when EPT is active.  We could add an
> ASSERT() to that effect; it should be safe in production, as the only
> side effect should be that we do a small pointless allocation.

So I've looked a little more closely: This is being added to
arch_monitor_init_domain(), called from vm_event_domctl(). I can't
see why this wouldn't be reachable with EPT enabled.

Jan



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

Re: [PATCH v4] x86/mm: Add mem access rights to NPT

[Isaila Alexandru]

On Mi, 2018-07-25 at 04:37 -0600, Jan Beulich wrote:
> > 
> > > 
> > > > 
> > > > On 25.07.18 at 11:25, <george.dunlap@citrix.com> wrote:
> > On 07/24/2018 01:02 PM, Jan Beulich wrote:
> > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > On 24.07.18 at 13:26, <george.dunlap@citrix.com> wrote:
> > > > On 07/24/2018 09:55 AM, Jan Beulich wrote:
> > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > > 
> > > > > > > > On 23.07.18 at 15:48, <aisaila@bitdefender.com> wrote:
> > > > > > +        {
> > > > > > +            xfree(d->arch.monitor.msr_bitmap);
> > > > > > +            return -ENOMEM;
> > > > > > +        }
> > > > > > +        radix_tree_init(p2m->mem_access_settings);
> > > > > > +    }
> > > > > What's the SVM connection here? Please don't forget that p2m-
> > > > > pt.c
> > > > > also serves the shadow case. Perhaps struct p2m_domain should
> > > > > contain a boolean indicator whether this auxiliary data
> > > > > structure is
> > > > > needed?
> > > > It's basically just "hap_enabled()" isn't it?
> > > Only if we can't make it there when EPT is active.
> > It can make it here when VMX is active and shadow is enabled, but
> > it
> > shouldn't be able to get here when EPT is active.  We could add an
> > ASSERT() to that effect; it should be safe in production, as the
> > only
> > side effect should be that we do a small pointless allocation.
> So I've looked a little more closely: This is being added to
> arch_monitor_init_domain(), called from vm_event_domctl(). I can't
> see why this wouldn't be reachable with EPT enabled.
> 
Hi George, 

Any thoughts on this?

Thanks,
Alex

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

Re: [PATCH v4] x86/mm: Add mem access rights to NPT

[Isaila Alexandru]

On Wed, 2018-07-25 at 04:37 -0600, Jan Beulich wrote:
> > > > On 25.07.18 at 11:25, <george.dunlap@citrix.com> wrote:
> > 
> > On 07/24/2018 01:02 PM, Jan Beulich wrote:
> > > > > > On 24.07.18 at 13:26, <george.dunlap@citrix.com> wrote:
> > > > 
> > > > On 07/24/2018 09:55 AM, Jan Beulich wrote:
> > > > > > > > On 23.07.18 at 15:48, <aisaila@bitdefender.com> wrote:
> > > > > > 
> > > > > > +        {
> > > > > > +            xfree(d->arch.monitor.msr_bitmap);
> > > > > > +            return -ENOMEM;
> > > > > > +        }
> > > > > > +        radix_tree_init(p2m->mem_access_settings);
> > > > > > +    }
> > > > > 
> > > > > What's the SVM connection here? Please don't forget that p2m-
> > > > > pt.c
> > > > > also serves the shadow case. Perhaps struct p2m_domain should
> > > > > contain a boolean indicator whether this auxiliary data
> > > > > structure is
> > > > > needed?
> > > > 
> > > > It's basically just "hap_enabled()" isn't it?
> > > 
> > > Only if we can't make it there when EPT is active.
> > 
> > It can make it here when VMX is active and shadow is enabled, but
> > it
> > shouldn't be able to get here when EPT is active.  We could add an
> > ASSERT() to that effect; it should be safe in production, as the
> > only
> > side effect should be that we do a small pointless allocation.
> 
> So I've looked a little more closely: This is being added to
> arch_monitor_init_domain(), called from vm_event_domctl(). I can't
> see why this wouldn't be reachable with EPT enabled.
> 
Hi George, 

Any input on this?

Regards, 
Alex


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

Re: [PATCH v4] x86/mm: Add mem access rights to NPT

[George Dunlap]

On 09/26/2018 09:17 AM, Isaila Alexandru wrote:
> On Wed, 2018-07-25 at 04:37 -0600, Jan Beulich wrote:
>>>>> On 25.07.18 at 11:25, <george.dunlap@citrix.com> wrote:
>>>
>>> On 07/24/2018 01:02 PM, Jan Beulich wrote:
>>>>>>> On 24.07.18 at 13:26, <george.dunlap@citrix.com> wrote:
>>>>>
>>>>> On 07/24/2018 09:55 AM, Jan Beulich wrote:
>>>>>>>>> On 23.07.18 at 15:48, <aisaila@bitdefender.com> wrote:
>>>>>>>
>>>>>>> +        {
>>>>>>> +            xfree(d->arch.monitor.msr_bitmap);
>>>>>>> +            return -ENOMEM;
>>>>>>> +        }
>>>>>>> +        radix_tree_init(p2m->mem_access_settings);
>>>>>>> +    }
>>>>>>
>>>>>> What's the SVM connection here? Please don't forget that p2m-
>>>>>> pt.c
>>>>>> also serves the shadow case. Perhaps struct p2m_domain should
>>>>>> contain a boolean indicator whether this auxiliary data
>>>>>> structure is
>>>>>> needed?
>>>>>
>>>>> It's basically just "hap_enabled()" isn't it?
>>>>
>>>> Only if we can't make it there when EPT is active.
>>>
>>> It can make it here when VMX is active and shadow is enabled, but
>>> it
>>> shouldn't be able to get here when EPT is active.  We could add an
>>> ASSERT() to that effect; it should be safe in production, as the
>>> only
>>> side effect should be that we do a small pointless allocation.
>>
>> So I've looked a little more closely: This is being added to
>> arch_monitor_init_domain(), called from vm_event_domctl(). I can't
>> see why this wouldn't be reachable with EPT enabled.
>>
> Hi George, 
> 
> Any input on this?

Sorry, you're still waiting for me to weigh in on whether you can get to
arch_monitor_init_domain() with EPT enabled?

The obvious answer is 'yes'; I hope you don't need me to tell you that. :-)

Going back through this conversation, I'm not 100% clear what I meant
with my "hap_enabled()" comment -- my best guess is that I was
suggesting the check be `( cpu_has_svm || !hap_enabled() )`.

But in any case, on the whole patch, I think that:

1) The feature is not abstracted well enough. mem_access.c should not
have to know whether additional storage is required or not; that should
be all taken care of within p2m-pt.c (or p2m-ept.c, should that ever
become necessary).

2) We've been talking for a long time about having a place to store
additional per-pfn information; it would be good if this mechanism were
made general enough to be used for other types of storage.

Let me play around with what you have and see if I can get a mock-up of
something like what I'm looking for.

 -George

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

Re: [PATCH v4] x86/mm: Add mem access rights to NPT

[George Dunlap]

On Mon, Jul 23, 2018 at 2:48 PM Alexandru Isaila
<aisaila@bitdefender.com> wrote:
>
> From: Isaila Alexandru <aisaila@bitdefender.com>
>
> 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@bitdefender.com>
>
> ---
> 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;
> +        }
>      }

BTW this introduces a bug -- if the check fails, this will fall
through into the gfn loop below, rather than returning the error.

> @@ -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;

Why did  you add in P2M_BASE_FLAGS here?

>      case p2m_grant_map_ro:
>          return flags | P2M_BASE_FLAGS | _PAGE_NX_BIT;

And is this `return` left here on purpose, or was it missed?

>  /* 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);

This is clearly wrong -- this isn't a leaf node, it's an intermediate
p2m table; and p2m_next_level() is just acting "under the covers",
filling in missing bits of the p2m table or breaking down superpages.
Since the access information is in a completely separate structure, it
shouldn't need to be modified here at all (indeed, it would be a bug
to do so).

But that does bring up an important issue -- it would appear that this
code is incorrect when setting superpages -- if we set a 2MiB page but
then read a non-2MiB-aligned entry within that page, we'll get the
default access rather than the one we set; same thing with splintering
a superpage into smaller pages.

There's a draft patch addressing this on the way.

 -George

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

Re: [PATCH v4] x86/mm: Add mem access rights to NPT

[George Dunlap]

[Resending]
On Wed, Sep 26, 2018 at 5:02 PM George Dunlap
<George.Dunlap@eu.citrix.com> wrote:
>
> On Mon, Jul 23, 2018 at 2:48 PM Alexandru Isaila
> <aisaila@bitdefender.com> wrote:
> >
> > From: Isaila Alexandru <aisaila@bitdefender.com>
> >
> > 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@bitdefender.com>
> >
> > ---
> > 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;
> > +        }
> >      }
>
> BTW this introduces a bug -- if the check fails, this will fall
> through into the gfn loop below, rather than returning the error.
>
> > @@ -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;
>
> Why did  you add in P2M_BASE_FLAGS here?
>
> >      case p2m_grant_map_ro:
> >          return flags | P2M_BASE_FLAGS | _PAGE_NX_BIT;
>
> And is this `return` left here on purpose, or was it missed?
>
> >  /* 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);
>
> This is clearly wrong -- this isn't a leaf node, it's an intermediate
> p2m table; and p2m_next_level() is just acting "under the covers",
> filling in missing bits of the p2m table or breaking down superpages.
> Since the access information is in a completely separate structure, it
> shouldn't need to be modified here at all (indeed, it would be a bug
> to do so).
>
> But that does bring up an important issue -- it would appear that this
> code is incorrect when setting superpages -- if we set a 2MiB page but
> then read a non-2MiB-aligned entry within that page, we'll get the
> default access rather than the one we set; same thing with splintering
> a superpage into smaller pages.
>
> There's a draft patch addressing this on the way.
>
>  -George

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