Re: [PATCH 5/5] xen/xsm: Address hypercall ABI problems

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.11.2021 00:14, Andrew Cooper wrote:
> On 08/11/2021 09:50, Jan Beulich wrote:
>> On 05.11.2021 14:55, Andrew Cooper wrote:
>>> Currently, __HYPERVISOR_xsm_op enters xsm_ops.do_{xsm,compat}_op() which 
>>> means
>>> that if any other XSM module registers a handler, we'll break the hypercall
>>> ABI totally by having the behaviour depend on the selected XSM module.
>>>
>>> There are 2 options:
>>>   1) Rename __HYPERVISOR_xsm_op to __HYPERVISOR_flask_op.  If another XSM
>>>      module wants hypercalls, they'll have to introduce a new top-level
>>>      hypercall.
>>>   2) Make the current flask_op() be common, and require new hypercalls to 
>>> fit
>>>      compatibly with xen_flask_op_t.  This can be done fairly easily by
>>>      subdividing the .cmd space.
>>>
>>> In this patch, we implement option 2.
>>>
>>> Move the stub {do,compat}_xsm_op() implementation into a new xsm_op.c so we
>>> can more easily do multi-inclusion compat magic.  Also add a new private.h,
>>> because flask/hook.c's local declaration of {do,compat}_flask_op() wasn't
>>> remotely safe.
>>>
>>> The top level now dispatches into {do,compat}_flask_op() based on op.cmd, 
>>> and
>>> handles the primary copy in/out.
>> I'm not convinced this is the only reasonable way of implementing 2).
>> I could also see number space to be separated in different ways, ...
>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>> ---
>>> CC: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
>>> CC: Daniel Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
>>>
>>> Only lightly tested.  Slightly RFC.  There are several things which aren't
>>> great, but probably want addressing in due course.
>>>
>>>   1) The public headers probably want to lose the flask name (in a 
>>> compatible
>>>      way), now that the hypercall is common.  This probably wants to be
>>>      combined with no longer taking a void handle.
>> ... leaving per-module public headers and perhaps merely adding an
>> abstracting
>>
>> struct xen_xsm_op_t {
>>      uint32_t op;
>>      ... /* placeholder */
>> };
>>
>> or (making explicit one possible variant of number space splitting)
>>
>> union xen_xsm_op_t {
>>      uint32_t op;
>>      struct {
>>          uint16_t cmd;
>>          uint16_t mod;
>>      } u;
>>      ... /* placeholder */
>> };
>>
>> in, say, a new public/xsm.h.
> 
> That doesn't work.  The ABI is fixed at sizeof(xen_flask_op_t) for all 
> other modules, because a caller which doesn't know which module is in 
> use must not have Xen over-read/write the object passed while it's 
> trying to figure things out.

Well, imo figuring out which module is in use should be via a sysctl.
Then there would be no restrictions by one module's interface
definitions on other modules.

>> As a result I consider this change either going too far (because of
>> not knowing future needs) or not far enough (by e.g. leaving
>> do_xsm_op() to use xen_flask_op_t.
> 
> Well - what it does is prevent someone from breaking the ABI with 
> literally this patch
> 
> diff --git a/xen/xsm/silo.c b/xen/xsm/silo.c
> index 3550dded7b4e..36b82fd3bd3e 100644
> --- a/xen/xsm/silo.c
> +++ b/xen/xsm/silo.c
> @@ -100,6 +100,11 @@ static int silo_argo_send(const struct domain *d1, 
> const struct domain *d2)
> 
>   #endif
> 
> +static long silo_do_xsm_op(XEN_GUEST_HANDLE_PARAM(void) op)
> +{
> +    /* ... */
> +}
> +
>   static const struct xsm_ops __initconstrel silo_xsm_ops = {
>       .evtchn_unbound = silo_evtchn_unbound,
>       .evtchn_interdomain = silo_evtchn_interdomain,
> @@ -110,6 +115,7 @@ static const struct xsm_ops __initconstrel 
> silo_xsm_ops = {
>       .argo_register_single_source = silo_argo_register_single_source,
>       .argo_send = silo_argo_send,
>   #endif
> +    .do_xsm_op = silo_do_xsm_op,
>   };
> 
>   const struct xsm_ops *__init silo_init(void)

So I'm afraid I don't see any ABI breakage here. Provided of course
silo_do_xsm_op() avoids the FLASK_* number space and uses a struct
layout compatible with the head of struct xen_flask_op.

Jan