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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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.

The dual hypercall solution would indeed be less terrible if there
weren't interface duplication.  But I still think it's a poor interface
to have half the operations be DOMCTLs and half be HVMOPs, and
occasionally moving between the two.

>> It just makes more sense to have all the altp2m operations in a single
>> place, and a simple way to control whether they're available to the
>> 'internal' use case or not.  As such, I am proposing 'C'.  I know Jan
>> considers this "badness", and objects to the continual "extension" of
>> the "badness", but I disagree, and I strongly object to the other two
>> options.
> 
> There's one aspect that I've apparently forgotten about over time: At
> least the mode is a per-domain property, so as long as the wider
> exposure in mixed mode only affects guest security (but not Xen's or
> that of other guests) this ongoing widening of exposure is perhaps
> indeed not a problem. Question is whether all presently available code
> has been audited for not allowing to compromise anything other than
> the guest itself. But that's fine as long as altp2m altogether is not
> (security) supported.

Right, I think making a case that the current code is reasonably safe is
a prerequisite for being declared security supported.  But that was
always the case.

>> --- 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.

Preferences?

 -George

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