On 13.07.2021 01:39, 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.
+1
>> --- 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.
Not really, it's a structure copy now. What I'm concerned about is
the size of that on-stack variable and its lack of an initializer,
undermining the many set_to_dummy_if_null() that xsm_fixup_ops()
uses. Can't xsm_fixup_ops() act on xsm_ops directly, perhaps after
memset()-ing it here first (if nothing else then just to be on the
safe side)?
>> 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.
Also, even if {flask,silo}_ops couldn't be const for some reason,
these init functions now want to return a pointer-to-const, as
the caller isn't supposed to modify the struct-s any further.
Jan
