Re: [Xen-devel] [PATCH v1] Fix p2m_set_suppress_ve

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Thu, Apr 4, 2019 at 6:49 AM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>
> On 04/04/2019 13:46, Razvan Cojocaru wrote:
> > On 4/3/19 6:30 PM, Jan Beulich wrote:
> >>>>> On 03.04.19 at 17:17, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> >>> On 4/3/19 5:58 PM, Jan Beulich wrote:
> >>>>>>> On 03.04.19 at 16:29, <aisaila@xxxxxxxxxxxxxxx> wrote:
> >>>>> --- a/xen/arch/x86/mm/p2m.c
> >>>>> +++ b/xen/arch/x86/mm/p2m.c
> >>>>> @@ -3011,8 +3011,16 @@ int p2m_set_suppress_ve(struct domain *d,
> >>>>> gfn_t gfn, bool suppress_ve,
> >>>>>        mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL, NULL);
> >>>>>        if ( !mfn_valid(mfn) )
> >>>>>        {
> >>>>> -        rc = -ESRCH;
> >>>>> -        goto out;
> >>>>> +        unsigned int page_order;
> >>>>> +
> >>>>> +        mfn = __get_gfn_type_access(host_p2m, gfn_x(gfn), &t, &a,
> >>>>> +                                    P2M_ALLOC | P2M_UNSHARE,
> >>>>> &page_order, 0);
> >>>>
> >>>> I'm not entirely certain about P2M_ALLOC, but I'm pretty sure that
> >>>> at least P2M_UNSHARE is too heavy: Why would you want to force
> >>>> un-sharing of a page when all you want to alter is #VE behavior?
> >>>
> >>> That logic was taken from p2m_set_altp2m_mem_access(), we thought the
> >>> two cases are very similar.
> >>
> >> I see.
> >
> > On the UNSHARE observation, we don't know why the author originally
> > requested the flag. We decided to keep it on the assumption that it
> > _probably_ handles some corner-case that somebody has come accross.
> >
> > We'll prepare a mini-series factoring out the code we've been
> > discussing in separate functions: one for getting things out of the
> > hostp2m if the entry is not present in the altp2m, and one for the
> > special page-order-dependent code (which is duplicated in
> > p2m_set_altp2m_mem_access() and p2m_change_altp2m_gfn()).
> >
> > Before going into that, are we now certain that ALLOC is sufficient? I
> > believe it should be for _our_ use-cases, but we don't want to break
> > anyone's code. Maybe Tamas knows more about this.
>
> UNSHARE is only for CoW/Paging, which is some experimental code added to
> Xen 4.2(?) and has been slowly bit-rotting ever since.

At least mem_sharing still (suprisingly) works - albeit some recent
sanity-check restrictions on grabbing the lock for two gfn's at the
same time breaks it for debug builds. But that's a different story
that I hope to address in the near future.

>
> We try to use good judgement when considering the intentions, but it is
> fully out of security support and hasn't seen any testing in years.
> You're not going to break anything by making a mistake here.

Tsk, I still use mem_sharing and in fact have put in effort to make it
compatible with altp2m. But as far as triggering UNSHARE just to be
safe, I agree. That is fine and it won't break anything. It is
actually inline with what we agreed on last time I worked on it as a
general guide - keep altp2m and mem_sharing compatible but at least
exclusive for each frame.

Tamas

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