* [PATCH v3] x86/mm: Add mem access rights to NPT
@ 2018-07-02 12:42 Alexandru Isaila
2018-07-17 12:59 ` PING: " Isaila Alexandru
2018-07-18 15:33 ` George Dunlap
0 siblings, 2 replies; 14+ messages in thread
From: Alexandru Isaila @ 2018-07-02 12:42 UTC (permalink / raw)
To: xen-devel
Cc: tamas, rcojocaru, george.dunlap, andrew.cooper3, jbeulich,
Isaila Alexandru
From: Isaila Alexandru <aisaila@bitdefender.com>
This patch adds access rights for the NPT pages. The access rights are
saved in a radix tree with the root saved in p2m_domain. The rights are manipulated through p2m_set_access()
and p2m_get_access() functions.
The patch follows the ept logic.
Note: It was tested with xen-access write
Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
---
Changes since V2:
- Delete blak line
- Add return if p2m_access_rwx = a
- Delete the comment from p2m_pt_get_entry()
- Moved radix_tree_init() to arch_monitor_init_domain().
---
xen/arch/x86/mm/mem_access.c | 3 ++
xen/arch/x86/mm/p2m-pt.c | 109 ++++++++++++++++++++++++++++++++++-----
xen/arch/x86/mm/p2m.c | 6 +++
xen/arch/x86/monitor.c | 13 +++++
xen/include/asm-x86/mem_access.h | 2 +-
xen/include/asm-x86/p2m.h | 6 +++
6 files changed, 124 insertions(+), 15 deletions(-)
diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index c0cd017..d78c82c 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -221,7 +221,10 @@ 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.gla_valid || cpu_has_svm )
+ {
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 )
diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index b8c5d2e..4330d1f 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,37 @@ 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:
+ case p2m_access_w:
+ flags |= _PAGE_NX_BIT;
+ flags &= ~_PAGE_RW;
+ break;
+ case p2m_access_rw:
+ flags |= _PAGE_NX_BIT;
+ break;
+ case p2m_access_n:
+ case p2m_access_n2rwx:
+ flags |= _PAGE_NX_BIT;
+ flags &= ~_PAGE_RW;
+ break;
+ case p2m_access_rx:
+ case p2m_access_wx:
+ 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 +208,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 +273,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 +322,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 +330,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 +495,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 )
{
@@ -569,13 +645,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 +685,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 +707,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 +739,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 +828,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 +891,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 +931,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 +968,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));
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..18b88a1 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,16 @@ 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 )
+ 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..a23300a 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -288,6 +288,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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* PING: [PATCH v3] x86/mm: Add mem access rights to NPT
2018-07-02 12:42 [PATCH v3] x86/mm: Add mem access rights to NPT Alexandru Isaila
@ 2018-07-17 12:59 ` Isaila Alexandru
2018-07-18 15:33 ` George Dunlap
1 sibling, 0 replies; 14+ messages in thread
From: Isaila Alexandru @ 2018-07-17 12:59 UTC (permalink / raw)
To: xen-devel; +Cc: george.dunlap, andrew.cooper3, tamas, jbeulich, rcojocaru
Any thoughts on this patch are appreciated.
Thanks,
Alex
On Lu, 2018-07-02 at 15:42 +0300, Alexandru Isaila wrote:
> From: Isaila Alexandru <aisaila@bitdefender.com>
>
> This patch adds access rights for the NPT pages. The access rights
> are
> saved in a radix tree with the root saved in p2m_domain. The rights
> are manipulated through p2m_set_access()
> and p2m_get_access() functions.
> The patch follows the ept logic.
>
> Note: It was tested with xen-access write
>
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
>
> ---
> Changes since V2:
> - Delete blak line
> - Add return if p2m_access_rwx = a
> - Delete the comment from p2m_pt_get_entry()
> - Moved radix_tree_init() to arch_monitor_init_domain().
> ---
> xen/arch/x86/mm/mem_access.c | 3 ++
> xen/arch/x86/mm/p2m-pt.c | 109
> ++++++++++++++++++++++++++++++++++-----
> xen/arch/x86/mm/p2m.c | 6 +++
> xen/arch/x86/monitor.c | 13 +++++
> xen/include/asm-x86/mem_access.h | 2 +-
> xen/include/asm-x86/p2m.h | 6 +++
> 6 files changed, 124 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/x86/mm/mem_access.c
> b/xen/arch/x86/mm/mem_access.c
> index c0cd017..d78c82c 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -221,7 +221,10 @@ 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.gla_valid || cpu_has_svm )
> + {
> 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 )
> diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
> index b8c5d2e..4330d1f 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,37 @@ 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:
> + case p2m_access_w:
> + flags |= _PAGE_NX_BIT;
> + flags &= ~_PAGE_RW;
> + break;
> + case p2m_access_rw:
> + flags |= _PAGE_NX_BIT;
> + break;
> + case p2m_access_n:
> + case p2m_access_n2rwx:
> + flags |= _PAGE_NX_BIT;
> + flags &= ~_PAGE_RW;
> + break;
> + case p2m_access_rx:
> + case p2m_access_wx:
> + 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 +208,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 +273,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 +322,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 +330,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 +495,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 )
> {
> @@ -569,13 +645,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 +685,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 +707,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 +739,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 +828,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 +891,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 +931,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 +968,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));
> 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..18b88a1 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,16 @@ 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 )
> + 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..a23300a 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -288,6 +288,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;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] x86/mm: Add mem access rights to NPT
2018-07-02 12:42 [PATCH v3] x86/mm: Add mem access rights to NPT Alexandru Isaila
2018-07-17 12:59 ` PING: " Isaila Alexandru
@ 2018-07-18 15:33 ` George Dunlap
2018-07-19 8:18 ` Isaila Alexandru
1 sibling, 1 reply; 14+ messages in thread
From: George Dunlap @ 2018-07-18 15:33 UTC (permalink / raw)
To: Alexandru Isaila
Cc: tamas, rcojocaru, Andrew Cooper, George Dunlap, xen-devel, jbeulich
> On Jul 2, 2018, at 8:42 AM, Alexandru Isaila <aisaila@bitdefender.com> wrote:
>
> From: Isaila Alexandru <aisaila@bitdefender.com>
>
> This patch adds access rights for the NPT pages. The access rights are
> saved in a radix tree with the root saved in p2m_domain. The rights are manipulated through p2m_set_access()
> and p2m_get_access() functions.
> The patch follows the ept logic.
This description needs to be much more complete. Something like this:
---
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 V2:
> - Delete blak line
> - Add return if p2m_access_rwx = a
> - Delete the comment from p2m_pt_get_entry()
> - Moved radix_tree_init() to arch_monitor_init_domain().
> ---
> xen/arch/x86/mm/mem_access.c | 3 ++
> xen/arch/x86/mm/p2m-pt.c | 109 ++++++++++++++++++++++++++++++++++-----
> xen/arch/x86/mm/p2m.c | 6 +++
> xen/arch/x86/monitor.c | 13 +++++
> xen/include/asm-x86/mem_access.h | 2 +-
> xen/include/asm-x86/p2m.h | 6 +++
> 6 files changed, 124 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index c0cd017..d78c82c 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -221,7 +221,10 @@ 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.gla_valid || cpu_has_svm )
> + {
I can’t immediately tell what this is about, which means it needs a comment.
It may also be (depending on why this is here) that “cpu_has_svm” makes this more fragile — if this is really about having a radix tree, for instance, then you should probably check for a radix tree.
> 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 )
> diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
> index b8c5d2e..4330d1f 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,37 @@ 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;
> + }
I think you want a blank line here.
> + switch ( access )
> + {
> + case p2m_access_r:
> + case p2m_access_w:
> + flags |= _PAGE_NX_BIT;
> + flags &= ~_PAGE_RW;
> + break;
> + case p2m_access_rw:
> + flags |= _PAGE_NX_BIT;
> + break;
> + case p2m_access_n:
> + case p2m_access_n2rwx:
> + flags |= _PAGE_NX_BIT;
> + flags &= ~_PAGE_RW;
> + break;
> + case p2m_access_rx:
> + case p2m_access_wx:
> + case p2m_access_rx2rw:
> + flags &= ~(_PAGE_NX_BIT | _PAGE_RW);
This looks like a mistake — this will unconditionally enable execution, even if the underlying p2m type forbids it. ept_p2m_type_to_flags() doesn’t do that.
> + break;
> + case p2m_access_x:
> + flags &= ~_PAGE_RW;
> + break;
> + case p2m_access_rwx:
> + default:
> + break;
> }
I think you want another blank line here too.
Also, this doesn’t seem to capture the ‘r’ part of the equation — shouldn’t p2m_access_n end up with a not-present p2m entry?
> + return flags;
> }
>
>
> @@ -174,6 +208,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 +273,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 +322,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 +330,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 +495,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 )
> {
> @@ -569,13 +645,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 +685,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 +707,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 +739,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 +828,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 +891,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 +931,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 +968,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));
> 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..18b88a1 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,16 @@ 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 )
> + return -ENOMEM;
This will leak d->arch.monitor.msr_bitmap. You need to use the `goto out_free:` pattern.
Everything else looks OK.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] x86/mm: Add mem access rights to NPT
2018-07-18 15:33 ` George Dunlap
@ 2018-07-19 8:18 ` Isaila Alexandru
2018-07-19 8:20 ` Razvan Cojocaru
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Isaila Alexandru @ 2018-07-19 8:18 UTC (permalink / raw)
To: George Dunlap; +Cc: Andrew Cooper, tamas, jbeulich, rcojocaru, xen-devel
On Mi, 2018-07-18 at 15:33 +0000, George Dunlap wrote:
>
> >
> > On Jul 2, 2018, at 8:42 AM, Alexandru Isaila <aisaila@bitdefender.c
> > om> wrote:
> >
> > From: Isaila Alexandru <aisaila@bitdefender.com>
> >
> > This patch adds access rights for the NPT pages. The access rights
> > are
> > saved in a radix tree with the root saved in p2m_domain. The rights
> > are manipulated through p2m_set_access()
> > and p2m_get_access() functions.
> > The patch follows the ept logic.
> This description needs to be much more complete. Something like
> this:
>
> ---
> 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.
> ---
>
I will update the patch description.
> >
> >
> > Note: It was tested with xen-access write
> >
> > Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
>
> >
> >
> > ---
> > Changes since V2:
> > - Delete blak line
> > - Add return if p2m_access_rwx = a
> > - Delete the comment from p2m_pt_get_entry()
> > - Moved radix_tree_init() to arch_monitor_init_domain().
> > ---
> > xen/arch/x86/mm/mem_access.c | 3 ++
> > xen/arch/x86/mm/p2m-pt.c | 109
> > ++++++++++++++++++++++++++++++++++-----
> > xen/arch/x86/mm/p2m.c | 6 +++
> > xen/arch/x86/monitor.c | 13 +++++
> > xen/include/asm-x86/mem_access.h | 2 +-
> > xen/include/asm-x86/p2m.h | 6 +++
> > 6 files changed, 124 insertions(+), 15 deletions(-)
> >
> > diff --git a/xen/arch/x86/mm/mem_access.c
> > b/xen/arch/x86/mm/mem_access.c
> > index c0cd017..d78c82c 100644
> > --- a/xen/arch/x86/mm/mem_access.c
> > +++ b/xen/arch/x86/mm/mem_access.c
> > @@ -221,7 +221,10 @@ 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.gla_valid || cpu_has_svm )
> > + {
> I can’t immediately tell what this is about, which means it needs a
> comment.
>
> It may also be (depending on why this is here) that “cpu_has_svm”
> makes this more fragile — if this is really about having a radix
> tree, for instance, then you should probably check for a radix tree.
This is about the different npfec on SVN. The gla in never valid so the
fault flag will not be set.
>
> >
> > 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 )
> > diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
> > index b8c5d2e..4330d1f 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,37 @@ 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;
> > + }
> I think you want a blank line here.
>
> >
> > + switch ( access )
> > + {
> > + case p2m_access_r:
> > + case p2m_access_w:
> > + flags |= _PAGE_NX_BIT;
> > + flags &= ~_PAGE_RW;
> > + break;
> > + case p2m_access_rw:
> > + flags |= _PAGE_NX_BIT;
> > + break;
> > + case p2m_access_n:
> > + case p2m_access_n2rwx:
> > + flags |= _PAGE_NX_BIT;
> > + flags &= ~_PAGE_RW;
> > + break;
> > + case p2m_access_rx:
> > + case p2m_access_wx:
> > + case p2m_access_rx2rw:
> > + flags &= ~(_PAGE_NX_BIT | _PAGE_RW);
> This looks like a mistake — this will unconditionally enable
> execution, even if the underlying p2m type forbids it.
> ept_p2m_type_to_flags() doesn’t do that.
>
> >
> > + break;
> > + case p2m_access_x:
> > + flags &= ~_PAGE_RW;
> > + break;
> > + case p2m_access_rwx:
> > + default:
> > + break;
> > }
> I think you want another blank line here too.
>
> Also, this doesn’t seem to capture the ‘r’ part of the equation —
> shouldn’t p2m_access_n end up with a not-present p2m entry?
SVM dosen't explicitly provide a read access bit so we treat read and
write the same way.
Alex
>
> >
> > + return flags;
> > }
> >
> >
> > @@ -174,6 +208,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 +273,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 +322,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 +330,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 +495,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 )
> > {
> > @@ -569,13 +645,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 +685,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 +707,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 +739,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 +828,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 +891,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 +931,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 +968,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));
> > 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..18b88a1 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,16 @@ 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 )
> > + return -ENOMEM;
> This will leak d->arch.monitor.msr_bitmap. You need to use the `goto
> out_free:` pattern.
>
> Everything else looks OK.
>
> -George
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] x86/mm: Add mem access rights to NPT
2018-07-19 8:18 ` Isaila Alexandru
@ 2018-07-19 8:20 ` Razvan Cojocaru
2018-07-19 8:30 ` Jan Beulich
2018-07-20 10:05 ` George Dunlap
2 siblings, 0 replies; 14+ messages in thread
From: Razvan Cojocaru @ 2018-07-19 8:20 UTC (permalink / raw)
To: Isaila Alexandru, George Dunlap; +Cc: Andrew Cooper, tamas, jbeulich, xen-devel
On 07/19/2018 11:18 AM, Isaila Alexandru wrote:
> On Mi, 2018-07-18 at 15:33 +0000, George Dunlap wrote:
>>
>>>
>>> On Jul 2, 2018, at 8:42 AM, Alexandru Isaila <aisaila@bitdefender.c
>>> om> wrote:
>>>
>>> From: Isaila Alexandru <aisaila@bitdefender.com>
>>>
>>> This patch adds access rights for the NPT pages. The access rights
>>> are
>>> saved in a radix tree with the root saved in p2m_domain. The rights
>>> are manipulated through p2m_set_access()
>>> and p2m_get_access() functions.
>>> The patch follows the ept logic.
>> This description needs to be much more complete. Something like
>> this:
>>
>> ---
>> 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.
>> ---
>>
> I will update the patch description.
>>>
>>>
>>> Note: It was tested with xen-access write
>>>
>>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
>>
>>>
>>>
>>> ---
>>> Changes since V2:
>>> - Delete blak line
>>> - Add return if p2m_access_rwx = a
>>> - Delete the comment from p2m_pt_get_entry()
>>> - Moved radix_tree_init() to arch_monitor_init_domain().
>>> ---
>>> xen/arch/x86/mm/mem_access.c | 3 ++
>>> xen/arch/x86/mm/p2m-pt.c | 109
>>> ++++++++++++++++++++++++++++++++++-----
>>> xen/arch/x86/mm/p2m.c | 6 +++
>>> xen/arch/x86/monitor.c | 13 +++++
>>> xen/include/asm-x86/mem_access.h | 2 +-
>>> xen/include/asm-x86/p2m.h | 6 +++
>>> 6 files changed, 124 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/mm/mem_access.c
>>> b/xen/arch/x86/mm/mem_access.c
>>> index c0cd017..d78c82c 100644
>>> --- a/xen/arch/x86/mm/mem_access.c
>>> +++ b/xen/arch/x86/mm/mem_access.c
>>> @@ -221,7 +221,10 @@ 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.gla_valid || cpu_has_svm )
>>> + {
>> I can’t immediately tell what this is about, which means it needs a
>> comment.
>>
>> It may also be (depending on why this is here) that “cpu_has_svm”
>> makes this more fragile — if this is really about having a radix
>> tree, for instance, then you should probably check for a radix tree.
>
> This is about the different npfec on SVN. The gla in never valid so the
> fault flag will not be set.
Specifically, this is what svm_do_nested_pgfault() does:
1781 struct npfec npfec = {
1782 .read_access = !(pfec & PFEC_insn_fetch),
1783 .write_access = !!(pfec & PFEC_write_access),
1784 .insn_fetch = !!(pfec & PFEC_insn_fetch),
1785 .present = !!(pfec & PFEC_page_present),
1786 };
1787
1788 /* These bits are mutually exclusive */
1789 if ( pfec & NPT_PFEC_with_gla )
1790 npfec.kind = npfec_kind_with_gla;
1791 else if ( pfec & NPT_PFEC_in_gpt )
1792 npfec.kind = npfec_kind_in_gpt;
1793
1794 ret = hvm_hap_nested_page_fault(gpa, ~0ul, npfec);
so gla_valid is never non-0 on SVM.
Thanks,
Razvan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] x86/mm: Add mem access rights to NPT
2018-07-19 8:18 ` Isaila Alexandru
2018-07-19 8:20 ` Razvan Cojocaru
@ 2018-07-19 8:30 ` Jan Beulich
2018-07-19 8:43 ` Razvan Cojocaru
2018-07-19 15:08 ` Tamas K Lengyel
2018-07-20 10:05 ` George Dunlap
2 siblings, 2 replies; 14+ messages in thread
From: Jan Beulich @ 2018-07-19 8:30 UTC (permalink / raw)
To: aisaila; +Cc: Andrew Cooper, tamas, george.dunlap, Razvan Cojocaru, xen-devel
>>> On 19.07.18 at 10:18, <aisaila@bitdefender.com> wrote:
> On Mi, 2018-07-18 at 15:33 +0000, George Dunlap wrote:
>> > On Jul 2, 2018, at 8:42 AM, Alexandru Isaila <aisaila@bitdefender.c
>> > @@ -112,8 +117,37 @@ 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;
>> > + }
>> I think you want a blank line here.
>>
>> >
>> > + switch ( access )
>> > + {
>> > + case p2m_access_r:
>> > + case p2m_access_w:
>> > + flags |= _PAGE_NX_BIT;
>> > + flags &= ~_PAGE_RW;
>> > + break;
>> > + case p2m_access_rw:
>> > + flags |= _PAGE_NX_BIT;
>> > + break;
>> > + case p2m_access_n:
>> > + case p2m_access_n2rwx:
>> > + flags |= _PAGE_NX_BIT;
>> > + flags &= ~_PAGE_RW;
>> > + break;
>> > + case p2m_access_rx:
>> > + case p2m_access_wx:
>> > + case p2m_access_rx2rw:
>> > + flags &= ~(_PAGE_NX_BIT | _PAGE_RW);
>> This looks like a mistake — this will unconditionally enable
>> execution, even if the underlying p2m type forbids it.
>> ept_p2m_type_to_flags() doesn’t do that.
>>
>> >
>> > + break;
>> > + case p2m_access_x:
>> > + flags &= ~_PAGE_RW;
>> > + break;
>> > + case p2m_access_rwx:
>> > + default:
>> > + break;
>> > }
>> I think you want another blank line here too.
>>
>> Also, this doesn’t seem to capture the ‘r’ part of the equation —
>> shouldn’t p2m_access_n end up with a not-present p2m entry?
>
> SVM dosen't explicitly provide a read access bit so we treat read and
> write the same way.
Read and write can't possibly be treated the same. You ought to use
the present bit to deny read (really: any) access, as also implied by
George's response.
Also - please trim the quoting of your replies.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] x86/mm: Add mem access rights to NPT
2018-07-19 8:30 ` Jan Beulich
@ 2018-07-19 8:43 ` Razvan Cojocaru
2018-07-19 10:02 ` Jan Beulich
2018-07-19 15:08 ` Tamas K Lengyel
1 sibling, 1 reply; 14+ messages in thread
From: Razvan Cojocaru @ 2018-07-19 8:43 UTC (permalink / raw)
To: Jan Beulich, aisaila; +Cc: Andrew Cooper, tamas, george.dunlap, xen-devel
On 07/19/2018 11:30 AM, Jan Beulich wrote:
>>>> On 19.07.18 at 10:18, <aisaila@bitdefender.com> wrote:
>> On Mi, 2018-07-18 at 15:33 +0000, George Dunlap wrote:
>>>> On Jul 2, 2018, at 8:42 AM, Alexandru Isaila <aisaila@bitdefender.c
>>>> + break;
>>>> + case p2m_access_x:
>>>> + flags &= ~_PAGE_RW;
>>>> + break;
>>>> + case p2m_access_rwx:
>>>> + default:
>>>> + break;
>>>> }
>>> I think you want another blank line here too.
>>>
>>> Also, this doesn’t seem to capture the ‘r’ part of the equation —
>>> shouldn’t p2m_access_n end up with a not-present p2m entry?
>>
>> SVM dosen't explicitly provide a read access bit so we treat read and
>> write the same way.
>
> Read and write can't possibly be treated the same. You ought to use
> the present bit to deny read (really: any) access, as also implied by
> George's response.
They aren't treated the same as far sending out a vm_event goes.
However, if we understand this correctly, there is no way to cause only
read, or only write exits for NPT. They are bundled together under _PAGE_RW.
So svm_do_nested_pgfault() tries to sort these out:
1781 struct npfec npfec = {
1782 .read_access = !(pfec & PFEC_insn_fetch),
1783 .write_access = !!(pfec & PFEC_write_access),
1784 .insn_fetch = !!(pfec & PFEC_insn_fetch),
1785 .present = !!(pfec & PFEC_page_present),
1786 };
1787
1788 /* These bits are mutually exclusive */
1789 if ( pfec & NPT_PFEC_with_gla )
1790 npfec.kind = npfec_kind_with_gla;
1791 else if ( pfec & NPT_PFEC_in_gpt )
1792 npfec.kind = npfec_kind_in_gpt;
1793
1794 ret = hvm_hap_nested_page_fault(gpa, ~0ul, npfec);
but a read access is considered to be something that's not an insn
fetch, and we only have a specific bit set for the write.
Since hvm_hap_nested_page_fault() looks at npfec to decide when to send
out a vm_event, this takes care of handling reads and writes differently
at this level; however it's not possible to set separate single "don't
read" or "don't write" exit-causing flags with NPT.
Thanks,
Razvan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] x86/mm: Add mem access rights to NPT
2018-07-19 8:43 ` Razvan Cojocaru
@ 2018-07-19 10:02 ` Jan Beulich
2018-07-19 13:08 ` Isaila Alexandru
0 siblings, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2018-07-19 10:02 UTC (permalink / raw)
To: aisaila, Razvan Cojocaru; +Cc: Andrew Cooper, tamas, george.dunlap, xen-devel
>>> On 19.07.18 at 10:43, <rcojocaru@bitdefender.com> wrote:
> On 07/19/2018 11:30 AM, Jan Beulich wrote:
>>>>> On 19.07.18 at 10:18, <aisaila@bitdefender.com> wrote:
>>> On Mi, 2018-07-18 at 15:33 +0000, George Dunlap wrote:
>>>>> On Jul 2, 2018, at 8:42 AM, Alexandru Isaila <aisaila@bitdefender.c
>>>>> + break;
>>>>> + case p2m_access_x:
>>>>> + flags &= ~_PAGE_RW;
>>>>> + break;
>>>>> + case p2m_access_rwx:
>>>>> + default:
>>>>> + break;
>>>>> }
>>>> I think you want another blank line here too.
>>>>
>>>> Also, this doesn’t seem to capture the ‘r’ part of the equation —
>>>> shouldn’t p2m_access_n end up with a not-present p2m entry?
>>>
>>> SVM dosen't explicitly provide a read access bit so we treat read and
>>> write the same way.
>>
>> Read and write can't possibly be treated the same. You ought to use
>> the present bit to deny read (really: any) access, as also implied by
>> George's response.
>
> They aren't treated the same as far sending out a vm_event goes.
> However, if we understand this correctly, there is no way to cause only
> read, or only write exits for NPT. They are bundled together under _PAGE_RW.
>
> So svm_do_nested_pgfault() tries to sort these out:
>
> 1781 struct npfec npfec = {
> 1782 .read_access = !(pfec & PFEC_insn_fetch),
> 1783 .write_access = !!(pfec & PFEC_write_access),
> 1784 .insn_fetch = !!(pfec & PFEC_insn_fetch),
> 1785 .present = !!(pfec & PFEC_page_present),
> 1786 };
> 1787
> 1788 /* These bits are mutually exclusive */
> 1789 if ( pfec & NPT_PFEC_with_gla )
> 1790 npfec.kind = npfec_kind_with_gla;
> 1791 else if ( pfec & NPT_PFEC_in_gpt )
> 1792 npfec.kind = npfec_kind_in_gpt;
> 1793
> 1794 ret = hvm_hap_nested_page_fault(gpa, ~0ul, npfec);
>
> but a read access is considered to be something that's not an insn
> fetch, and we only have a specific bit set for the write.
>
> Since hvm_hap_nested_page_fault() looks at npfec to decide when to send
> out a vm_event, this takes care of handling reads and writes differently
> at this level; however it's not possible to set separate single "don't
> read" or "don't write" exit-causing flags with NPT.
All fine, but George's question was raised in the context of permission
conversion from p2m to pte representation.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] x86/mm: Add mem access rights to NPT
2018-07-19 10:02 ` Jan Beulich
@ 2018-07-19 13:08 ` Isaila Alexandru
2018-07-20 9:16 ` George Dunlap
0 siblings, 1 reply; 14+ messages in thread
From: Isaila Alexandru @ 2018-07-19 13:08 UTC (permalink / raw)
To: Jan Beulich, Razvan Cojocaru
Cc: Andrew Cooper, tamas, george.dunlap, xen-devel
On Jo, 2018-07-19 at 04:02 -0600, Jan Beulich wrote:
> >
> > >
> > > >
> > > > On 19.07.18 at 10:43, <rcojocaru@bitdefender.com> wrote:
> > On 07/19/2018 11:30 AM, Jan Beulich wrote:
> > >
> > > >
> > > > >
> > > > > >
> > > > > > On 19.07.18 at 10:18, <aisaila@bitdefender.com> wrote:
> > > > On Mi, 2018-07-18 at 15:33 +0000, George Dunlap wrote:
> > > > >
> > > > > >
> > > > > > On Jul 2, 2018, at 8:42 AM, Alexandru Isaila <aisaila@bitde
> > > > > > fender.c
> > > > > > + break;
> > > > > > + case p2m_access_x:
> > > > > > + flags &= ~_PAGE_RW;
> > > > > > + break;
> > > > > > + case p2m_access_rwx:
> > > > > > + default:
> > > > > > + break;
> > > > > > }
> > > > > I think you want another blank line here too.
> > > > >
> > > > > Also, this doesn’t seem to capture the ‘r’ part of the
> > > > > equation —
> > > > > shouldn’t p2m_access_n end up with a not-present p2m entry?
> > > > SVM dosen't explicitly provide a read access bit so we treat
> > > > read and
> > > > write the same way.
> > > Read and write can't possibly be treated the same. You ought to
> > > use
> > > the present bit to deny read (really: any) access, as also
> > > implied by
> > > George's response.
> > They aren't treated the same as far sending out a vm_event goes.
> > However, if we understand this correctly, there is no way to cause
> > only
> > read, or only write exits for NPT. They are bundled together under
> > _PAGE_RW.
> >
> > So svm_do_nested_pgfault() tries to sort these out:
> >
> > 1781 struct npfec npfec = {
> > 1782 .read_access = !(pfec & PFEC_insn_fetch),
> > 1783 .write_access = !!(pfec & PFEC_write_access),
> > 1784 .insn_fetch = !!(pfec & PFEC_insn_fetch),
> > 1785 .present = !!(pfec & PFEC_page_present),
> > 1786 };
> > 1787
> > 1788 /* These bits are mutually exclusive */
> > 1789 if ( pfec & NPT_PFEC_with_gla )
> > 1790 npfec.kind = npfec_kind_with_gla;
> > 1791 else if ( pfec & NPT_PFEC_in_gpt )
> > 1792 npfec.kind = npfec_kind_in_gpt;
> > 1793
> > 1794 ret = hvm_hap_nested_page_fault(gpa, ~0ul, npfec);
> >
> > but a read access is considered to be something that's not an insn
> > fetch, and we only have a specific bit set for the write.
> >
> > Since hvm_hap_nested_page_fault() looks at npfec to decide when to
> > send
> > out a vm_event, this takes care of handling reads and writes
> > differently
> > at this level; however it's not possible to set separate single
> > "don't
> > read" or "don't write" exit-causing flags with NPT.
> All fine, but George's question was raised in the context of
> permission
> conversion from p2m to pte representation.
I've tried a test with xen access that sets XENMEM_access_n on all the
pages and in p2m_type_to_flags() remove the _PAGE_PRESENT flag for
the p2m_access_n case and the guest fails with triple fault. There are
a lot of checks against _PAGE_PRESENT in p2m-pt and a lot of return
invalid if the flag is not present. I don't think this is the way to go
with the p2m_access_n flags.
Thanks,
Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] x86/mm: Add mem access rights to NPT
2018-07-19 8:30 ` Jan Beulich
2018-07-19 8:43 ` Razvan Cojocaru
@ 2018-07-19 15:08 ` Tamas K Lengyel
2018-07-19 18:42 ` Jan Beulich
1 sibling, 1 reply; 14+ messages in thread
From: Tamas K Lengyel @ 2018-07-19 15:08 UTC (permalink / raw)
To: Jan Beulich
Cc: Alexandru Isaila, Andrew Cooper, George Dunlap, Razvan Cojocaru,
Xen-devel
On Thu, Jul 19, 2018 at 2:30 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 19.07.18 at 10:18, <aisaila@bitdefender.com> wrote:
> > On Mi, 2018-07-18 at 15:33 +0000, George Dunlap wrote:
> >> > On Jul 2, 2018, at 8:42 AM, Alexandru Isaila <aisaila@bitdefender.c
> >> > @@ -112,8 +117,37 @@ 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;
> >> > + }
> >> I think you want a blank line here.
> >>
> >> >
> >> > + switch ( access )
> >> > + {
> >> > + case p2m_access_r:
> >> > + case p2m_access_w:
> >> > + flags |= _PAGE_NX_BIT;
> >> > + flags &= ~_PAGE_RW;
> >> > + break;
> >> > + case p2m_access_rw:
> >> > + flags |= _PAGE_NX_BIT;
> >> > + break;
> >> > + case p2m_access_n:
> >> > + case p2m_access_n2rwx:
> >> > + flags |= _PAGE_NX_BIT;
> >> > + flags &= ~_PAGE_RW;
> >> > + break;
> >> > + case p2m_access_rx:
> >> > + case p2m_access_wx:
> >> > + case p2m_access_rx2rw:
> >> > + flags &= ~(_PAGE_NX_BIT | _PAGE_RW);
> >> This looks like a mistake — this will unconditionally enable
> >> execution, even if the underlying p2m type forbids it.
> >> ept_p2m_type_to_flags() doesn’t do that.
> >>
> >> >
> >> > + break;
> >> > + case p2m_access_x:
> >> > + flags &= ~_PAGE_RW;
> >> > + break;
> >> > + case p2m_access_rwx:
> >> > + default:
> >> > + break;
> >> > }
> >> I think you want another blank line here too.
> >>
> >> Also, this doesn’t seem to capture the ‘r’ part of the equation —
> >> shouldn’t p2m_access_n end up with a not-present p2m entry?
> >
> > SVM dosen't explicitly provide a read access bit so we treat read and
> > write the same way.
>
> Read and write can't possibly be treated the same. You ought to use
> the present bit to deny read (really: any) access, as also implied by
> George's response.
We already treat write accesses also as read on Intel because of
hardware limitations with CMPXCHG. So I don't see a problem with this.
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] x86/mm: Add mem access rights to NPT
2018-07-19 15:08 ` Tamas K Lengyel
@ 2018-07-19 18:42 ` Jan Beulich
0 siblings, 0 replies; 14+ messages in thread
From: Jan Beulich @ 2018-07-19 18:42 UTC (permalink / raw)
To: tamas; +Cc: aisaila, andrew.cooper3, george.dunlap, rcojocaru, xen-devel
>>> Tamas K Lengyel <tamas@tklengyel.com> 07/19/18 5:09 PM >>>
>On Thu, Jul 19, 2018 at 2:30 AM Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 19.07.18 at 10:18, <aisaila@bitdefender.com> wrote:
> > On Mi, 2018-07-18 at 15:33 +0000, George Dunlap wrote:
> >> > On Jul 2, 2018, at 8:42 AM, Alexandru Isaila <aisaila@bitdefender.c
>> >> > + break;
>> >> > + case p2m_access_x:
>> >> > + flags &= ~_PAGE_RW;
>> >> > + break;
>> >> > + case p2m_access_rwx:
>> >> > + default:
>> >> > + break;
>> >> > }
>> >> I think you want another blank line here too.
>> >>
>> >> Also, this doesn’t seem to capture the ‘r’ part of the equation —
>> >> shouldn’t p2m_access_n end up with a not-present p2m entry?
>> >
>> > SVM dosen't explicitly provide a read access bit so we treat read and
>> > write the same way.
>>
>> Read and write can't possibly be treated the same. You ought to use
>> the present bit to deny read (really: any) access, as also implied by
>> George's response.
>
>We already treat write accesses also as read on Intel because of
>hardware limitations with CMPXCHG. So I don't see a problem with this.
Right - write implies read. Which means no-read implies no-write. Which
still means to me that p2m_access_n can't result in other than a not-
present entry.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] x86/mm: Add mem access rights to NPT
2018-07-19 13:08 ` Isaila Alexandru
@ 2018-07-20 9:16 ` George Dunlap
2018-07-20 11:58 ` Isaila Alexandru
0 siblings, 1 reply; 14+ messages in thread
From: George Dunlap @ 2018-07-20 9:16 UTC (permalink / raw)
To: Isaila Alexandru, Jan Beulich, Razvan Cojocaru
Cc: Andrew Cooper, tamas, xen-devel
On 07/19/2018 02:08 PM, Isaila Alexandru wrote:
> On Jo, 2018-07-19 at 04:02 -0600, Jan Beulich wrote:
>>>
>>>>
>>>>>
>>>>> On 19.07.18 at 10:43, <rcojocaru@bitdefender.com> wrote:
>>> On 07/19/2018 11:30 AM, Jan Beulich wrote:
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>> On 19.07.18 at 10:18, <aisaila@bitdefender.com> wrote:
>>>>> On Mi, 2018-07-18 at 15:33 +0000, George Dunlap wrote:
>>>>>>
>>>>>>>
>>>>>>> On Jul 2, 2018, at 8:42 AM, Alexandru Isaila <aisaila@bitde
>>>>>>> fender.c
>>>>>>> + break;
>>>>>>> + case p2m_access_x:
>>>>>>> + flags &= ~_PAGE_RW;
>>>>>>> + break;
>>>>>>> + case p2m_access_rwx:
>>>>>>> + default:
>>>>>>> + break;
>>>>>>> }
>>>>>> I think you want another blank line here too.
>>>>>>
>>>>>> Also, this doesn’t seem to capture the ‘r’ part of the
>>>>>> equation —
>>>>>> shouldn’t p2m_access_n end up with a not-present p2m entry?
>>>>> SVM dosen't explicitly provide a read access bit so we treat
>>>>> read and
>>>>> write the same way.
>>>> Read and write can't possibly be treated the same. You ought to
>>>> use
>>>> the present bit to deny read (really: any) access, as also
>>>> implied by
>>>> George's response.
>>> They aren't treated the same as far sending out a vm_event goes.
>>> However, if we understand this correctly, there is no way to cause
>>> only
>>> read, or only write exits for NPT. They are bundled together under
>>> _PAGE_RW.
>>>
>>> So svm_do_nested_pgfault() tries to sort these out:
>>>
>>> 1781 struct npfec npfec = {
>>> 1782 .read_access = !(pfec & PFEC_insn_fetch),
>>> 1783 .write_access = !!(pfec & PFEC_write_access),
>>> 1784 .insn_fetch = !!(pfec & PFEC_insn_fetch),
>>> 1785 .present = !!(pfec & PFEC_page_present),
>>> 1786 };
>>> 1787
>>> 1788 /* These bits are mutually exclusive */
>>> 1789 if ( pfec & NPT_PFEC_with_gla )
>>> 1790 npfec.kind = npfec_kind_with_gla;
>>> 1791 else if ( pfec & NPT_PFEC_in_gpt )
>>> 1792 npfec.kind = npfec_kind_in_gpt;
>>> 1793
>>> 1794 ret = hvm_hap_nested_page_fault(gpa, ~0ul, npfec);
>>>
>>> but a read access is considered to be something that's not an insn
>>> fetch, and we only have a specific bit set for the write.
>>>
>>> Since hvm_hap_nested_page_fault() looks at npfec to decide when to
>>> send
>>> out a vm_event, this takes care of handling reads and writes
>>> differently
>>> at this level; however it's not possible to set separate single
>>> "don't
>>> read" or "don't write" exit-causing flags with NPT.
>> All fine, but George's question was raised in the context of
>> permission
>> conversion from p2m to pte representation.
>
> I've tried a test with xen access that sets XENMEM_access_n on all the
> pages and in p2m_type_to_flags() remove the _PAGE_PRESENT flag for
> the p2m_access_n case and the guest fails with triple fault. There are
> a lot of checks against _PAGE_PRESENT in p2m-pt and a lot of return
> invalid if the flag is not present. I don't think this is the way to go
> with the p2m_access_n flags.
I will absolutely nack any interface where if the caller says, "Please
remove read permission", the hypervisor says, "OK!" but then allows read
permission anyway -- particularly in one which is allegedly designed for
security tools.
If it's not practical / more work than it's worth doing at the moment to
implement p2m_access_n on NPT, then you should return an error when it's
requested.
The same really should be true for write-only permission as well -- if
it's not possible to allow writes but not reads, then you should return
an error when such permissions are requested.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] x86/mm: Add mem access rights to NPT
2018-07-19 8:18 ` Isaila Alexandru
2018-07-19 8:20 ` Razvan Cojocaru
2018-07-19 8:30 ` Jan Beulich
@ 2018-07-20 10:05 ` George Dunlap
2 siblings, 0 replies; 14+ messages in thread
From: George Dunlap @ 2018-07-20 10:05 UTC (permalink / raw)
To: Isaila Alexandru; +Cc: Andrew Cooper, tamas, jbeulich, rcojocaru, xen-devel
On 07/19/2018 09:18 AM, Isaila Alexandru wrote:
> On Mi, 2018-07-18 at 15:33 +0000, George Dunlap wrote:
>>
>>>
>>> On Jul 2, 2018, at 8:42 AM, Alexandru Isaila <aisaila@bitdefender.c
>>> om> wrote:
>>>
>>> From: Isaila Alexandru <aisaila@bitdefender.com>
>>>
>>> This patch adds access rights for the NPT pages. The access rights
>>> are
>>> saved in a radix tree with the root saved in p2m_domain. The rights
>>> are manipulated through p2m_set_access()
>>> and p2m_get_access() functions.
>>> The patch follows the ept logic.
>> This description needs to be much more complete. Something like
>> this:
>>
>> ---
>> 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.
>> ---
>>
> I will update the patch description.
>>>
>>>
>>> Note: It was tested with xen-access write
>>>
>>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
>>
>>>
>>>
>>> ---
>>> Changes since V2:
>>> - Delete blak line
>>> - Add return if p2m_access_rwx = a
>>> - Delete the comment from p2m_pt_get_entry()
>>> - Moved radix_tree_init() to arch_monitor_init_domain().
>>> ---
>>> xen/arch/x86/mm/mem_access.c | 3 ++
>>> xen/arch/x86/mm/p2m-pt.c | 109
>>> ++++++++++++++++++++++++++++++++++-----
>>> xen/arch/x86/mm/p2m.c | 6 +++
>>> xen/arch/x86/monitor.c | 13 +++++
>>> xen/include/asm-x86/mem_access.h | 2 +-
>>> xen/include/asm-x86/p2m.h | 6 +++
>>> 6 files changed, 124 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/mm/mem_access.c
>>> b/xen/arch/x86/mm/mem_access.c
>>> index c0cd017..d78c82c 100644
>>> --- a/xen/arch/x86/mm/mem_access.c
>>> +++ b/xen/arch/x86/mm/mem_access.c
>>> @@ -221,7 +221,10 @@ 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.gla_valid || cpu_has_svm )
>>> + {
>> I can’t immediately tell what this is about, which means it needs a
>> comment.
>>
>> It may also be (depending on why this is here) that “cpu_has_svm”
>> makes this more fragile — if this is really about having a radix
>> tree, for instance, then you should probably check for a radix tree.
>
> This is about the different npfec on SVN. The gla in never valid so the
> fault flag will not be set.
Right -- then really this check was always a VMX-ism, and should really
have been:
/* VMX can only tell the fault type if gla_valid is set */
if ( !(cpu_has_vmx && !npfec.gla_valid) ) {
...
But in any case, if cpu_has_vmx && !npfec.gla_valid, then npfec.kind
should be npfec_kind_unknown (==0); so is there any reason not to read
npfec.kind here unconditionally? i.e., like below?
if ( npfec.gla_valid )
{
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;
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] x86/mm: Add mem access rights to NPT
2018-07-20 9:16 ` George Dunlap
@ 2018-07-20 11:58 ` Isaila Alexandru
0 siblings, 0 replies; 14+ messages in thread
From: Isaila Alexandru @ 2018-07-20 11:58 UTC (permalink / raw)
To: George Dunlap, Jan Beulich, Razvan Cojocaru
Cc: Andrew Cooper, tamas, xen-devel
> I will absolutely nack any interface where if the caller says,
> "Please
> remove read permission", the hypervisor says, "OK!" but then allows
> read
> permission anyway -- particularly in one which is allegedly designed
> for
> security tools.
>
> If it's not practical / more work than it's worth doing at the moment
> to
> implement p2m_access_n on NPT, then you should return an error when
> it's
> requested.
>
> The same really should be true for write-only permission as well --
> if
> it's not possible to allow writes but not reads, then you should
> return
> an error when such permissions are requested.
I will limit the supported access rights and return error for
read/write only and _n.
Regards,
Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-07-20 11:58 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-02 12:42 [PATCH v3] x86/mm: Add mem access rights to NPT Alexandru Isaila
2018-07-17 12:59 ` PING: " Isaila Alexandru
2018-07-18 15:33 ` George Dunlap
2018-07-19 8:18 ` Isaila Alexandru
2018-07-19 8:20 ` Razvan Cojocaru
2018-07-19 8:30 ` Jan Beulich
2018-07-19 8:43 ` Razvan Cojocaru
2018-07-19 10:02 ` Jan Beulich
2018-07-19 13:08 ` Isaila Alexandru
2018-07-20 9:16 ` George Dunlap
2018-07-20 11:58 ` Isaila Alexandru
2018-07-19 15:08 ` Tamas K Lengyel
2018-07-19 18:42 ` Jan Beulich
2018-07-20 10:05 ` George Dunlap
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.