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

[Jan Beulich <jbeulich@xxxxxxxx>]

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