Re: [PATCH] xsm: refactor and optimize policy loading

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

On 5/24/22 11:50, Jan Beulich wrote:
> On 23.05.2022 17:40, Daniel P. Smith wrote:
>> --- a/xen/xsm/xsm_core.c
>> +++ b/xen/xsm/xsm_core.c
>> @@ -55,19 +55,35 @@ static enum xsm_bootparam __initdata xsm_bootparam =
>>      XSM_BOOTPARAM_DUMMY;
>>  #endif
>>  
>> +static bool __initdata init_policy =
>> +#ifdef CONFIG_XSM_FLASK_DEFAULT
>> +    true;
>> +#else
>> +    false;
>> +#endif
> 
> Simply IS_ENABLED(CONFIG_XSM_FLASK_DEFAULT) without any #ifdef-ary?

Ack, didn't even think of that. Thanks!

>> @@ -148,11 +156,11 @@ int __init xsm_multiboot_init(
>>  
>>      printk("XSM Framework v" XSM_FRAMEWORK_VERSION " initialized\n");
>>  
>> -    if ( XSM_MAGIC )
>> +    if ( init_policy && XSM_MAGIC )
>>      {
>>          ret = xsm_multiboot_policy_init(module_map, mbi, &policy_buffer,
>>                                          &policy_size);
>> -        if ( ret )
>> +        if ( ret != 0 )
> 
> Nit: Stray change?

Yep, I twiddled with the if statement while testing/debugging all the
permutations of enabled policies, default policy, built-in policy, and
cmdline selected policy. Will clean up.

>> @@ -36,10 +36,17 @@ int __init xsm_multiboot_policy_init(
>>  {
>>      int i;
>>      module_t *mod = (module_t *)__va(mbi->mods_addr);
>> -    int rc = 0;
>> +    int rc = -ENOENT;
> 
> I'm afraid I can't easily convince myself that this and the other
> -ENOENT is really relevant to this change and/or not breaking
> anything which currently does work (i.e. not entirely benign).
> Please can you extend the description accordingly or split off
> this adjustment?

Let me expand the commit explanation, and you can let me know how much
of this detail you would like to see in the commit message.

When enabling CONFIG_XSM_FLASK today, the XSM_MAGIC variable becomes
defined as a non-zero value. This results in xsm_XXXX_policy_init() to
be called regardless of the XSM policy selection, either through the
respective CONFIG_XSM_XXXXX_DEFAULT flags or through the cmdline
parameter. Additionally, the concept of policy initialization is split
between xsm_XXXX_policy_init() and xsm_core_init(), with the latter
being where the built-in policy would be selected. This forces
xsm_XXXX_policy_init() to have to return successful for configurations
where no policy loading was necessary. It also means that the situation
where selecting XSM flask, with no built-in policy, and failure to
provide a policy via MB/DT relies on the security server to fault when
it is unable to load a policy.

What this commit does is moves all policy buffer initialization to
xsm_XXXX_policy_init(). As the init function, it should only return
success (0) if it was able to initialize the policy buffer with either
the built-in policy or a policy loaded from MB/DT. The second half of
this commit corrects the logical flow so that the policy buffer
initialization only occurs for XSM policy modules that consume a policy
buffer, e.g. flask.

I would note the opening of the commit message notes dom0less and
hyperlaunch because I came across this while finalizing the first
hyperlaunch patch series focused on fully integrating hyperlaunch's
bootmodule structure, which will also will enable commonality with
dom0less' bootmodule structure, into x86 MB1/MB2/EFI startup paths. I
figured this change should just go alone and not with hyperlaunch
bootmodule work.

>> @@ -79,7 +87,16 @@ int __init xsm_dt_policy_init(void **policy_buffer, 
>> size_t *policy_size)
>>      paddr_t paddr, len;
>>  
>>      if ( !mod || !mod->size )
>> +#ifdef CONFIG_XSM_FLASK_POLICY
>> +    {
>> +        *policy_buffer = (void *)xsm_flask_init_policy;
> 
> I don't think we want a cast here, especially not when it discards
> "const". Instead the local variables' types want adjusting in
> xsm_{multiboot,dt}_init() as well as the types of the respective
> parameters of xsm_{multiboot,dt}_policy_init().

True, will fix.

>> +        *policy_size = xsm_flask_init_policy_size;
>>          return 0;
>> +    }
>> +#else
>> +        /* No policy was loaded */
>> +        return -ENOENT;
>> +#endif
> 
> I think this is easier to read if you put the braces there
> unconditionally and have the #if / #else inside. And if you wanted
> to I think you could get away without any #else then.

Ack.


v/r,
dps