Re: [PATCH v1] Fix p2m_set_suppress_ve

[Andrew Cooper <andrew.cooper3@citrix.com>]

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@bitdefender.com> wrote:
>>> On 4/3/19 5:58 PM, Jan Beulich wrote:
>>>>>>> On 03.04.19 at 16:29, <aisaila@bitdefender.com> 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.

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.

~Andrew

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