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

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 6/18/21 7:34 AM, Andrew Cooper wrote:
> 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?

ack

>> 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.
ack

>>          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.
>  */

ack

>>      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".

Honestly I didn't think EAGAIN was correct in the first place, so will
gladly change it.

>> +
>> +    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?

I don't believe so, the one in xsm_core_init is setting the
default/fallback of xsm_ops var to dummy_* before attempting to register
a policy module's ops. Here in register_xsm we are taking a new instance
of a struct xsm_ops and ensuring every handler has a defined entry
before overwriting the xsm_ops variable with passed in xsm module's ops.
Now with that said, I do like your suggest down at the end.

>> -    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.

I agree full.

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

v2 will have this broken out

v/r,
dps