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

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

On Thu, Feb 14, 2019 at 04:25:49AM -0700, Jan Beulich wrote:
> >>> On 11.02.19 at 18:46, <roger.pau@xxxxxxxxxx> wrote:
> > @@ -948,6 +951,11 @@ static inline void p2m_entry_modify(struct p2m_domain 
> > *p2m, p2m_type_t nt,
> >          p2m->ioreq.entry_count++;
> >          break;
> >  
> > +    case p2m_map_foreign:
> > +        BUG_ON(!mfn_valid(nfn) ||
> > +               !page_get_owner_and_reference(mfn_to_page(nfn)));
> > +        break;
> 
> Asserting that the passed in MFN is valid is fine. Asserting that a
> reference can be got is not, as this sets us up for a DoS in case
> of a refcount overflow, or the page having got ballooned out by
> its owner. That is, the issue of you folding the two original calls
> into one is wider than just the two distinct error codes getting lost
> that were previously produced - you can't (currently) report up
> any error from this low layer. (And I'm sorry, I should have noticed
> this on v1 already.)

What about using something like:

case p2m_map_foreign:
    BUG_ON(!mfn_valid(nfn));

    if ( !page_get_owner_and_reference(mfn_to_page(nfn)) )
    {
        ASSERT_UNREACHABLE();
        return;
    }

    break;

It's not strictly worse than what's currently done in EPT code, but I
agree it should be improved.

Improving this will mean modifying the write_p2m_entry hook, and all
the callers, together with fixing the handling of the return code from
atomic_write_ept_entry, which is currently only asserted.

Thanks, Roger.

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