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

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

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