Re: [PATCH v3 7/7] xsm: removing facade that XSM can be enabled/disabled

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.08.2021 16:06, Daniel P. Smith wrote:
> The XSM facilities are always in use by Xen with the facade of being able to
> turn XSM on and off. This option is in fact about allowing the selection of
> which policies are available and which are used at runtime.  To provide this
> facade a complicated serious of #ifdef's are used to selective include

Nit: It took me a moment to realize that the sentence reads oddly because
you likely mean "series", not "serious".

> different headers or portions of headers. This series of #ifdef gyrations
> switches between two different versions of the XSM hook interfaces and their
> respective backing implementation.  All of this is done to provide a minimal
> size/performance optimization for when alternative policies are disabled.
> 
> To unwind the #ifdef gyrations a series of changes were necessary,
>     * replace CONFIG_XSM with XSM_CONFIGURABLE to allow visibility of
>       selecting alternate XSM policy modules to those that require it
>     * adjusted CONFIG_XSM_SILO, CONFIG_XSM_FLASK, and the default module
>       selection to sensible defaults
>     * collapsed the "dummy/defualt" XSM interface and implementation with the
>       "multiple policy" interface to provide a single inlined implementation
>       that attempts to use a registered hook and falls back to the check from
>       the dummy implementation
>     * the collapse to a single interface broke code relying on the alternate
>       interface, specifically SILO, this was reworked to remove the
>       indirection/abstraction making SILO explicit in its access control
>       decisions
>     * with the change of the XSM hooks to fall back to enforcing the dummy
>       policy, it is no longer necessary to fill NULL entries in the struct
>       xsm_ops returned by an XSM module's init

It would be nice if some of this could be split. Is this really close to
impossible?

> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -200,23 +200,15 @@ 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.
> -
> -       If unsure, say N.
> +config XSM_CONFIGURABLE
> +    bool "Enable Configuring Xen Security Modules"

Is there a reason to change not only the prompt, but also the name of
the Kconfig setting? This alone is the reason for some otherwise
unnecessary code churn.

Also please correct indentation here.

>  config XSM_FLASK
> -     def_bool y
> -     prompt "FLux Advanced Security Kernel support"
> -     depends on XSM
> -     ---help---
> +     bool "FLux Advanced Security Kernel support"
> +     default n

I don't understand this change in default (and as an aside, a default
of "n" doesn't need spelling out): In the description you say "adjusted
CONFIG_XSM_SILO, CONFIG_XSM_FLASK, and the default module selection to
sensible defaults". If that's to describe this change, then I'm afraid
I don't see why defaulting to "n" is more sensible once the person
configuring Xen has chosen the configure XSM's (or XSM_CONFIGURABLE's)
sub-options. If that's unrelated to the change here, then I'm afraid
I'm missing justification altogether. (Same for SILO then.)

> +     depends on XSM_CONFIGURABLE
> +     select XSM_EVTCHN_LABELING

Neither this nor any prior patch introduces an option of this name,
and there's also none in the present tree. All afaics; I may have
overlooked something or typo-ed a "grep" command.

> @@ -265,14 +258,14 @@ 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
> +     depends on XSM_CONFIGURABLE

With the larger set of "default" lines I'd like to suggest to keep
"depends on" ahead of them.

> @@ -282,7 +275,7 @@ endchoice
>  config LATE_HWDOM
>       bool "Dedicated hardware domain"
>       default n
> -     depends on XSM && X86
> +     depends on XSM_FLASK && X86

This change is not mentioned or justified in the description. In fact
I think it is unrelated to the change here and hence would want breaking
out.

>       ---help---

As you're changing these elsewhere, any chance of you also changing
this one to just "help"?

> --- a/xen/include/xsm/xsm.h
> +++ b/xen/include/xsm/xsm.h
> @@ -19,545 +19,1023 @@
>  #include <xen/sched.h>
>  #include <xen/multiboot.h>
>  #include <xsm/xsm-core.h>
> -
> -#ifdef CONFIG_XSM
> +#include <xsm/dummy.h>
> +#include <public/version.h>
>  
>  extern struct xsm_ops xsm_ops;
>  
> -static inline void xsm_security_domaininfo (struct domain *d,
> -                                        struct xen_domctl_getdomaininfo 
> *info)
> +static inline void xsm_security_domaininfo(
> +    struct domain *d,
> +    struct xen_domctl_getdomaininfo *info)
>  {
> -    alternative_vcall(xsm_ops.security_domaininfo, d, info);
> +    if ( xsm_ops.security_domaininfo )
> +        alternative_vcall(xsm_ops.security_domaininfo, d, info);

Here and everywhere else, when !XSM_CONFIGURABLE you now needlessly
force NULL checks to occur which are never going to be true. There's
then also the dead indirect call and the associated patching data. I
think this wants hiding in another pair of wrappers, which simply
expand to nothing when !XSM_CONFIGURABLE - perhaps xsm_vcall() and
xsm_call().

>  }
>  
> -static inline int xsm_domain_create (xsm_default_t def, struct domain *d, 
> u32 ssidref)
> +static inline int xsm_domain_create(xsm_default_t action, struct domain *d,
> +                                    u32 ssidref)

It would be nice if you kept on converting u32 -> uint32_t as you did
in earlier patches.

>  {
> -    return alternative_call(xsm_ops.domain_create, d, ssidref);
> +    if ( xsm_ops.domain_create )
> +        return alternative_call(xsm_ops.domain_create, d, ssidref);
> +
> +    XSM_ASSERT_ACTION(XSM_HOOK);

Any reason not to put these assertions first in the functions?

> +    return xsm_default_action(action, current->domain, d);
>  }

Since

static inline int xsm_domain_create(xsm_default_t action, struct domain *d,
                                    uint32_t ssidref)
{
    XSM_ASSERT_ACTION(XSM_HOOK);
    return xsm_call(xsm_ops.domain_create, d, ssidref);
    return xsm_default_action(action, current->domain, d);
}

won't work, and since integrating "return" into xsm_call() also would't
yield a sufficiently clear result:

static inline int xsm_domain_create(xsm_default_t action, struct domain *d,
                                    uint32_t ssidref)
{
    XSM_ASSERT_ACTION(XSM_HOOK);
    xsm_call(xsm_ops.domain_create, d, ssidref);
    return xsm_default_action(action, current->domain, d);
}

(as control flow would be unobvious), the xsm_default_action() invocation
likely would also need integrating into xsm_call() then.

Unless there's some obstacle, this would eliminate a whole lot of code
redundancy.

> -static inline int xsm_set_target (xsm_default_t def, struct domain *d, 
> struct domain *e)
> +static inline int xsm_set_target(xsm_default_t action, struct domain *d,
> +                                 struct domain *e)
>  {
> -    return alternative_call(xsm_ops.set_target, d, e);
> +    if ( xsm_ops.set_target )
> +        return alternative_call(xsm_ops.set_target, d, e);
> +
> +    XSM_ASSERT_ACTION(XSM_HOOK);
> +    return xsm_default_action(action, current->domain, NULL);
>  }

While benign because xsm_default_action() does nothing for XSM_HOOK, I
think there's an inconsistency here which rather wants correcting (in
a prereq patch): The default hook should have been passed consistent
arguments, no matter whether used because of !XSM or because of the
module in use left the hook unset.

Of course such anomalies are much easier to notice (outside of review
of patches introducing such) with you now placing both invocations
next to each other.

> -static inline int xsm_evtchn_interdomain (xsm_default_t def, struct domain 
> *d1,
> -                struct evtchn *chan1, struct domain *d2, struct evtchn 
> *chan2)
> +static inline int xsm_evtchn_interdomain(xsm_default_t action,
> +                                         struct domain *d1,
> +                                         struct evtchn *chan1,
> +                                         struct domain *d2,
> +                                         struct evtchn *chan2)
>  {
> -    return alternative_call(xsm_ops.evtchn_interdomain, d1, chan1, d2, 
> chan2);
> +    if ( xsm_ops.evtchn_interdomain )
> +        return alternative_call(xsm_ops.evtchn_interdomain, d1, chan1, d2,
> +                                chan2);
> +
> +    XSM_ASSERT_ACTION(XSM_HOOK);
> +    return xsm_default_action(action, d1, d2);
>  }

There's another anomaly here: The first argument to xsm_default_action()
typically is current->domain in similar functions. Here d1 gets passed in
despite always being current->domain. I think the unnecessary parameter
wants dropping (again in a prereq patch)) from the wrapper and hook, to
avoid giving the wrong impression of both domains potentially being remote
ones.

> -static inline int xsm_evtchn_send (xsm_default_t def, struct domain *d, 
> struct evtchn *chn)
> +static inline int xsm_evtchn_send(xsm_default_t action, struct domain *d,
> +                                  struct evtchn *chn)
>  {
> -    return alternative_call(xsm_ops.evtchn_send, d, chn);
> +    if ( xsm_ops.evtchn_send )
> +        return alternative_call(xsm_ops.evtchn_send, d, chn);
> +
> +    XSM_ASSERT_ACTION(XSM_HOOK);
> +    return xsm_default_action(action, d, NULL);

This again looks wrong (and again is benign only because XSM_HOOK means
xsm_default_action() ignores the other function parameters), wanting to
follow the usual

    return xsm_default_action(action, current->domain, d);

pattern instead.

> -static inline int xsm_evtchn_reset (xsm_default_t def, struct domain *d1, 
> struct domain *d2)
> +static inline int xsm_evtchn_reset(xsm_default_t action, struct domain *d1,
> +                                   struct domain *d2)
>  {
> -    return alternative_call(xsm_ops.evtchn_reset, d1, d2);
> +    if ( xsm_ops.evtchn_reset )
> +        return alternative_call(xsm_ops.evtchn_reset, d1, d2);
> +
> +    XSM_ASSERT_ACTION(XSM_TARGET);
> +    return xsm_default_action(action, d1, d2);

See xsm_evtchn_interdomain() above.

> -static inline int xsm_grant_mapref (xsm_default_t def, struct domain *d1, 
> struct domain *d2,
> -                                                                uint32_t 
> flags)
> +static inline int xsm_grant_mapref(xsm_default_t action, struct domain *d1,
> +                                   struct domain *d2, uint32_t flags)
>  {
> -    return alternative_call(xsm_ops.grant_mapref, d1, d2, flags);
> +    if ( xsm_ops.grant_mapref )
> +        return alternative_call(xsm_ops.grant_mapref, d1, d2, flags);
> +
> +    XSM_ASSERT_ACTION(XSM_HOOK);
> +    return xsm_default_action(action, d1, d2);

Again (more similar grant ones follow).

> -static inline int xsm_memory_adjust_reservation (xsm_default_t def, struct 
> domain *d1, struct
> -                                                                    domain 
> *d2)
> +static inline int xsm_memory_adjust_reservation(xsm_default_t action,
> +                                                struct domain *d1,
> +                                                struct domain *d2)
>  {
> -    return alternative_call(xsm_ops.memory_adjust_reservation, d1, d2);
> +    if ( xsm_ops.memory_adjust_reservation )
> +        return alternative_call(xsm_ops.memory_adjust_reservation, d1, d2);
> +
> +    XSM_ASSERT_ACTION(XSM_TARGET);
> +    return xsm_default_action(action, d1, d2);

Again (more similar memory ones follow).

> -static inline int xsm_memory_pin_page(xsm_default_t def, struct domain *d1, 
> struct domain *d2,
> -                                      struct page_info *page)
> +static inline int xsm_memory_pin_page(xsm_default_t action, struct domain 
> *d1,
> +                                      struct domain *d2, struct page_info 
> *page)
>  {
> -    return alternative_call(xsm_ops.memory_pin_page, d1, d2, page);
> +    if ( xsm_ops.memory_pin_page )
> +        return alternative_call(xsm_ops.memory_pin_page, d1, d2, page);
> +
> +    XSM_ASSERT_ACTION(XSM_HOOK);
> +    return xsm_default_action(action, d1, d2);

This one has the same issue, but is more interesting: There's no
similar hook/check for unpinning a page (nor does the same check
get re-used there). Plus it's x86 (more precisely PV) specific.

> -static inline int xsm_map_gmfn_foreign (xsm_default_t def, struct domain *d, 
> struct domain *t)
> +static inline int xsm_map_gmfn_foreign(xsm_default_t action, struct domain 
> *d,
> +                                       struct domain *t)
>  {
> -    return alternative_call(xsm_ops.map_gmfn_foreign, d, t);
> +    if ( xsm_ops.map_gmfn_foreign )
> +        return alternative_call(xsm_ops.map_gmfn_foreign, d, t);
> +
> +    XSM_ASSERT_ACTION(XSM_TARGET);
> +    return xsm_default_action(action, d, t);

This one is also interesting: There's no check at all here that
current->domain has any permissions towards d or t. Interestingly
even flask_map_gmfn_foreign() doesn't check this.

> -static inline int xsm_console_io (xsm_default_t def, struct domain *d, int 
> cmd)
> +static inline int xsm_console_io(xsm_default_t action, struct domain *d, int 
> cmd)
>  {
> -    return alternative_call(xsm_ops.console_io, d, cmd);
> +    if ( xsm_ops.console_io )
> +        return alternative_call(xsm_ops.console_io, d, cmd);
> +
> +    XSM_ASSERT_ACTION(XSM_OTHER);
> +    if ( d->is_console )
> +        return xsm_default_action(XSM_HOOK, d, NULL);
> +#ifdef CONFIG_VERBOSE_DEBUG
> +    if ( cmd == CONSOLEIO_write )
> +        return xsm_default_action(XSM_HOOK, d, NULL);
> +#endif
> +    return xsm_default_action(XSM_PRIV, d, NULL);

Same implication of d == current->domain here again. I guess I'll
stop enumerating further ones.

> -static inline int xsm_pci_config_permission (xsm_default_t def, struct 
> domain *d, uint32_t machine_bdf, uint16_t start, uint16_t end, uint8_t access)
> +static inline int xsm_pci_config_permission(xsm_default_t action,
> +                                            struct domain *d,
> +                                            uint32_t machine_bdf,
> +                                            uint16_t start,
> +                                            uint16_t end,
> +                                            uint8_t access)
>  {
> -    return alternative_call(xsm_ops.pci_config_permission, d, machine_bdf, 
> start, end, access);
> +    if ( xsm_ops.pci_config_permission )
> +        return alternative_call(xsm_ops.pci_config_permission, d, 
> machine_bdf, start, end, access);

Nit: Line length.

> -static inline int xsm_pmu_op (xsm_default_t def, struct domain *d, unsigned 
> int op)
> +static inline int xsm_pmu_op(xsm_default_t action, struct domain *d,
> +                             unsigned int op)
>  {
> -    return alternative_call(xsm_ops.pmu_op, d, op);
> +    if ( xsm_ops.pmu_op )
> +        return alternative_call(xsm_ops.pmu_op, d, op);
> +
> +    XSM_ASSERT_ACTION(XSM_OTHER);
> +    switch ( op )
> +    {
> +    case XENPMU_init:
> +    case XENPMU_finish:
> +    case XENPMU_lvtpc_set:
> +    case XENPMU_flush:
> +        return xsm_default_action(XSM_HOOK, d, current->domain);
> +    default:
> +        return xsm_default_action(XSM_PRIV, d, current->domain);

Urgh - isn't this the wrong way round? (Luckily vPMU isn't security
supported, so no XSA would be needed.)

> --- a/xen/xsm/Makefile
> +++ b/xen/xsm/Makefile
> @@ -1,6 +1,5 @@
>  obj-y += xsm_core.o
> -obj-$(CONFIG_XSM) += xsm_policy.o
> -obj-$(CONFIG_XSM) += dummy.o
> +obj-y += xsm_policy.o

Why would this now need compiling in all cases?

> --- a/xen/xsm/silo.c
> +++ b/xen/xsm/silo.c
> @@ -17,6 +17,7 @@
>   * You should have received a copy of the GNU General Public License along 
> with
>   * this program; If not, see <http://www.gnu.org/licenses/>.
>   */
> +#include <xsm/xsm-core.h>
>  #include <xsm/dummy.h>

As already mentioned elsewhere - where possible please arrange #include-s
alphabetically.

> @@ -124,16 +122,12 @@ static int __init xsm_core_init(const void 
> *policy_buffer, size_t policy_size)
>          break;
>      }
>  
> -    /*
> -     * This handles three cases,
> -     *   - dummy policy module was selected
> -     *   - a policy module does not provide all handlers
> -     *   - a policy module failed to init
> -     */
> -    xsm_fixup_ops(&xsm_ops);
> -
> -    if ( xsm_ops_registered != XSM_OPS_REGISTERED )
> +    if ( xsm_ops_registered != XSM_OPS_REGISTERED ) {

Nit (style): Brace goes on its own line.

Jan