On Mon, Jul 23, 2018 at 2:48 PM Alexandru Isaila
<aisa...@bitdefender.com> wrote:
>
> From: Isaila Alexandru <aisa...@bitdefender.com>
>
> This patch adds access control for NPT mode.
>
> There aren’t enough extra bits to store the access rights in the NPT p2m
> table, so we add a radix tree to store the rights. For efficiency,
> remove entries which match the default permissions rather than
> continuing to store them.
>
> Modify p2m-pt.c:p2m_type_to_flags() to mirror the ept version: taking an
> access, and removing / adding RW or NX flags as appropriate.
>
> Note: It was tested with xen-access write
>
> Signed-off-by: Alexandru Isaila <aisa...@bitdefender.com>
>
> ---
> Changes since V3:
> - Add p2m_pt_check_access() to filter n, w, wx, n2rwx from
> supported page rights
> - Add rights check for the default_access change in the
> IVALID_GFN case
> - Add blank lines
> - Remove cpu_has_svm if from p2m_mem_access_check()
> - Add xfree(msr_bitmap) in case of error on
> xalloc(raxid_tree_root).
> ---
> xen/arch/x86/mm/mem_access.c | 17 +++---
> xen/arch/x86/mm/p2m-ept.c | 7 +++
> xen/arch/x86/mm/p2m-pt.c | 124
> ++++++++++++++++++++++++++++++++++-----
> xen/arch/x86/mm/p2m.c | 6 ++
> xen/arch/x86/monitor.c | 15 +++++
> xen/include/asm-x86/mem_access.h | 2 +-
> xen/include/asm-x86/p2m.h | 7 +++
> 7 files changed, 156 insertions(+), 22 deletions(-)
>
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index c0cd017..cab72bc 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -221,12 +221,12 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long
> gla,
> {
> req->u.mem_access.flags |= MEM_ACCESS_GLA_VALID;
> req->u.mem_access.gla = gla;
> -
> - if ( npfec.kind == npfec_kind_with_gla )
> - req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA;
> - else if ( npfec.kind == npfec_kind_in_gpt )
> - req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT;
> }
> +
> + if ( npfec.kind == npfec_kind_with_gla )
> + req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA;
> + else if ( npfec.kind == npfec_kind_in_gpt )
> + req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT;
> req->u.mem_access.flags |= npfec.read_access ? MEM_ACCESS_R : 0;
> req->u.mem_access.flags |= npfec.write_access ? MEM_ACCESS_W : 0;
> req->u.mem_access.flags |= npfec.insn_fetch ? MEM_ACCESS_X : 0;
> @@ -366,8 +366,11 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn,
> uint32_t nr,
> /* If request to set default access. */
> if ( gfn_eq(gfn, INVALID_GFN) )
> {
> - p2m->default_access = a;
> - return 0;
> + if ( (rc = p2m->check_access(a)) == 0 )
> + {
> + p2m->default_access = a;
> + return 0;
> + }
> }
BTW this introduces a bug -- if the check fails, this will fall
through into the gfn loop below, rather than returning the error.
> @@ -87,23 +88,27 @@ static unsigned long p2m_type_to_flags(const struct
> p2m_domain *p2m,
> case p2m_ram_paged:
> case p2m_ram_paging_in:
> default:
> - return flags | _PAGE_NX_BIT;
> + flags |= P2M_BASE_FLAGS | _PAGE_NX_BIT;
> + break;
Why did you add in P2M_BASE_FLAGS here?
> case p2m_grant_map_ro:
> return flags | P2M_BASE_FLAGS | _PAGE_NX_BIT;
And is this `return` left here on purpose, or was it missed?
> /* Returns: 0 for success, -errno for failure */
> static int
> p2m_next_level(struct p2m_domain *p2m, void **table,
> @@ -201,6 +268,7 @@ p2m_next_level(struct p2m_domain *p2m, void **table,
> new_entry = l1e_from_mfn(mfn, P2M_BASE_FLAGS | _PAGE_RW);
>
> p2m_add_iommu_flags(&new_entry, level,
> IOMMUF_readable|IOMMUF_writable);
> + p2m_set_access(p2m, gfn, p2m->default_access);
This is clearly wrong -- this isn't a leaf node, it's an intermediate
p2m table; and p2m_next_level() is just acting "under the covers",
filling in missing bits of the p2m table or breaking down superpages.
Since the access information is in a completely separate structure, it
shouldn't need to be modified here at all (indeed, it would be a bug
to do so).
But that does bring up an important issue -- it would appear that this
code is incorrect when setting superpages -- if we set a 2MiB page but
then read a non-2MiB-aligned entry within that page, we'll get the
default access rather than the one we set; same thing with splintering
a superpage into smaller pages.
There's a draft patch addressing this on the way.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
