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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Feb 07, 2019 at 05:49:16PM +0000, George Dunlap wrote:
> On 1/30/19 10:36 AM, Roger Pau Monne wrote:
> > 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?

Not given the current Xen interface, see
XENMEM_add_to_physmap{_batch}. This will have to change if the
interface is expanded to allow 2M or 1G mappings.

> 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).

Yes, I think that's a sensible safety belt.

> 
> >  
> >      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.

I would go for the BUG_ON ATM, because it's a simpler solution and
callers of p2m_entry_modify should make sure the operation is correct.

Thanks, Roger.

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