Re: [Xen-devel] [PATCH] hvm/altp2m: Clarify the proper way to extend the altp2m interface

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 07/10/2018 12:58 PM, Jan Beulich wrote:
>>>> On 10.07.18 at 12:30, <george.dunlap@xxxxxxxxxx> wrote:
>> On 07/10/2018 11:01 AM, Jan Beulich wrote:
>>>>>> On 10.07.18 at 11:33, <george.dunlap@xxxxxxxxxx> wrote:
>>>> As far as I can tell there are three possible solutions to this
>>>> controversy:
>>>>
>>>> A. Remove the 'internal' functionality as a target by converting the
>>>> current HVMOP into a DOMCTL.
>>>>
>>>> B. Have two hypercalls -- an HVMOP which contains functionality
>>>> expected to be used by the 'internal' agent, and a DOMCTL for
>>>> functionality which is expected to be used only be the 'internal'
>>>> agent.
>>>>
>>>> C. Agree to add all new subops to the current hypercall (HVMOP), even
>>>> if we're not sure if they should be exposed to the guest.
>>>>
>>>> I think A is a terrible idea.  Having a single in-guest agent is a
>>>> reasonable design choice, and apparently it was even implemented at
>>>> some point; we should make it straightforward for someone in the
>>>> future to pick up the work if they want to.
>>>>
>>>> I think B is also a terrible idea.  The people extending it at the
>>>> moment are primarily concerned with the 'external' use case.  There is
>>>> nobody around to represent whether new functionality should end up in
>>>> the HVMOP or the DOMCTL, which means that by default it will end up in
>>>> the DOMCTL.  If it is discovered, afterwards, that the new operations
>>>> *would* be safe and useful for the 'internal' use case, then we will
>>>> have to duplicate them inside the HVMOP.
>>>
>>> They'd have to be _moved_ to HVMOP, not duplicated.
>>
>> I'd assumed we would want to be backwards compatible with existing dom0
>> agents.
> 
> No domctl should ever be considered stable, and no dom0 agent
> should call them without going through the libxc wrapper.

Naturally; but we shouldn't change them for no reason, and I was
envisioning an agent designed to compile against several versions of Xen
looking like this:

#if XEN_INTERFACE < nnn
   domctl_altp2m_op(blah blah blah);
#else
   hvmop_altp2m_op(blah blah blah);
#endif

But again, if the exact hypercall were switched behind the scenes in
libxc, that would make this option siginificantly less bad.

I'm not a fan of having two different hypercalls for this, but if you
posted a similar patch documenting that as the way forward I wouldn't
argue against it.  (I remember Andy not being a fan either, but I don't
know what his concerns were.)

> 
>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>> @@ -4460,6 +4460,34 @@ static int hvmop_get_param(
>>>>      return rc;
>>>>  }
>>>>  
>>>> +/*
>>>> + * altp2m operations are envisioned as being used in several different 
>>>> + * modes:
>>>> + * 
>>>> + * - external: All control and decisions are made by an external agent
>>>> + *   running domain 0.
>>>> + *
>>>> + * - internal: altp2m operations are used exclusively by an in-guest agent
>>>> + *   to protect itself from the guest kernel and in-guest attackers.  
>>>> + * 
>>>> + * - coordinated: An in-guest agent handles #VE and VMFUNCs locally,
>>>> + *   but makes requests of an external entity for bigger changes (such
>>>> + *   as modifying altp2m entires).
>>>> + *
>>>> + * This corresponds to the three values for HVM_PARAM_ALTP2M
>>>> + * (external, mixed, limited). All three models have advantages and
>>>> + * disadvantages.
>>>> + *
>>>> + * Normally hypercalls made by a program in domain 0 in order to
>>>> + * control a guest would be DOMCTLs rather than HVMOPs.  But in order
>>>> + * to properly enable the 'internal' use case, as well as to avoid
>>>> + * fragmentation, all altp2m subops should come under this single
>>>> + * HVMOP.
>>>> + *
>>>> + * New subops which may not be suitable for the 'internal' use case
>>>> + * should be disabled in the "XEN_ALTP2M_mixed" case in
>>>> + * xsm_hvm_altp2mhvm_op().
>>>> + */
>>>
>>> To me this last statement sort of implies (due to the lack of any black
>>> or white listing in xsm_hvm_altp2mhvm_op()'s XEN_ALTP2M_mixed
>>> handling) that all current ops are considered safe for internal use,
>>> which I highly doubt.
>> Given a blacklist (or an invitation to make one), someone might indeed
>> infer that the items not blacklisted had been evaluated and deemed safe
>> to use.  We have two possible solutions:
>>
>> 1. Go through and make such an evaluation, blacklisting everything we
>> don't think is necessary / safe
>>
>> 2. Write a comment saying that the interface hasn't been evaluated.
>>
>> Or 2a: Include a comment saying the 'internal' interface hasn't been
>> evaluated for safety, and don't bother blacklisting new ops until such
>> an evaluation has been made.
> 
> I see no value in a black list if only future things would be added to it.
> I'm okay with 2a, but I think 1 would make easier the future job of
> auditing the whole lot for supportability.

The issue with 1 is who's going to do it.  It really requires someone
who wants to use the interface -- or at least someone who has a good
understanding of how the previous user actually used the interface.

I don't think it's a good use of my time; given that you're not a fan of
the whole idea in the first place, you're probably not a good candidate;
and I expect Tamas and Razvan aren't terribly interested either.

 -George

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