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

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

On 7/12/21 7:39 PM, Andrew Cooper wrote:
> 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.

Looks like Jan also agreed, so I will add this to the mix.

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

As Jan has pointed out it is not a ref issue, but it was very bad of me
to leave the stack var uninitialized. Regardless as you pointed out,
this will be irrelevant with moving patch 3 ahead of this.

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

After this and seeing Jan's comments, I am going to walk through this
again and see if there is more adjustments/cleanups I can do/

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

Ack

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

Ack

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

Ack

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

Ack

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

Ack

> ~Andrew
> 

v/r,
dps