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

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

[Alexandru Stefan ISAILA]

This patch adds access control for NPT mode.

The access rights are stored in the NPT p2m table 56:53.
The bits are free after clearing the IOMMU flags [1].

Modify p2m_type_to_flags() to accept and interpret an access value,
parallel to the ept code.

Add a set_default_access() method to the p2m-pt and p2m-ept versions
of the p2m rather than setting it directly, to deal with different
default permitted access values.

[1] 854a49a7486a02edae5b3e53617bace526e9c1b1

Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>

---
Note: Tested with xen-access write/exec

Changes since V1:
	- Remove radix tree and store access bits in NPT p2m table.
---
 xen/arch/x86/mm/mem_access.c |   7 +--
 xen/arch/x86/mm/p2m-ept.c    |   9 +++
 xen/arch/x86/mm/p2m-pt.c     | 111 ++++++++++++++++++++++++++++++-----
 xen/include/asm-x86/p2m.h    |   2 +
 4 files changed, 109 insertions(+), 20 deletions(-)

diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index 0144f92b98..d8cc7a50de 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -374,10 +374,7 @@ 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;
-    }
+        return p2m->set_default_access(p2m, a);
 
     p2m_lock(p2m);
     if ( ap2m )
@@ -520,7 +517,7 @@ void arch_p2m_set_access_required(struct domain *d, bool access_required)
 
 bool p2m_mem_access_sanity_check(const struct domain *d)
 {
-    return is_hvm_domain(d) && cpu_has_vmx && hap_enabled(d);
+    return is_hvm_domain(d) && hap_enabled(d);
 }
 
 /*
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 6b8468c793..a6f2347bc1 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -624,6 +624,14 @@ bool_t ept_handle_misconfig(uint64_t gpa)
     return spurious ? (rc >= 0) : (rc > 0);
 }
 
+int ept_set_default_access(struct p2m_domain *p2m, p2m_access_t p2ma)
+{
+    /* All access is permitted */
+    p2m->default_access = p2ma;
+    
+    return 0;
+}
+
 /*
  * ept_set_entry() computes 'need_modify_vtd_table' for itself,
  * by observing whether any gfn->mfn translations are modified.
@@ -1222,6 +1230,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->set_default_access = ept_set_default_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 3a0a500d66..7f79eac979 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -63,10 +63,13 @@
 #define needs_recalc(level, ent) _needs_recalc(level##e_get_flags(ent))
 #define valid_recalc(level, ent) (!(level##e_get_flags(ent) & _PAGE_ACCESSED))
 
+#define _PAGE_ACCESS_BITS  (0x1e00) /* mem_access rights 16:13 */
+
 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 = (unsigned long)(t & 0x7f) << 12;
 
@@ -79,23 +82,28 @@ 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 |= _PAGE_NX_BIT;
+        break;
     case p2m_grant_map_ro:
-        return flags | P2M_BASE_FLAGS | _PAGE_NX_BIT;
+        flags |= P2M_BASE_FLAGS | _PAGE_NX_BIT;
+        break;
     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;
@@ -104,8 +112,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;
 }
 
 
@@ -152,6 +184,17 @@ p2m_free_entry(struct p2m_domain *p2m, l1_pgentry_t *p2m_entry, int page_order)
     p2m_free_ptp(p2m, l1e_get_page(*p2m_entry));
 }
 
+static void p2m_set_access(intpte_t *entry, p2m_access_t access)
+{
+    *entry |= put_pte_flags((get_pte_flags(*entry) & ~_PAGE_ACCESS_BITS) |
+                            (MASK_INSR(access, _PAGE_ACCESS_BITS)));
+}
+
+static p2m_access_t p2m_get_access(intpte_t entry)
+{
+    return (p2m_access_t)(MASK_EXTR(get_pte_flags(entry), _PAGE_ACCESS_BITS));
+}
+
 // Walk one level of the P2M table, allocating a new table if required.
 // Returns 0 on error.
 //
@@ -226,6 +269,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(&new_entry.l1, p2m->default_access);
             rc = p2m->write_p2m_entry(p2m, gfn, l1_entry + i, new_entry, level);
             if ( rc )
             {
@@ -237,6 +281,7 @@ p2m_next_level(struct p2m_domain *p2m, void **table,
         unmap_domain_page(l1_entry);
 
         new_entry = l1e_from_mfn(mfn, P2M_BASE_FLAGS | _PAGE_RW);
+        p2m_set_access(&new_entry.l1, p2m->default_access);
         rc = p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry,
                                   level + 1);
         if ( rc )
@@ -425,8 +470,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(e.l1);
             unsigned long flags = p2m_type_to_flags(p2m, nt,
-                                                    _mfn(mfn), level);
+                                                    _mfn(mfn), level, a);
 
             if ( level )
             {
@@ -474,6 +520,32 @@ int p2m_pt_handle_deferred_changes(uint64_t gpa)
     return rc;
 }
 
+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;
+}
+
+static int p2m_pt_set_default_access(struct p2m_domain *p2m,
+                                     p2m_access_t p2ma)
+{
+    int rc = p2m_pt_check_access(p2ma);
+
+    if ( !rc )
+        p2m->default_access = p2ma;
+
+    return rc;
+}
+
 /* Checks only applicable to entries with order > PAGE_ORDER_4K */
 static void check_entry(mfn_t mfn, p2m_type_t new, p2m_type_t old,
                         unsigned int order)
@@ -514,6 +586,9 @@ p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn,
     if ( !sve )
         return -EOPNOTSUPP;
 
+    if ( (rc = p2m_pt_check_access(p2ma)) )
+        return rc;
+
     if ( tb_init_done )
     {
         struct {
@@ -564,10 +639,11 @@ p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn,
         check_entry(mfn, p2mt, p2m_flags_to_type(flags), page_order);
         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;
 
+        p2m_set_access(&entry_content.l1, p2ma);
         rc = p2m->write_p2m_entry(p2m, gfn, p2m_entry, entry_content, 3);
         /* NB: paging_write_p2m_entry() handles tlb flushes properly */
         if ( rc )
@@ -597,10 +673,11 @@ 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();
 
+        p2m_set_access(&entry_content.l1, p2ma);
         /* level 1 entry */
         rc = p2m->write_p2m_entry(p2m, gfn, p2m_entry, entry_content, 1);
         /* NB: paging_write_p2m_entry() handles tlb flushes properly */
@@ -625,10 +702,11 @@ p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn,
         check_entry(mfn, p2mt, p2m_flags_to_type(flags), page_order);
         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;
 
+        p2m_set_access(&entry_content.l1, p2ma);
         rc = p2m->write_p2m_entry(p2m, gfn, p2m_entry, entry_content, 2);
         /* NB: paging_write_p2m_entry() handles tlb flushes properly */
         if ( rc )
@@ -676,8 +754,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 )
     {
@@ -740,6 +817,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(l3e->l3);
             unmap_domain_page(l3e);
 
             ASSERT(mfn_valid(mfn) || !p2m_is_ram(*t));
@@ -779,6 +857,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(l2e->l2);
         unmap_domain_page(l2e);
         
         ASSERT(mfn_valid(mfn) || !p2m_is_ram(*t));
@@ -815,6 +894,7 @@ pod_retry_l1:
     }
     mfn = l1e_get_mfn(*l1e);
     *t = p2m_recalc_type(recalc || _needs_recalc(flags), l1t, p2m, gfn);
+    *a = p2m_get_access(l1e->l1);
     unmap_domain_page(l1e);
 
     ASSERT(mfn_valid(mfn) || !p2m_is_any_ram(*t) || p2m_is_paging(*t));
@@ -1063,6 +1143,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->set_default_access = p2m_pt_set_default_access;
 #if P2M_AUDIT
     p2m->audit_p2m = p2m_pt_audit_p2m;
 #else
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 94285db1b4..32e71067b7 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -269,6 +269,8 @@ 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                (*set_default_access)(struct p2m_domain *p2m,
+                                             p2m_access_t p2ma);
 
     /*
      * P2M updates may require TLBs to be flushed (invalidated).
-- 
2.17.1


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

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

[Jan Beulich]

On 22.08.2019 16:02, Alexandru Stefan ISAILA wrote:
> This patch adds access control for NPT mode.
> 
> The access rights are stored in the NPT p2m table 56:53.

Why starting from bit 53? I can't seem to find any use of bit 52.

> The bits are free after clearing the IOMMU flags [1].
> 
> Modify p2m_type_to_flags() to accept and interpret an access value,
> parallel to the ept code.
> 
> Add a set_default_access() method to the p2m-pt and p2m-ept versions
> of the p2m rather than setting it directly, to deal with different
> default permitted access values.

I think this would better be a separate change.

> --- a/xen/arch/x86/mm/p2m-pt.c
> +++ b/xen/arch/x86/mm/p2m-pt.c
> @@ -63,10 +63,13 @@
>  #define needs_recalc(level, ent) _needs_recalc(level##e_get_flags(ent))
>  #define valid_recalc(level, ent) (!(level##e_get_flags(ent) & _PAGE_ACCESSED))
>  
> +#define _PAGE_ACCESS_BITS  (0x1e00) /* mem_access rights 16:13 */

I guess this is too disconnected from the two page.h headers where
the correlation between bit positions gets explained, so I guess
you want to extend the comment and either refer there, or replicate
some of it to make understandable why 16:13 matches 56:53.

I'm also concerned how easy it'll be for someone to find this
definition when wanting to use other of the available bits.

> @@ -104,8 +112,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;

I can't seem to be able to follow you here. In fact I don't see
how you would be able to express execute-only with NPT. If this
is really needed for some reason, then a justifying comment
should be added.

> @@ -152,6 +184,17 @@ p2m_free_entry(struct p2m_domain *p2m, l1_pgentry_t *p2m_entry, int page_order)
>      p2m_free_ptp(p2m, l1e_get_page(*p2m_entry));
>  }
>  
> +static void p2m_set_access(intpte_t *entry, p2m_access_t access)
> +{
> +    *entry |= put_pte_flags((get_pte_flags(*entry) & ~_PAGE_ACCESS_BITS) |
> +                            (MASK_INSR(access, _PAGE_ACCESS_BITS)));
> +}
> +
> +static p2m_access_t p2m_get_access(intpte_t entry)
> +{
> +    return (p2m_access_t)(MASK_EXTR(get_pte_flags(entry), _PAGE_ACCESS_BITS));

Is the cast really needed here?

> @@ -226,6 +269,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(&new_entry.l1, p2m->default_access);
>              rc = p2m->write_p2m_entry(p2m, gfn, l1_entry + i, new_entry, level);
>              if ( rc )
>              {
> @@ -237,6 +281,7 @@ p2m_next_level(struct p2m_domain *p2m, void **table,
>          unmap_domain_page(l1_entry);
>  
>          new_entry = l1e_from_mfn(mfn, P2M_BASE_FLAGS | _PAGE_RW);
> +        p2m_set_access(&new_entry.l1, p2m->default_access);
>          rc = p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry,
>                                    level + 1);
>          if ( rc )

Is it really intended to insert the access bits also into non-leaf
entries (which may be what is being processed here)? (May also
apply further down.)

> @@ -474,6 +520,32 @@ int p2m_pt_handle_deferred_changes(uint64_t gpa)
>      return rc;
>  }
>  
> +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;

I'm not convinced EINVAL is appropriate here - the argument isn't
invalid, it's just that there's no way to represent it.

Jan

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

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

[Alexandru Stefan ISAILA]

On 29.08.2019 18:04, Jan Beulich wrote:
> On 22.08.2019 16:02, Alexandru Stefan ISAILA wrote:
>> This patch adds access control for NPT mode.
>>
>> The access rights are stored in the NPT p2m table 56:53.
> 
> Why starting from bit 53? I can't seem to find any use of bit 52.

There is a comment in page.h that warns that bit 12(52) is taken.
"/*
  * Bit 12 of a 24-bit flag mask. This corresponds to bit 52 of a pte.
  * This is needed to distinguish between user and kernel PTEs since 
_PAGE_USER
  * is asserted for both.
  */
#define _PAGE_GUEST_KERNEL (1U<<12)
"

> 
>> The bits are free after clearing the IOMMU flags [1].
>>
>> Modify p2m_type_to_flags() to accept and interpret an access value,
>> parallel to the ept code.
>>
>> Add a set_default_access() method to the p2m-pt and p2m-ept versions
>> of the p2m rather than setting it directly, to deal with different
>> default permitted access values.
> 
> I think this would better be a separate change.

Ok I will brake the patch in two parts. I was following George's v1 patch.

> 
>> --- a/xen/arch/x86/mm/p2m-pt.c
>> +++ b/xen/arch/x86/mm/p2m-pt.c
>> @@ -63,10 +63,13 @@
>>   #define needs_recalc(level, ent) _needs_recalc(level##e_get_flags(ent))
>>   #define valid_recalc(level, ent) (!(level##e_get_flags(ent) & _PAGE_ACCESSED))
>>   
>> +#define _PAGE_ACCESS_BITS  (0x1e00) /* mem_access rights 16:13 */
> 
> I guess this is too disconnected from the two page.h headers where
> the correlation between bit positions gets explained, so I guess
> you want to extend the comment and either refer there, or replicate
> some of it to make understandable why 16:13 matches 56:53.
> 

I will extend the comment so as the bit shifting will be clear.

> I'm also concerned how easy it'll be for someone to find this
> definition when wanting to use other of the available bits.

Yes you are right, I will add a comment in page.h that bits 56:53 are 
used for memory access rights on SVM.

> 
>> @@ -104,8 +112,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;
> 
> I can't seem to be able to follow you here. In fact I don't see
> how you would be able to express execute-only with NPT. If this
> is really needed for some reason, then a justifying comment
> should be added.

Execute-only should be expressed as not PAGE_RW and PAGE_NX_BIT not set.

> 
>> @@ -152,6 +184,17 @@ p2m_free_entry(struct p2m_domain *p2m, l1_pgentry_t *p2m_entry, int page_order)
>>       p2m_free_ptp(p2m, l1e_get_page(*p2m_entry));
>>   }
>>   
>> +static void p2m_set_access(intpte_t *entry, p2m_access_t access)
>> +{
>> +    *entry |= put_pte_flags((get_pte_flags(*entry) & ~_PAGE_ACCESS_BITS) |
>> +                            (MASK_INSR(access, _PAGE_ACCESS_BITS)));
>> +}
>> +
>> +static p2m_access_t p2m_get_access(intpte_t entry)
>> +{
>> +    return (p2m_access_t)(MASK_EXTR(get_pte_flags(entry), _PAGE_ACCESS_BITS));
> 
> Is the cast really needed here?

Not really, I can remove it in the next version.

> 
>> @@ -226,6 +269,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(&new_entry.l1, p2m->default_access);
>>               rc = p2m->write_p2m_entry(p2m, gfn, l1_entry + i, new_entry, level);
>>               if ( rc )
>>               {
>> @@ -237,6 +281,7 @@ p2m_next_level(struct p2m_domain *p2m, void **table,
>>           unmap_domain_page(l1_entry);
>>   
>>           new_entry = l1e_from_mfn(mfn, P2M_BASE_FLAGS | _PAGE_RW);
>> +        p2m_set_access(&new_entry.l1, p2m->default_access);
>>           rc = p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry,
>>                                     level + 1);
>>           if ( rc )
> 
> Is it really intended to insert the access bits also into non-leaf
> entries (which may be what is being processed here)? (May also
> apply further down.)
> 
>> @@ -474,6 +520,32 @@ int p2m_pt_handle_deferred_changes(uint64_t gpa)
>>       return rc;
>>   }
>>   
>> +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;
> 
> I'm not convinced EINVAL is appropriate here - the argument isn't
> invalid, it's just that there's no way to represent it.

Would EPERM be a better return here?

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

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

[Jan Beulich]

On 02.09.2019 13:23, Alexandru Stefan ISAILA wrote:
> On 29.08.2019 18:04, Jan Beulich wrote:
>> On 22.08.2019 16:02, Alexandru Stefan ISAILA wrote:
>>> This patch adds access control for NPT mode.
>>>
>>> The access rights are stored in the NPT p2m table 56:53.
>>
>> Why starting from bit 53? I can't seem to find any use of bit 52.
> 
> There is a comment in page.h that warns that bit 12(52) is taken.
> "/*
>   * Bit 12 of a 24-bit flag mask. This corresponds to bit 52 of a pte.
>   * This is needed to distinguish between user and kernel PTEs since 
> _PAGE_USER
>   * is asserted for both.
>   */
> #define _PAGE_GUEST_KERNEL (1U<<12)
> "

But that's a PV-only thing. With sufficient care it should be
possible to have overlapping uses. And given that the available
bit are a pretty limited resource, I'd very much appreciate if
you at least tried to make this work.

>>> @@ -104,8 +112,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;
>>
>> I can't seem to be able to follow you here. In fact I don't see
>> how you would be able to express execute-only with NPT. If this
>> is really needed for some reason, then a justifying comment
>> should be added.
> 
> Execute-only should be expressed as not PAGE_RW and PAGE_NX_BIT not set.

But that still doesn't yield exec-only. Where is this "should be
expressed" stated? I.e. on what basis is it tolerable to also allow
read access despite a request to the contrary?

>>> @@ -474,6 +520,32 @@ int p2m_pt_handle_deferred_changes(uint64_t gpa)
>>>       return rc;
>>>   }
>>>   
>>> +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;
>>
>> I'm not convinced EINVAL is appropriate here - the argument isn't
>> invalid, it's just that there's no way to represent it.
> 
> Would EPERM be a better return here?

Quite a bit better, yes. But still not optimal, but I confess that
I also can't find an optimal one. EDOM would look to be suitable too,
if one was to ignore the "math" aspect of it.

Jan

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