Re: [PATCH V2] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

From: Razvan Cojocaru <rcojocaru@bitdefender.com>
To: Tamas K Lengyel <tamas@tklengyel.com>
Cc: Ravi Sahita <ravi.sahita@intel.com>,
	"wei.liu2@citrix.com" <wei.liu2@citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	Xen-devel <xen-devel@lists.xen.org>,
	Jan Beulich <JBeulich@suse.com>
Subject: Re: [PATCH V2] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
Date: Wed, 15 Mar 2017 15:12:52 +0200	[thread overview]
Message-ID: <fab13c17-7ef1-7f81-a57d-beae66d7a493@bitdefender.com> (raw)
In-Reply-To: <CABfawhmd_gGdNHuAOjzf41NHD_V2xwmG3AE3y4pS3Hipx5U=YQ@mail.gmail.com>

On 03/13/2017 07:17 PM, Tamas K Lengyel wrote:
> On Mon, Mar 13, 2017 at 6:29 AM, Razvan Cojocaru
> <rcojocaru@bitdefender.com> wrote:
>> On 03/13/2017 02:19 PM, Jan Beulich wrote:
>>>>>> On 13.03.17 at 13:01, <rcojocaru@bitdefender.com> wrote:
>>>> On 03/10/2017 09:01 PM, Tamas K Lengyel wrote:
>>>>> On Fri, Mar 10, 2017 at 4:21 AM, Andrew Cooper
>>>>> <andrew.cooper3@citrix.com> wrote:
>>>>>> On 10/03/17 07:34, Jan Beulich wrote:
>>>>>>>>>> On 09.03.17 at 18:29, <tamas@tklengyel.com> wrote:
>>>>>>>> On Thu, Mar 9, 2017 at 9:56 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>>> However - is this interface supposed to be usable by a guest on itself?
>>>>>>>>> Arguably the same question would apply to some of the other sub-ops
>>>>>>>>> too, but anyway.
>>>>>>>> AFAIK the only op the guest would use on itself is
>>>>>>>> HVMOP_altp2m_vcpu_enable_notify.
>>>>>>> Which then means we should move all of them out of here, into a
>>>>>>> suitable domctl. That will in turn reduce the scope of the bogus
>>>>>>> interface versioning, which Andrew did point out, quite a bit.
>>>>>>
>>>>>> The original usecase for altp2m was for an entirely in-guest agent,
>>>>>> which is why they got in as hvmops to start with.  I don't think it is
>>>>>> wise to break that.
>>>>>>
>>>>>> I think there needs to be slightly finer grain control, identifying
>>>>>> whether a domain may use altp2m, and whether it may configure altp2m
>>>>>> permissions itself.
>>>>>>
>>>>>> The nature of altp2m means that using EPTP switching/etc necessarily can
>>>>>> only happen from inside guest context, but whether you trust the domain
>>>>>> to make adjustments to the permissions itself depends on your usecase
>>>>>> and threat model.
>>>>>>
>>>>>
>>>>> So I'm actively using EPT switching and gfn remapping from a
>>>>> privileged monitor domain (not with VMFUNC). My entire usecase for
>>>>> altp2m is purely external without any in-guest agents. In fact, I have
>>>>> to deploy a custom XSM policy to blacklist altp2mhvm_op being issued
>>>>> from the guest.
>>>>>
>>>>> The reason I mentioned HVMOP_altp2m_vcpu_enable_notify as being the
>>>>> only one I believe that is only accessible from within the guest is
>>>>> this distinction in arch/x86/hvm/hvm.c:
>>>>>
>>>>>     d = ( a.cmd != HVMOP_altp2m_vcpu_enable_notify ) ?
>>>>>         rcu_lock_domain_by_any_id(a.domain) : rcu_lock_current_domain();
>>>>>
>>>>> For the other ops I'm not sure if they were really required to be
>>>>> accessible from within the guest or not. I'm not even sure using them
>>>>> would work from the guest with the above check in place. However, if
>>>>> they do work from the guest then I have no idea how it was supposed to
>>>>> work for security purposes as any application in that guest could just
>>>>> issue a hypercall to manipulate it or even turn it off.
>>>>
>>>> Thanks to all for the replies! What I'm taking away from this is:
>>>>
>>>> 1. The hypercall continuation model proposed by Tamas is fine for HVMOPs.
>>>>
>>>> 2. But we're not sure if these should be DOMCTLs or HVMOPs (except for
>>>> HVMOP_altp2m_vcpu_enable_notify).
>>>>
>>>> 3. If we keep them as HVMOPs, the code for handling the set_mem_access()
>>>> part needs to be duplicated, both for the hypercall continuation / HVMOP
>>>> hypercall structure part, and for the compat part (since the _multi()
>>>> function sends arrays / handles to the hypervisor).
>>>>
>>>> So an agreement on point 2 is required before being able to proceed.
>>>
>>> I think as long as there's no need for the guest to use an operation
>>> on itself, it should not be a hvmop. After all, if you make it a domctl
>>> now and later find a need for it to be called by the guest, we can
>>> always replace the domctl by a hvmop. If, however, you start out
>>> with a hvmop, we'll be bound to be supporting it virtually forever.
>>
>> Since we're on this point, IMHO the xc_altp2m_ prefixed versions of
>> set_mem_access() and set_mem_access_multi() shouldn't exist at all.
>> Plain xc_set_mem_access() and xc_set_mem_access_multi() (as DOMCTLs)
>> should be enough, as long as we also add the view_id as an
>> extra-parameter, where view ID 0 is (already) the default EPT view.
>>
>> As it stands now, xc_set_mem_access() can do less than
>> xc_altp2m_set_mem_access() in that its view ID is always 0, but more
>> than xc_altp2m_set_mem_access() in that it is able to set more than one
>> page with a single hypercall, while the underlying hypervisor code is
>> the same.
> 
> Yeap, I remember suggesting that the two set_mem_access interfaces
> should be merged when altp2m was being contributed. Unfortunately we
> were not yet maintainers to make that suggestion a requirement so it
> was let in without that change..
> 
>>
>> Maybe I'm missing something design-wise (obviously if these really do
>> need to be HVMOPs a separate libxc function is required). Maybe the
>> altp2m maintainers have a different view of the matter.
>>
> 
> I think altp2m is still considered experimental at this point.. With
> that said I'm not sure if the altp2m HVMOPs need to be considered as
> set-in-stone as other HVMOPs might be. I would also like to see the
> mem_access setting interfaces merged.

Then I'll rework the patch to remove the special altp2m functions and
add an extra parameter to the regular xc_set_mem_access() functions
(while also increasing the DOMCTL version macro value). Unless somebody
objects, of course.

Thanks,
Razvan

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