Re: [PATCH v2 06/10] xsm: enable xsm to always be included

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.07.2021 22:32, Daniel P. Smith wrote:
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -200,23 +200,20 @@ 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"

I remain unconvinced of the removal of this top level option. I don't
view my concern on code size / performance as "unreasonable" (as Andrew
did call it) when comparing with the current !XSM configuration, and I
also remain to be convinced of people who had to simply answer 'n' to
the XSM kconfig prompt being happy to make an informed decision for all
the (previously sub-)options that they will now be prompted for. As
said before - it is one thing to re-work what exactly !XSM means
internally (and the conversion away from inline functions may very well
be a win; we simply don't know without you showing e.g. bloat-o-meter
like data). It is another to require in-depth knowledge to configure
Xen that previously wasn't required.

Keeping the top-level option would then also make it unnecessary to
alter some of the (prior sub-)options' defaults.

As to Andrew previously having said

"There is no such thing as !XSM even in staging right now.

 All over Xen, we have calls to xsm_*() functions which, even in the !XSM
 case, contain a non-trivial security policy."

Yes, this is one possible viewpoint, which I simply do not share. I view
the xsm_*() calls in the !XSM case as simple surrogates, not anything that
deserves the name "module". This is actually supported by the help text
of the XSM Kconfig option saying "which allows administrators fine-grained
control": There's nothing fine-grained with what currently is !XSM.

> -       If unsure, say N.
> +config XSM_EVTCHN_LABELING
> +     bool "Enables security labeling of event channels"

Does this really need to have a prompt, instead of simply being
selected by the module(s) needing it? IOW what do users gain when they
answer y here but n for XSM_FLASK?

Furthermore, if the prompt is to remain, then I'll have to again
raise the question of ordering of options: This way, via e.g. the
"syncconfig" or "oldconfig" targets, I'd be asked for the setting here
when, if I'd then also set XSM_FLASK=y, the question was in vein - the
option will be set to y anyway.

> +     default n

May I ask to omit "default n" lines. I'm unaware of anything good they
do.

> @@ -265,24 +262,26 @@ config XSM_SILO
>         If unsure, say Y.
>  
>  choice
> -     prompt "Default XSM implementation"
> -     depends on XSM
> +     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"
> +             bool "Classic Dom0 behavior"
>       config XSM_FLASK_DEFAULT
>               bool "FLux Advanced Security Kernel" if XSM_FLASK
>       config XSM_SILO_DEFAULT
>               bool "SILO" if XSM_SILO
> +
>  endchoice

Nit: I see only two consistent formatting options here: Either there is
a blank line ahead of "endchoice" and after "choice", or there are none
in both places. I don't mind which one it is, but I do mind
inconsistencies getting introduced.

Jan