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

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Mon, Jul 23, 2018 at 2:48 PM Alexandru Isaila
<aisaila@xxxxxxxxxxxxxxx> wrote:
>
> From: Isaila Alexandru <aisaila@xxxxxxxxxxxxxxx>
>
> This patch adds access control for NPT mode.
>
> There aren’t enough extra bits to store the access rights in the NPT p2m
> table, so we add a radix tree to store the rights.  For efficiency,
> remove entries which match the default permissions rather than
> continuing to store them.
>
> Modify p2m-pt.c:p2m_type_to_flags() to mirror the ept version: taking an
> access, and removing / adding RW or NX flags as appropriate.
>
> Note: It was tested with xen-access write
>
> Signed-off-by: Alexandru Isaila <aisaila@xxxxxxxxxxxxxxx>
>
> ---
> Changes since V3:
>         - Add p2m_pt_check_access() to filter n, w, wx, n2rwx from
>           supported page rights
>         - Add rights check for the default_access change in the
>           IVALID_GFN case
>         - Add blank lines
>         - Remove cpu_has_svm if from p2m_mem_access_check()
>         - Add xfree(msr_bitmap) in case of error on
>           xalloc(raxid_tree_root).
> ---
>  xen/arch/x86/mm/mem_access.c     |  17 +++---
>  xen/arch/x86/mm/p2m-ept.c        |   7 +++
>  xen/arch/x86/mm/p2m-pt.c         | 124 
> ++++++++++++++++++++++++++++++++++-----
>  xen/arch/x86/mm/p2m.c            |   6 ++
>  xen/arch/x86/monitor.c           |  15 +++++
>  xen/include/asm-x86/mem_access.h |   2 +-
>  xen/include/asm-x86/p2m.h        |   7 +++
>  7 files changed, 156 insertions(+), 22 deletions(-)
>
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index c0cd017..cab72bc 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -221,12 +221,12 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long 
> gla,
>          {
>              req->u.mem_access.flags |= MEM_ACCESS_GLA_VALID;
>              req->u.mem_access.gla = gla;
> -
> -            if ( npfec.kind == npfec_kind_with_gla )
> -                req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA;
> -            else if ( npfec.kind == npfec_kind_in_gpt )
> -                req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT;
>          }
> +
> +        if ( npfec.kind == npfec_kind_with_gla )
> +            req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA;
> +        else if ( npfec.kind == npfec_kind_in_gpt )
> +            req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT;
>          req->u.mem_access.flags |= npfec.read_access    ? MEM_ACCESS_R : 0;
>          req->u.mem_access.flags |= npfec.write_access   ? MEM_ACCESS_W : 0;
>          req->u.mem_access.flags |= npfec.insn_fetch     ? MEM_ACCESS_X : 0;
> @@ -366,8 +366,11 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, 
> uint32_t nr,
>      /* If request to set default access. */
>      if ( gfn_eq(gfn, INVALID_GFN) )
>      {
> -        p2m->default_access = a;
> -        return 0;
> +        if ( (rc = p2m->check_access(a)) == 0 )
> +        {
> +            p2m->default_access = a;
> +            return 0;
> +        }
>      }

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