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

[George Dunlap <dunlapg@xxxxxxxxx>]

[Resending]
On Wed, Sep 26, 2018 at 5:02 PM George Dunlap
<George.Dunlap@xxxxxxxxxxxxx> wrote:
>
> 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