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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Mon, Mar 13, 2017 at 6:29 AM, Razvan Cojocaru
<rcojocaru@xxxxxxxxxxxxxxx> wrote:
> On 03/13/2017 02:19 PM, Jan Beulich wrote:
>>>>> On 13.03.17 at 13:01, <rcojocaru@xxxxxxxxxxxxxxx> 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@xxxxxxxxxx> wrote:
>>>>> On 10/03/17 07:34, Jan Beulich wrote:
>>>>>>>>> On 09.03.17 at 18:29, <tamas@xxxxxxxxxxxxx> wrote:
>>>>>>> On Thu, Mar 9, 2017 at 9:56 AM, Jan Beulich <JBeulich@xxxxxxxx> 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.

Tamas

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