Re: [PATCH v2 02/10] xsm: refactor xsm_ops handling

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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