[PATCH] xsm: refactor and optimize policy loading

[PATCH] xsm: refactor and optimize policy loading

[Daniel P. Smith]

It is possible to select a few different build configurations that results in
the unnecessary walking of the boot module list looking for a policy module.
This specifically occurs when the flask policy is enabled but either the dummy
or the SILO policy is selected as the enforcing policy. This is not ideal for
configurations like hyperlaunch and dom0less when there could be a number of
modules to be walked or unnecessary device tree lookups

This patch does two things, it moves all policy initialization logic under the
xsm_XXXX_policy_init() functions and introduces the init_policy flag.  The
init_policy flag will be set based on which enforcing policy is selected and
gates whether the boot modules should be checked for a policy file.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
 xen/xsm/xsm_core.c   | 30 +++++++++++++++++++-----------
 xen/xsm/xsm_policy.c | 21 +++++++++++++++++++--
 2 files changed, 38 insertions(+), 13 deletions(-)

diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c
index 2286a502e3..0dfc970283 100644
--- 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
+
 static int __init cf_check parse_xsm_param(const char *s)
 {
     int rc = 0;
 
     if ( !strcmp(s, "dummy") )
+    {
         xsm_bootparam = XSM_BOOTPARAM_DUMMY;
+        init_policy = false;
+    }
 #ifdef CONFIG_XSM_FLASK
     else if ( !strcmp(s, "flask") )
+    {
         xsm_bootparam = XSM_BOOTPARAM_FLASK;
+        init_policy = true;
+    }
 #endif
 #ifdef CONFIG_XSM_SILO
     else if ( !strcmp(s, "silo") )
+    {
         xsm_bootparam = XSM_BOOTPARAM_SILO;
+        init_policy = false;
+    }
 #endif
     else
         rc = -EINVAL;
@@ -80,14 +96,6 @@ static int __init xsm_core_init(const void *policy_buffer, size_t policy_size)
 {
     const struct xsm_ops *ops = NULL;
 
-#ifdef CONFIG_XSM_FLASK_POLICY
-    if ( policy_size == 0 )
-    {
-        policy_buffer = xsm_flask_init_policy;
-        policy_size = xsm_flask_init_policy_size;
-    }
-#endif
-
     if ( xsm_ops_registered != XSM_OPS_UNREGISTERED )
     {
         printk(XENLOG_ERR
@@ -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 )
         {
             bootstrap_map(NULL);
             printk(XENLOG_ERR "Error %d initializing XSM policy\n", ret);
@@ -176,7 +184,7 @@ int __init xsm_dt_init(void)
 
     printk("XSM Framework v" XSM_FRAMEWORK_VERSION " initialized\n");
 
-    if ( XSM_MAGIC )
+    if ( init_policy && XSM_MAGIC )
     {
         ret = xsm_dt_policy_init(&policy_buffer, &policy_size);
         if ( ret )
diff --git a/xen/xsm/xsm_policy.c b/xen/xsm/xsm_policy.c
index 8dafbc9381..0e32418999 100644
--- a/xen/xsm/xsm_policy.c
+++ b/xen/xsm/xsm_policy.c
@@ -8,7 +8,7 @@
  *  Contributors:
  *  Michael LeMay, <mdlemay@epoch.ncsc.mil>
  *  George Coker, <gscoker@alpha.ncsc.mil>
- *  
+ *
  *  This program is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License version 2,
  *  as published by the Free Software Foundation.
@@ -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;
     u32 *_policy_start;
     unsigned long _policy_len;
 
+#ifdef CONFIG_XSM_FLASK_POLICY
+    /* Initially set to builtin policy, overriden if boot module is found. */
+    *policy_buffer = (void *)xsm_flask_init_policy;
+    *policy_size = xsm_flask_init_policy_size;
+    rc = 0;
+#endif
+
     /*
      * Try all modules and see whichever could be the binary policy.
      * Adjust module_map for the module that is the binary policy.
@@ -61,6 +68,7 @@ int __init xsm_multiboot_policy_init(
                    _policy_len,_policy_start);
 
             __clear_bit(i, module_map);
+            rc = 0;
             break;
 
         }
@@ -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;
+        *policy_size = xsm_flask_init_policy_size;
         return 0;
+    }
+#else
+        /* No policy was loaded */
+        return -ENOENT;
+#endif
 
     paddr = mod->start;
     len = mod->size;
-- 
2.20.1

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

[Jan Beulich]

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?

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

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

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

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

Jan

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

[Daniel P. Smith]

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

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

[Jan Beulich]

On 24.05.2022 19:42, Daniel P. Smith wrote:
> On 5/24/22 11:50, Jan Beulich wrote:
>> On 23.05.2022 17:40, Daniel P. Smith wrote:
>>> @@ -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'm afraid this doesn't clarify anything for me wrt the addition of
-ENOENT. There's no case of returning -ENOENT right now afaics (and
there's no case of you dealing with the -ENOENT anywhere in your
changes, so there would look to be an overall change in behavior as
viewed from the outside, i.e. the callers of xsm_{multiboot,dt}_init()).
If that's wrong for some reason (and yes, it looks at least questionable
on the surface), that's what wants spelling out imo. A question then
might be whether that's not a separate change, potentially even a fix
which may want to be considered for backport. Of course otoh the sole
caller of xsm_multiboot_init() ignores the return value altogether,
and the sole caller of xsm_dt_init() only cares for the specific value
of 1. This in turn looks at least questionable to me.

Jan

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

[Daniel P. Smith]

On 5/25/22 02:37, Jan Beulich wrote:
> On 24.05.2022 19:42, Daniel P. Smith wrote:
>> On 5/24/22 11:50, Jan Beulich wrote:
>>> On 23.05.2022 17:40, Daniel P. Smith wrote:
>>>> @@ -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'm afraid this doesn't clarify anything for me wrt the addition of
> -ENOENT. There's no case of returning -ENOENT right now afaics (and
> there's no case of you dealing with the -ENOENT anywhere in your
> changes, so there would look to be an overall change in behavior as
> viewed from the outside, i.e. the callers of xsm_{multiboot,dt}_init()).
> If that's wrong for some reason (and yes, it looks at least questionable
> on the surface), that's what wants spelling out imo. A question then
> might be whether that's not a separate change, potentially even a fix
> which may want to be considered for backport. Of course otoh the sole
> caller of xsm_multiboot_init() ignores the return value altogether,
> and the sole caller of xsm_dt_init() only cares for the specific value
> of 1. This in turn looks at least questionable to me.

Okay, let me change the view a bit.

The existing behavior is for xsm_{multiboot,dt}_init() to return 0 if
they did not locate a policy file for initializing the policy buffer. It
did so because it expected later that xsm_core_init() would just set it
to the built-in policy. BUT, it also served the purpose of succeeding if
it were called when either the dummy or SILO, neither of which needs the
policy buffer initialized, is the enforcing policy.

This change starts with moving the lines that set the policy buffer to
the built-in policy into xsm_{multiboot,dt}_init() respectively and only
return success if it was able to populate the policy buffer. In other
words, if there is not a built-in policy and a policy file was not able
to be loaded, it returns -NOENT to represent it was not able to find a
policy file. This change makes these functions do as their name
suggests, to initialize the policy buffer either from MB or DT with a
fallback to the built-in policy otherwise fail.

Upon making the function behave correctly, it exposes a valid set of
conditions that under the current behavior succeeds but will fail under
the correct behavior for xsm_{multiboot,dt}_init(). With flask enabled
in the build but not the built-in policy and either dummy or SILO is the
enforcing policy, then xsm_{multiboot,dt}_init() will be called and
fail. This is where the second half of the change comes into effect,
which is to introduce a gating that will only attempt to initialize the
policy buffer if the enforcing XSM policy requires a policy file. With
this gating in place, under the above set of conditions
xsm_{multiboot,dt}_init() will not be called and XSM initialization will
succeed as it should.

Now to your question of whether these changes could be split and given a
focused explanation in their respective commits. Yes, I can split it
into two separate commits. While the gating of the call to
xsm_{multiboot,dt}_init() is an independent change, the change to
xsm_{multiboot,dt}_init() itself is not and must proceed after the
gating change. This means it is possible to backport the first commit,
gating, independently. If the desire is to backport the second commit,
xsm_{multiboot,dt}_init() behavior, then it would require both commits
to be backported.

I hope this helps better clarify my reasoning and if you would like to
see the changes split how I highlighted, just let me know.

v/r,
dps

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

[Jan Beulich]

On 25.05.2022 16:11, Daniel P. Smith wrote:
> On 5/25/22 02:37, Jan Beulich wrote:
>> On 24.05.2022 19:42, Daniel P. Smith wrote:
>>> On 5/24/22 11:50, Jan Beulich wrote:
>>>> On 23.05.2022 17:40, Daniel P. Smith wrote:
>>>>> @@ -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'm afraid this doesn't clarify anything for me wrt the addition of
>> -ENOENT. There's no case of returning -ENOENT right now afaics (and
>> there's no case of you dealing with the -ENOENT anywhere in your
>> changes, so there would look to be an overall change in behavior as
>> viewed from the outside, i.e. the callers of xsm_{multiboot,dt}_init()).
>> If that's wrong for some reason (and yes, it looks at least questionable
>> on the surface), that's what wants spelling out imo. A question then
>> might be whether that's not a separate change, potentially even a fix
>> which may want to be considered for backport. Of course otoh the sole
>> caller of xsm_multiboot_init() ignores the return value altogether,
>> and the sole caller of xsm_dt_init() only cares for the specific value
>> of 1. This in turn looks at least questionable to me.
> 
> Okay, let me change the view a bit.
> 
> The existing behavior is for xsm_{multiboot,dt}_init() to return 0 if
> they did not locate a policy file for initializing the policy buffer. It
> did so because it expected later that xsm_core_init() would just set it
> to the built-in policy. BUT, it also served the purpose of succeeding if
> it were called when either the dummy or SILO, neither of which needs the
> policy buffer initialized, is the enforcing policy.
> 
> This change starts with moving the lines that set the policy buffer to
> the built-in policy into xsm_{multiboot,dt}_init() respectively and only
> return success if it was able to populate the policy buffer. In other
> words, if there is not a built-in policy and a policy file was not able
> to be loaded, it returns -NOENT to represent it was not able to find a
> policy file. This change makes these functions do as their name
> suggests, to initialize the policy buffer either from MB or DT with a
> fallback to the built-in policy otherwise fail.
> 
> Upon making the function behave correctly, it exposes a valid set of
> conditions that under the current behavior succeeds but will fail under
> the correct behavior for xsm_{multiboot,dt}_init(). With flask enabled
> in the build but not the built-in policy and either dummy or SILO is the
> enforcing policy, then xsm_{multiboot,dt}_init() will be called and
> fail. This is where the second half of the change comes into effect,
> which is to introduce a gating that will only attempt to initialize the
> policy buffer if the enforcing XSM policy requires a policy file. With
> this gating in place, under the above set of conditions
> xsm_{multiboot,dt}_init() will not be called and XSM initialization will
> succeed as it should.
> 
> Now to your question of whether these changes could be split and given a
> focused explanation in their respective commits. Yes, I can split it
> into two separate commits. While the gating of the call to
> xsm_{multiboot,dt}_init() is an independent change, the change to
> xsm_{multiboot,dt}_init() itself is not and must proceed after the
> gating change. This means it is possible to backport the first commit,
> gating, independently. If the desire is to backport the second commit,
> xsm_{multiboot,dt}_init() behavior, then it would require both commits
> to be backported.
> 
> I hope this helps better clarify my reasoning and if you would like to
> see the changes split how I highlighted, just let me know.

I'd like to leave to you whether to split. All I'm after is that from
the description it becomes clear what the (intended) effect of the
added -ENOENT errors is, which didn't exist before. Now that we're
about to start adopting some Misra-C rules, this may even need to
extend to cover the case of so far missing error checks potentially
being added up the call tree.

Jan

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

[Jason Andryuk]

On Mon, May 23, 2022 at 11:40 AM Daniel P. Smith
<dpsmith@apertussolutions.com> wrote:
>
> It is possible to select a few different build configurations that results in
> the unnecessary walking of the boot module list looking for a policy module.
> This specifically occurs when the flask policy is enabled but either the dummy
> or the SILO policy is selected as the enforcing policy. This is not ideal for
> configurations like hyperlaunch and dom0less when there could be a number of
> modules to be walked or unnecessary device tree lookups
>
> This patch does two things, it moves all policy initialization logic under the
> xsm_XXXX_policy_init() functions and introduces the init_policy flag.  The
> init_policy flag will be set based on which enforcing policy is selected and
> gates whether the boot modules should be checked for a policy file.

I can see the use of init_policy to skip the search.  (I'm not the
biggest fan of the name, need_policy/uses_policy/has_policy?, but
that's not a big deal).  That part seems fine.

I don't care for the movement of `policy_buffer =
xsm_flask_init_policy;` since it replaces the single location with two
locations.  I prefer leaving the built-in policy fallback in
xsm_core_init since it is multiboot/devicetree agnostic.  i.e. the
boot-method specific code passes a policy if it finds one, and
xsm_core_init can fallback to the built-in policy if none is supplied.
Since a built-in policy is flask specific, it could potentially be
pushed down in flask_init.

Is there a need for the xsm_flask_init_policy movement I am missing?

Regards,
Jason

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

[Daniel P. Smith]

On 5/24/22 12:17, Jason Andryuk wrote:
> On Mon, May 23, 2022 at 11:40 AM Daniel P. Smith
> <dpsmith@apertussolutions.com> wrote:
>>
>> It is possible to select a few different build configurations that results in
>> the unnecessary walking of the boot module list looking for a policy module.
>> This specifically occurs when the flask policy is enabled but either the dummy
>> or the SILO policy is selected as the enforcing policy. This is not ideal for
>> configurations like hyperlaunch and dom0less when there could be a number of
>> modules to be walked or unnecessary device tree lookups
>>
>> This patch does two things, it moves all policy initialization logic under the
>> xsm_XXXX_policy_init() functions and introduces the init_policy flag.  The
>> init_policy flag will be set based on which enforcing policy is selected and
>> gates whether the boot modules should be checked for a policy file.
> 
> I can see the use of init_policy to skip the search.  (I'm not the
> biggest fan of the name, need_policy/uses_policy/has_policy?, but
> that's not a big deal).  That part seems fine.

Yep, I went through that and several other variations trying to find the
one that would "read" best at the places it was set and checked. If
anyone has a strong stance for a preferred naming, I have no objections.

> I don't care for the movement of `policy_buffer =
> xsm_flask_init_policy;` since it replaces the single location with two
> locations.  I prefer leaving the built-in policy fallback in
> xsm_core_init since it is multiboot/devicetree agnostic.  i.e. the
> boot-method specific code passes a policy if it finds one, and
> xsm_core_init can fallback to the built-in policy if none is supplied.
> Since a built-in policy is flask specific, it could potentially be
> pushed down in flask_init.
> 
> Is there a need for the xsm_flask_init_policy movement I am missing?

I would be willing to bet that the de-duplication is the reason it was
initially done this way. But as I explained in the response to Jan,
doing so means that xsm_XXXX_policy_init() will have to return success
even if it did not successfully initialize the policy buffer. I
considered making a static inline function to point the policy buffer at
the built-in policy, but quickly walked back from it. The reason being
is that it is not any real logic duplications, just two lines of
variable setting. IMHO a single repeat of setting a pair of variables is
better than the bifurcating of the policy buffer initialization.

v/r,
dps