Re: [Xen-devel] [PATCH 7/8] x86/mm: handle foreign mappings in p2m_entry_modify

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 1/30/19 10:36 AM, Roger Pau Monne wrote:
> So that the specific handling can be removed from
> atomic_write_ept_entry and be shared with npt and shadow code.
> 
> This commit also removes the check that prevent non-ept PVH dom0 from
> mapping foreign pages.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> Cc: Jun Nakajima <jun.nakajima@xxxxxxxxx>
> Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
> Cc: Tim Deegan <tim@xxxxxxx>
> ---
>  xen/arch/x86/mm/hap/hap.c       |   3 +-
>  xen/arch/x86/mm/p2m-ept.c       | 108 +++++++-------------------------
>  xen/arch/x86/mm/p2m-pt.c        |   7 ---
>  xen/arch/x86/mm/shadow/common.c |   3 +-
>  xen/include/asm-x86/p2m.h       |  30 ++++++++-
>  5 files changed, 53 insertions(+), 98 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
> index dc46d5e14f..4f52639be5 100644
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -735,7 +735,8 @@ hap_write_p2m_entry(struct domain *d, unsigned long gfn, 
> l1_pgentry_t *p,
>      }
>  
>      p2m_entry_modify(p2m_get_hostp2m(d), 
> p2m_flags_to_type(l1e_get_flags(new)),
> -                     p2m_flags_to_type(old_flags), level);
> +                     p2m_flags_to_type(old_flags), l1e_get_mfn(new),
> +                     l1e_get_mfn(*p), level);
>  
>      safe_write_pte(p, new);
>      if ( old_flags & _PAGE_PRESENT )
> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> index 0ece6608cb..2b0c3ab265 100644
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -45,65 +45,13 @@ static inline bool_t is_epte_valid(ept_entry_t *e)
>      return ((e->epte & ~(1ul << 63)) != 0 && e->sa_p2mt != p2m_invalid);
>  }
>  
> -/* returns : 0 for success, -errno otherwise */
> -static int atomic_write_ept_entry(struct p2m_domain *p2m,
> -                                  ept_entry_t *entryptr, ept_entry_t new,
> -                                  int level)
> +static void atomic_write_ept_entry(struct p2m_domain *p2m,
> +                                   ept_entry_t *entryptr, ept_entry_t new,
> +                                   int level)
>  {
> -    int rc;
> -    unsigned long oldmfn = mfn_x(INVALID_MFN);
> -    bool_t check_foreign = (new.mfn != entryptr->mfn ||
> -                            new.sa_p2mt != entryptr->sa_p2mt);
> -
> -    if ( level )
> -    {
> -        ASSERT(!is_epte_superpage(&new) || !p2m_is_foreign(new.sa_p2mt));
> -        write_atomic(&entryptr->epte, new.epte);
> -        return 0;
> -    }
> -
> -    if ( unlikely(p2m_is_foreign(new.sa_p2mt)) )
> -    {
> -        rc = -EINVAL;
> -        if ( !is_epte_present(&new) )
> -                goto out;
> -
> -        if ( check_foreign )
> -        {
> -            struct domain *fdom;
> -
> -            if ( !mfn_valid(_mfn(new.mfn)) )
> -                goto out;
> -
> -            rc = -ESRCH;
> -            fdom = page_get_owner(mfn_to_page(_mfn(new.mfn)));
> -            if ( fdom == NULL )
> -                goto out;
> -
> -            /* get refcount on the page */
> -            rc = -EBUSY;
> -            if ( !get_page(mfn_to_page(_mfn(new.mfn)), fdom) )
> -                goto out;
> -        }
> -    }
> -
> -    if ( unlikely(p2m_is_foreign(entryptr->sa_p2mt)) && check_foreign )
> -        oldmfn = entryptr->mfn;
> -
> -    p2m_entry_modify(p2m, new.sa_p2mt, entryptr->sa_p2mt, level);
> -
> +    p2m_entry_modify(p2m, new.sa_p2mt, entryptr->sa_p2mt, _mfn(new.mfn),
> +                     _mfn(entryptr->mfn), level);
>      write_atomic(&entryptr->epte, new.epte);
> -
> -    if ( unlikely(oldmfn != mfn_x(INVALID_MFN)) )
> -        put_page(mfn_to_page(_mfn(oldmfn)));
> -
> -    rc = 0;
> -
> - out:
> -    if ( rc )
> -        gdprintk(XENLOG_ERR, "epte o:%"PRIx64" n:%"PRIx64" rc:%d\n",
> -                 entryptr->epte, new.epte, rc);
> -    return rc;
>  }

This is pretty awesome. :-)

> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index 834d49d2d4..1cc8acb3fe 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -933,9 +933,12 @@ struct hvm_ioreq_server *p2m_get_ioreq_server(struct 
> domain *d,
>                                                unsigned int *flags);
>  
>  static inline void p2m_entry_modify(struct p2m_domain *p2m, p2m_type_t nt,
> -                                    p2m_type_t ot, unsigned int level)
> +                                    p2m_type_t ot, mfn_t nfn, mfn_t ofn,
> +                                    unsigned int level)
>  {
> -    if ( level != 1 || nt == ot )
> +    struct page_info *pg;
> +
> +    if ( level != 1 || (nt == ot && mfn_eq(nfn, ofn)) )
>          return;

Are you sure that foreign mappings (or ioreq server pages, for that
matter) can never be level > 1?

ioreq server pages may be relatively harmless if we get out of sync; but
the reference count with the foreign mapping is really dangerous if it
gets screwed up.

I'd be tempted to say that we should BUG_ON(level > 1 && nt == foreign).

>  
>      switch ( nt )
> @@ -948,6 +951,17 @@ static inline void p2m_entry_modify(struct p2m_domain 
> *p2m, p2m_type_t nt,
>          p2m->ioreq.entry_count++;
>          break;
>  
> +    case p2m_map_foreign:
> +        pg = mfn_to_page(nfn);
> +
> +        if ( !pg || !page_get_owner_and_reference(pg) )
> +        {
> +            ASSERT_UNREACHABLE();
> +            return;
> +        }

Similarly, I'd be tempted to say we should BUG_ON() here instead.  If a
rogue guest can trigger this path, it would be a DoS; but the alternate
would be to allow the mfn to be put into the p2m table without a
reference, which could potentially be far worse.

The alternate would be to have this return an error value, which would
1) cause the p2m write to fail, and 2) be checked all the way up the chain.

Less worried about the removal side, as  if we have BUG_ON's on the
insertion side, they *really* shouldn't happen.

 -George

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