Re: [PATCH 3/6] xsm: enabling xsm to always be included

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.06.2021 22:27, Daniel P. Smith wrote:
> On 6/18/21 8:26 AM, Jan Beulich wrote:
>> On 18.06.2021 01:39, Daniel P. Smith wrote:
>>> The only difference between !CONFIG_XSM and CONFIG_XSM with 
>>> !CONFIG_XSM_SILO and !CONFIG_XSM_FLASK
>>> is whether the XSM hooks in dummy.h are called as static inline functions 
>>> or as function
>>> pointers to static functions. As such this commit,
>>>  * eliminates CONFIG_XSM
>>
>> Following from what Andrew has said (including him mentioning your
>> changing of certain Kconfig option defaults), I'm not convinced this is
>> a good move. This still ought to serve as the overall XSM-yes-or-no
>> setting. If internally you make said two variants match in behavior,
>> that's a different thing.
> 
> Apologies that I did not express this clearly. What I was attempting to
> say is the fact of the matter is that there is no logical behavior
> difference between "XSM no" and "XSM yes with dummy policy". The only
> difference is the mechanics of how the dummy functions get called.
> Specifically via macro magic the dummy functions are either flipped into
> static inline declarations that are then included into the code where
> they are invoked or the macro magic has them ending up in the dummy.c
> XSM module where they are wrapped in macro generated functions that are
> set as the functions in the dummy xsm_ops structure. Thus it is always
> the same logic being invoked, it is just mechanics of how you get to the
> logic.

That's what I understood, really. What I dislike is the inline functions
going away in what we currently call !XSM.

>>>  * introduces CONFIG_XSM_EVTCHN_LABELING as replacement for enabling event 
>>> channel labels
>>
>> Is this mode needed as separate functionality at all? Nothing defines
>> XSM_NEED_GENERIC_EVTCHN_SSID anywhere. _If_ XSM went away as a separate
>> setting, then imo this one should go away as well.
>>
>>> --- a/xen/common/Kconfig
>>> +++ b/xen/common/Kconfig
>>> @@ -197,22 +197,33 @@ config XENOPROF
>>>  
>>>       If unsure, say Y.
>>>  
>>> -config XSM
>>> -   bool "Xen Security Modules support"
>>> -   default ARM
>>> -   ---help---
>>> -     Enables the security framework known as Xen Security Modules which
>>> -     allows administrators fine-grained control over a Xen domain and
>>> -     its capabilities by defining permissible interactions between domains,
>>> -     the hypervisor itself, and related resources such as memory and
>>> -     devices.
>>> +menu "Xen Security Modules"
>>>  
>>> -     If unsure, say N.
>>> +choice
>>> +   prompt "Default XSM module"
>>> +   default XSM_SILO_DEFAULT if XSM_SILO && ARM
>>> +   default XSM_FLASK_DEFAULT if XSM_FLASK
>>> +   default XSM_SILO_DEFAULT if XSM_SILO
>>> +   default XSM_DUMMY_DEFAULT
>>> +   config XSM_DUMMY_DEFAULT
>>> +           bool "Match non-XSM behavior"
>>> +   config XSM_FLASK_DEFAULT
>>> +           bool "FLux Advanced Security Kernel" if XSM_FLASK
>>> +   config XSM_SILO_DEFAULT
>>> +           bool "SILO" if XSM_SILO
>>> +endchoice
>>
>> This did live after the individual options it depends on for a reason,
>> and you don't say anywhere why you need to move it up. The way you
>> have it, with the default command line kconfig tool, users will be
>> presented with dependent options before having chosen the settings of
>> the dependency ones. That's because this tool, to a degree, moves
>> linearly through the options it has parsed.
> 
> Yes, this is specifically why I moved it up. Clearly we have different
> approaches to how we like to interact with configurations, which is not
> bad thing. I personally found it awkward the other way but can easily
> move it back.

I'm having a hard time seeing how presenting dependent options ahead
of their dependencies can be a good thing: The user will have made
their "choice" here, while the availability of the individual settings
then may change in case the depended upon options change from their
defaults upon the user reacting to those prompts.

>>> @@ -261,25 +271,12 @@ config XSM_SILO
>>>  
>>>       If unsure, say Y.
>>>  
>>> -choice
>>> -   prompt "Default XSM implementation"
>>> -   depends on XSM
>>> -   default XSM_SILO_DEFAULT if XSM_SILO && ARM
>>> -   default XSM_FLASK_DEFAULT if XSM_FLASK
>>> -   default XSM_SILO_DEFAULT if XSM_SILO
>>> -   default XSM_DUMMY_DEFAULT
>>> -   config XSM_DUMMY_DEFAULT
>>> -           bool "Match non-XSM behavior"
>>> -   config XSM_FLASK_DEFAULT
>>> -           bool "FLux Advanced Security Kernel" if XSM_FLASK
>>> -   config XSM_SILO_DEFAULT
>>> -           bool "SILO" if XSM_SILO
>>> -endchoice
>>> +endmenu
>>>  
>>>  config LATE_HWDOM
>>>     bool "Dedicated hardware domain"
>>>     default n
>>> -   depends on XSM && X86
>>> +   depends on XSM_FLASK && X86
>>
>> I don't think this is a compatible change. I'm not going to exclude that
>> this is how it was meant, but as it stands LATE_HWDOM right now doesn't
>> really require FLASK, and could e.g. also go with SILO or dummy. If you
>> _mean_ to change this, then your description needs to say so (and ideally
>> it would then be split out, so - if this is actually a bug - it could
>> also be backported).
> 
> Actually this is the root cause that started all of this work. If you
> want to get technical, LATE_HWDOM does not rely on XSM at all. The issue
> is that you cannot use it as it was originally intended, to run Xen
> without a classic dom0 while still having all existing capabilities.
> Specifically the hardware domain does not have the ability to assign the
> pass-through devices for which it is in control. This is were Flask
> comes in to enable assigning specific privileges to labels and then
> constructing domains with those labels, In particular it grants the
> ability to do pass-through assignment to the label assigned to the
> hardware domain. With the upcoming XSM-Roles patch set, these privileges
> are assigned to roles and it becomes possible to assign the necessary
> roles to the hardware domain.

In which case this needs spelling out in the description, to justify 
the change in behavior (which I'm not sure I really follow / agree with
just yet).

Jan