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

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

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.


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

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

v/r,
dps