Re: [PATCH 1/6] xsm: refactor xsm_ops handling

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 18/06/2021 00:39, Daniel P. Smith wrote:
> The assignment and setup of xsm_ops structure was refactored to make it a
> one-time assignment. The calling of the xsm_ops were refactored to use the
> alternate_call framework to reduce the need for retpolines.
>
> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>

I think the commit message needs a little more explanation for anyone
doing code archaeology.

AFAICT, the current infrastructure has some (incomplete?) support for
Flask to unload itself as the security engine, which doesn't sounds like
a clever thing in general.

What we do here is make a semantic change to say that the security
engine (Dummy, Flask or SILO) gets chosen once during boot, and is
immutable thereafter.  This is better from a security standpoint (no
accidentally unloading Flask at runtime), and allows for the use of the
alternative_vcall() infrastructure to drop all the function pointer calls.

Does that about sum things up?

> diff --git a/xen/xsm/flask/flask_op.c b/xen/xsm/flask/flask_op.c
> index 01e52138a1..df9fcc1d6d 100644
> --- a/xen/xsm/flask/flask_op.c
> +++ b/xen/xsm/flask/flask_op.c
> @@ -225,26 +225,7 @@ static int flask_security_sid(struct 
> xen_flask_sid_context *arg)
>  
>  static int flask_disable(void)
>  {
> -    static int flask_disabled = 0;
> -
> -    if ( ss_initialized )
> -    {
> -        /* Not permitted after initial policy load. */
> -        return -EINVAL;
> -    }
> -
> -    if ( flask_disabled )
> -    {
> -        /* Only do this once. */
> -        return -EINVAL;
> -    }
> -
> -    printk("Flask:  Disabled at runtime.\n");
> -
> -    flask_disabled = 1;
> -
> -    /* Reset xsm_ops to the original module. */
> -    xsm_ops = &dummy_xsm_ops;
> +    printk("Flask:  Disabling is not supported.\n");

Judging by this, should this patch be split up more?

I think you want to remove FLASK_DISABLE (and this function too - just
return -EOPNOTSUPP in the parent) as a separate explained change (as it
is a logical change in how Flask works).

The second patch wants to be the rest, which changes the indirection of
xsm_ops and converts to vcall().  This is a fairly mechanical change
without semantic changes.

I'm unsure if you want a 3rd patch in the middle, separating the
xsm_core_init() juggling, with the "switch to using vcall()".  It might
be a good idea for more easily demonstrating the changes, but I'll leave
it to your judgement.

> diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c
> index 5eab21e1b1..acc1af7166 100644
> --- a/xen/xsm/xsm_core.c
> +++ b/xen/xsm/xsm_core.c
>  static int __init xsm_core_init(const void *policy_buffer, size_t 
> policy_size)
>  {
>  #ifdef CONFIG_XSM_FLASK_POLICY
> @@ -87,17 +86,22 @@ static int __init xsm_core_init(const void 
> *policy_buffer, size_t policy_size)
>      }
>  #endif
>  
> -    if ( verify(&dummy_xsm_ops) )
> +    if ( xsm_ops_registered != XSM_OPS_UNREGISTERED )
>      {
> -        printk(XENLOG_ERR "Could not verify dummy_xsm_ops structure\n");
> +        printk(XENLOG_ERR
> +            "Could not init XSM, xsm_ops register already attempted\n");

Indentation.

>          return -EIO;
>      }
>  
> -    xsm_ops = &dummy_xsm_ops;
> +    /* install the dummy ops as default to ensure ops
> +     * are defined if requested policy fails init
> +     */
> +    xsm_fixup_ops(&xsm_ops);

/* Comment style. */

or

/*
 * Multi-
 * line comment style.
 */

>      switch ( xsm_bootparam )
>      {
>      case XSM_BOOTPARAM_DUMMY:
> +        xsm_ops_registered = XSM_OPS_REGISTERED;
>          break;
>  
>      case XSM_BOOTPARAM_FLASK:
> @@ -113,6 +117,9 @@ static int __init xsm_core_init(const void 
> *policy_buffer, size_t policy_size)
>          break;
>      }
>  
> +    if ( xsm_ops_registered != XSM_OPS_REGISTERED )
> +        xsm_ops_registered = XSM_OPS_REG_FAILED;
> +
>      return 0;
>  }
>  
> @@ -197,16 +204,21 @@ bool __init has_xsm_magic(paddr_t start)
>  
>  int __init register_xsm(struct xsm_operations *ops)
>  {
> -    if ( verify(ops) )
> +    if ( xsm_ops_registered != XSM_OPS_UNREGISTERED )
> +        return -EAGAIN;

I know you moved this around the function, but it really isn't -EAGAIN
material any more.  It's "too late - nope".

-EEXIST is probably best for "I'm never going to tolerate another call".

> +
> +    if ( !ops )
>      {
> -        printk(XENLOG_ERR "Could not verify xsm_operations structure\n");
> +        xsm_ops_registered = XSM_OPS_REG_FAILED;
> +        printk(XENLOG_ERR "Invalid xsm_operations structure registered\n");
>          return -EINVAL;

Honestly, I'd be half tempted to declare register_xsm() with
__nonnull(0) and let the compiler reject any attempt to pass a NULL ops
pointer.

Both callers pass a pointer to a static singleton objects.

>      }
>  
> -    if ( xsm_ops != &dummy_xsm_ops )
> -        return -EAGAIN;
> +    /* use dummy ops for any empty ops */
> +    xsm_fixup_ops(ops);

Isn't this redundant with the call in xsm_core_init(), seeing as
register_xsm() must be nested within the switch statement?

> -    xsm_ops = ops;
> +    xsm_ops = *ops;
> +    xsm_ops_registered = XSM_OPS_REGISTERED;
>  
>      return 0;
>  }

Having got to the end, the xsm_core_init() vs register_xsm() dynamic is
quite weird.

I think it would result in clearer code to have init_{flask,silo}()
return pointers to their struct xsm_operations *, and let
xsm_core_init() do the copy in to xsm_ops.  This reduces the scope of
xsm_ops_state to this function only, and gets rid of at least one
runtime panic() call which is dead code.

If you were to go with this approach, you'd definitely want to split
into the 3-patch approach.

~Andrew