On 12/07/2021 21:32, Daniel P. Smith wrote:
> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
> index ad3cddbf7d..a8805f514b 100644
> --- a/xen/include/xsm/xsm.h
> +++ b/xen/include/xsm/xsm.h
> @@ -747,16 +747,14 @@ extern int xsm_dt_policy_init(void **policy_buffer,
> size_t *policy_size);
> extern bool has_xsm_magic(paddr_t);
> #endif
>
> -extern int register_xsm(struct xsm_operations *ops);
> -
> -extern struct xsm_operations dummy_xsm_ops;
> extern void xsm_fixup_ops(struct xsm_operations *ops);
>
> #ifdef CONFIG_XSM_FLASK
> -extern void flask_init(const void *policy_buffer, size_t policy_size);
> +extern struct xsm_operations *flask_init(const void *policy_buffer, size_t
> policy_size);
> #else
> -static inline void flask_init(const void *policy_buffer, size_t policy_size)
> +static inline struct xsm_operations *flask_init(const void *policy_buffer,
> size_t policy_size)
> {
> + return NULL;
> }
> #endif
As you touch almost every current user of xsm_operations and introduce
quite a few more, can I recommend taking the opportunity to shorten the
name to struct xsm_ops.
> diff --git a/xen/xsm/flask/flask_op.c b/xen/xsm/flask/flask_op.c
> index 01e52138a1..32e079d676 100644
> --- a/xen/xsm/flask/flask_op.c
> +++ b/xen/xsm/flask/flask_op.c
> @@ -226,6 +226,7 @@ static int flask_security_sid(struct
> xen_flask_sid_context *arg)
> static int flask_disable(void)
> {
> static int flask_disabled = 0;
> + struct xsm_operations default_ops;
>
> if ( ss_initialized )
> {
> @@ -244,7 +245,8 @@ static int flask_disable(void)
> flask_disabled = 1;
>
> /* Reset xsm_ops to the original module. */
> - xsm_ops = &dummy_xsm_ops;
> + xsm_fixup_ops(&default_ops);
> + xsm_ops = default_ops;
>
> return 0;
> }
These two hunks will disappear when patch 3 is reordered with respect to
this one.
... which is good because you've just pointed xsm_ops at a
soon-to-be-out-of-scope local variable on the stack.
> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
> index f1a1217c98..a3a88aa8ed 100644
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -1883,7 +1883,8 @@ static struct xsm_operations flask_ops = {
> #endif
> };
flask and silo ops should become:
static const struct xsm_ops __initconst {flask,silo}_ops = {
as they're neither modified, nor needed after init, following this change.
>
> -void __init flask_init(const void *policy_buffer, size_t policy_size)
> +struct xsm_operations __init *flask_init(const void *policy_buffer,
> + size_t policy_size)
struct xsm_ops *__init flask_init(...)
Otherwise you've got the __init in the middle of the return type, and
some compilers are more picky than others.
> @@ -1923,6 +1921,9 @@ void __init flask_init(const void *policy_buffer,
> size_t policy_size)
> printk(XENLOG_INFO "Flask: Starting in enforcing mode.\n");
> else
> printk(XENLOG_INFO "Flask: Starting in permissive mode.\n");
> +
> + return &flask_ops;
> +
Stray newline.
> }
>
> /*
> diff --git a/xen/xsm/silo.c b/xen/xsm/silo.c
> index fc2ca5cd2d..808350f122 100644
> --- a/xen/xsm/silo.c
> +++ b/xen/xsm/silo.c
> @@ -112,12 +112,11 @@ static struct xsm_operations silo_xsm_ops = {
> #endif
> };
>
> -void __init silo_init(void)
> +struct xsm_operations __init *silo_init(void)
Same here as with flask.
> diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c
> index 5eab21e1b1..7265f742e9 100644
> --- a/xen/xsm/xsm_core.c
> +++ b/xen/xsm/xsm_core.c
> @@ -68,17 +76,10 @@ static int __init parse_xsm_param(const char *s)
> }
> custom_param("xsm", parse_xsm_param);
>
> -static inline int verify(struct xsm_operations *ops)
> -{
> - /* verify the security_operations structure exists */
> - if ( !ops )
> - return -EINVAL;
> - xsm_fixup_ops(ops);
> - return 0;
> -}
> -
> static int __init xsm_core_init(const void *policy_buffer, size_t
> policy_size)
> {
> + struct xsm_operations *mod_ops;
> +
Hard tabs, and later in this function. Also, how about just 'ops' for
the local variable name?
> @@ -113,6 +124,17 @@ 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
Stray double space.
~Andrew
