[PATCH] hvm/altp2m: Clarify the proper way to extend the altp2m interface

[PATCH] hvm/altp2m: Clarify the proper way to extend the altp2m interface

[George Dunlap]

The altp2m functionality was originally envisioned to be used in
several different configurations, one of which was a single in-guest
agent that had full operational control of altp2m.  This required the
single hypercall to be an HVMOP, which is the only type of hypercall
an HVM guest is allowed to make.

Exposing the altp2m functionality to the guest was controversial at
the time, but was ultimately accepted.  The fact that altp2m is an
HVMOP rather than a DOMCTL has caused some problems, however, for
those moving forward trying to extend the interface: Extending the
interface even for the 'external' use case now means extending an
HVMOP, which implicitly extends the surface of attack for the
'internal' use case as well.  The result has been that every addition
to this interface has also been controversial.

Settle the controversy once and for all by documenting 1) the purpose
of the altp2m interface, and 2) how to extend it.  In particular:

* Specify that the fully in-guest agent is a target use case

* Specify that all extensions to altp2m functionality should be subops
  of the HVMOP hypercall

* Specify that new subops should be disabled in ALTP2M_mixed mode by
  default, unless specifically evaluated as being useful for the
  'internal' use case.

Hopefully this will allow the altp2m interface to be developed further
without unnecessary controversy.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Konrad Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: Tamas K Lengyel <tamas.lengyel@zentific.com>
CC: Lars Kurth <lars.kurth@citrix.com>

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.

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.

Disabling new subops for the 'internal' use case by default means that
we can add new subops without worrying about making the 'internal' use
case less secure; but if in the future someone makes the case that
they are safe and necessary, we can enable them without having code
duplication.

In any case need to come to an agreement once and for all so that
Tamas and Razvan can do their work without continual arguments over a
mode they're not using.
---
 xen/arch/x86/hvm/hvm.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index e022f5ab0e..90a4be5e86 100644
--- 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().
+ */
 static int do_altp2m_op(
     XEN_GUEST_HANDLE_PARAM(void) arg)
 {
-- 
2.18.0


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

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

[Roger Pau Monné]

On Tue, Jul 10, 2018 at 10:33:22AM +0100, George Dunlap wrote:
> The altp2m functionality was originally envisioned to be used in
> several different configurations, one of which was a single in-guest
> agent that had full operational control of altp2m.  This required the
> single hypercall to be an HVMOP, which is the only type of hypercall
> an HVM guest is allowed to make.

That's not true. HVM guests can use a bunch of hypercalls, like
GNTTABOP, XENMEM, PHYSDEVOP...

> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index e022f5ab0e..90a4be5e86 100644
> --- 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.

Shouldn't you use the existing HVM_PARAM_ALTP2M values in the
enumeration above instead of introducing a new nomenclature?

Roger.

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

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

[Wei Liu]

On Tue, Jul 10, 2018 at 10:33:22AM +0100, George Dunlap wrote:
> The altp2m functionality was originally envisioned to be used in
> several different configurations, one of which was a single in-guest
> agent that had full operational control of altp2m.  This required the
> single hypercall to be an HVMOP, which is the only type of hypercall
> an HVM guest is allowed to make.
> 
> Exposing the altp2m functionality to the guest was controversial at
> the time, but was ultimately accepted.  The fact that altp2m is an
> HVMOP rather than a DOMCTL has caused some problems, however, for
> those moving forward trying to extend the interface: Extending the
> interface even for the 'external' use case now means extending an
> HVMOP, which implicitly extends the surface of attack for the
> 'internal' use case as well.  The result has been that every addition
> to this interface has also been controversial.
> 
> Settle the controversy once and for all by documenting 1) the purpose
> of the altp2m interface, and 2) how to extend it.  In particular:
> 
> * Specify that the fully in-guest agent is a target use case
> 
> * Specify that all extensions to altp2m functionality should be subops
>   of the HVMOP hypercall
> 
> * Specify that new subops should be disabled in ALTP2M_mixed mode by
>   default, unless specifically evaluated as being useful for the
>   'internal' use case.
> 
> Hopefully this will allow the altp2m interface to be developed further
> without unnecessary controversy.
> 
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> ---
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> CC: Konrad Wilk <konrad.wilk@oracle.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> CC: Tamas K Lengyel <tamas.lengyel@zentific.com>
> CC: Lars Kurth <lars.kurth@citrix.com>
> 
> 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.

The second "internal" should be "external".

> 
> 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.
> 
> 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.
> 
> Disabling new subops for the 'internal' use case by default means that
> we can add new subops without worrying about making the 'internal' use
> case less secure; but if in the future someone makes the case that
> they are safe and necessary, we can enable them without having code
> duplication.
> 
> In any case need to come to an agreement once and for all so that
> Tamas and Razvan can do their work without continual arguments over a
> mode they're not using.

Yes this is important.

> ---
>  xen/arch/x86/hvm/hvm.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index e022f5ab0e..90a4be5e86 100644
> --- 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.

I don't understand this argument. There is no risk of code duplication /
fragmentation if the implementation is contained within a function.
Should we choose to split one HVMOP into a DOMCTL and a HVMOP, there is
now two entries to the internal function, each of which with proper
checks, but they will call the same internal function eventually.

I admit I haven't followed the discussion closely.

Wei.

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

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

[Jan Beulich]

>>> On 10.07.18 at 11:33, <george.dunlap@citrix.com> 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.

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

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

Jan



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

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

[George Dunlap]

On 07/10/2018 10:43 AM, Roger Pau Monné wrote:
> On Tue, Jul 10, 2018 at 10:33:22AM +0100, George Dunlap wrote:
>> The altp2m functionality was originally envisioned to be used in
>> several different configurations, one of which was a single in-guest
>> agent that had full operational control of altp2m.  This required the
>> single hypercall to be an HVMOP, which is the only type of hypercall
>> an HVM guest is allowed to make.
> 
> That's not true. HVM guests can use a bunch of hypercalls, like
> GNTTABOP, XENMEM, PHYSDEVOP...

But it can't maken DOMCTLs, which would be the other natural fit.  I'll
see if I can capture that accurately.

> 
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index e022f5ab0e..90a4be5e86 100644
>> --- 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.
> 
> Shouldn't you use the existing HVM_PARAM_ALTP2M values in the
> enumeration above instead of introducing a new nomenclature?

I did think about that.  The problem is that 'mixed' is basically the
wrong label.  Or rather, it covers two different use cases: A more fully
empowered guest agent coordinating with an external agent, and a solo
guest agent doing everything itself.

Replacing 'coordinated' with 'limited' might make sense though.

 -George

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

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

[Ian Jackson]

Wei Liu writes ("Re: [PATCH] hvm/altp2m: Clarify the proper way to extend the altp2m interface"):
> On Tue, Jul 10, 2018 at 10:33:22AM +0100, George Dunlap wrote:
> > + * 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.
> 
> I don't understand this argument. There is no risk of code duplication /
> fragmentation if the implementation is contained within a function.
> Should we choose to split one HVMOP into a DOMCTL and a HVMOP, there is
> now two entries to the internal function, each of which with proper
> checks, but they will call the same internal function eventually.
> 
> I admit I haven't followed the discussion closely.

"each of which with proper checks".  You end up doing some of the
parameter validation twice.  That is very undesirable indeed.

Ian.

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

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

[George Dunlap]

On 07/10/2018 11:01 AM, Jan Beulich wrote:
>>>> On 10.07.18 at 11:33, <george.dunlap@citrix.com> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Wei Liu]

On Tue, Jul 10, 2018 at 11:26:45AM +0100, Ian Jackson wrote:
> Wei Liu writes ("Re: [PATCH] hvm/altp2m: Clarify the proper way to extend the altp2m interface"):
> > On Tue, Jul 10, 2018 at 10:33:22AM +0100, George Dunlap wrote:
> > > + * 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.
> > 
> > I don't understand this argument. There is no risk of code duplication /
> > fragmentation if the implementation is contained within a function.
> > Should we choose to split one HVMOP into a DOMCTL and a HVMOP, there is
> > now two entries to the internal function, each of which with proper
> > checks, but they will call the same internal function eventually.
> > 
> > I admit I haven't followed the discussion closely.
> 
> "each of which with proper checks".  You end up doing some of the
> parameter validation twice.  That is very undesirable indeed.

Fair enough.

Wei.

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

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

[Ian Jackson]

George Dunlap writes ("Re: [PATCH] hvm/altp2m: Clarify the proper way to extend the altp2m interface"):
> On 07/10/2018 11:01 AM, Jan Beulich wrote:
> 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.

We should have a way of remembering the design intent, even if it
hasn't been audited.

SUPPORT.md can deal with the possible problem of people thinking the
non-blacklisted items are fully ready for use.

Ian.

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

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

[George Dunlap]

Forgot to CC BitDefender

On 07/10/2018 10:33 AM, George Dunlap wrote:
> The altp2m functionality was originally envisioned to be used in
> several different configurations, one of which was a single in-guest
> agent that had full operational control of altp2m.  This required the
> single hypercall to be an HVMOP, which is the only type of hypercall
> an HVM guest is allowed to make.
> 
> Exposing the altp2m functionality to the guest was controversial at
> the time, but was ultimately accepted.  The fact that altp2m is an
> HVMOP rather than a DOMCTL has caused some problems, however, for
> those moving forward trying to extend the interface: Extending the
> interface even for the 'external' use case now means extending an
> HVMOP, which implicitly extends the surface of attack for the
> 'internal' use case as well.  The result has been that every addition
> to this interface has also been controversial.
> 
> Settle the controversy once and for all by documenting 1) the purpose
> of the altp2m interface, and 2) how to extend it.  In particular:
> 
> * Specify that the fully in-guest agent is a target use case
> 
> * Specify that all extensions to altp2m functionality should be subops
>   of the HVMOP hypercall
> 
> * Specify that new subops should be disabled in ALTP2M_mixed mode by
>   default, unless specifically evaluated as being useful for the
>   'internal' use case.
> 
> Hopefully this will allow the altp2m interface to be developed further
> without unnecessary controversy.
> 
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> ---
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> CC: Konrad Wilk <konrad.wilk@oracle.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> CC: Tamas K Lengyel <tamas.lengyel@zentific.com>
> CC: Lars Kurth <lars.kurth@citrix.com>
> 
> 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.
> 
> 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.
> 
> Disabling new subops for the 'internal' use case by default means that
> we can add new subops without worrying about making the 'internal' use
> case less secure; but if in the future someone makes the case that
> they are safe and necessary, we can enable them without having code
> duplication.
> 
> In any case need to come to an agreement once and for all so that
> Tamas and Razvan can do their work without continual arguments over a
> mode they're not using.
> ---
>  xen/arch/x86/hvm/hvm.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index e022f5ab0e..90a4be5e86 100644
> --- 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().
> + */
>  static int do_altp2m_op(
>      XEN_GUEST_HANDLE_PARAM(void) arg)
>  {
> 


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

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

[George Dunlap]

On 07/10/2018 11:32 AM, Ian Jackson wrote:
> George Dunlap writes ("Re: [PATCH] hvm/altp2m: Clarify the proper way to extend the altp2m interface"):
>> On 07/10/2018 11:01 AM, Jan Beulich wrote:
>> 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.
> 
> We should have a way of remembering the design intent, even if it
> hasn't been audited.
> 
> SUPPORT.md can deal with the possible problem of people thinking the
> non-blacklisted items are fully ready for use.

I think SUPPORT.md should primarily be for end users to determine
whether they should use an interface or not.  A technical discussion
about exactly to what degree an interface has (or hasn't) been vetted is
more towards a developer who may be looking to get things into a
supported state, and so should probably be in the code somewhere.

 -George

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

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

[Ian Jackson]

George Dunlap writes ("Re: [PATCH] hvm/altp2m: Clarify the proper way to extend the altp2m interface"):
> On 07/10/2018 11:32 AM, Ian Jackson wrote:
> > We should have a way of remembering the design intent, even if it
> > hasn't been audited.
> > 
> > SUPPORT.md can deal with the possible problem of people thinking the
> > non-blacklisted items are fully ready for use.
> 
> I think SUPPORT.md should primarily be for end users to determine
> whether they should use an interface or not.  A technical discussion
> about exactly to what degree an interface has (or hasn't) been vetted is
> more towards a developer who may be looking to get things into a
> supported state, and so should probably be in the code somewhere.

I agree with that.  But currently I think none of it has been
audited.  If there is any question of doubt we can leave a comment in
one place referenceing the SUPPORT.md status and saying `one reason is
that none of this stuff has been audited'.

Ian.

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

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

[George Dunlap]

On 07/10/2018 11:59 AM, Ian Jackson wrote:
> George Dunlap writes ("Re: [PATCH] hvm/altp2m: Clarify the proper way to extend the altp2m interface"):
>> On 07/10/2018 11:32 AM, Ian Jackson wrote:
>>> We should have a way of remembering the design intent, even if it
>>> hasn't been audited.
>>>
>>> SUPPORT.md can deal with the possible problem of people thinking the
>>> non-blacklisted items are fully ready for use.
>>
>> I think SUPPORT.md should primarily be for end users to determine
>> whether they should use an interface or not.  A technical discussion
>> about exactly to what degree an interface has (or hasn't) been vetted is
>> more towards a developer who may be looking to get things into a
>> supported state, and so should probably be in the code somewhere.
> 
> I agree with that.  But currently I think none of it has been
> audited.  If there is any question of doubt we can leave a comment in
> one place referenceing the SUPPORT.md status and saying `one reason is
> that none of this stuff has been audited'.

I would assume that for the original interface, the authors at least did
some thinking about whether exposing each bit was safe or not (whether
this rises to the level of 'audit' is a different question).  But in the
mean time, other extensions have been made, which have generally not
been thought about so much.

OTOH, someone picking up this use case will want to do their own
analysis of all the code anyway; so maybe making the distinction between
"was in the original design" and "was added later" isn't worth the effort.

Basically -- I suggested the idea of a blacklist for new subops as
something to make Jan less worried about security creep.  If he wants
it, let's have it.  Otherwise, let's not bother.

 -George

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

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

[Jan Beulich]

>>> On 10.07.18 at 12:30, <george.dunlap@citrix.com> wrote:
> On 07/10/2018 11:01 AM, Jan Beulich wrote:
>>>>> On 10.07.18 at 11:33, <george.dunlap@citrix.com> 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.

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

Jan


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

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

[George Dunlap]

On 07/10/2018 12:58 PM, Jan Beulich wrote:
>>>> On 10.07.18 at 12:30, <george.dunlap@citrix.com> wrote:
>> On 07/10/2018 11:01 AM, Jan Beulich wrote:
>>>>>> On 10.07.18 at 11:33, <george.dunlap@citrix.com> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel