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

[Jan Beulich <jbeulich@xxxxxxxx>]

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