Re: [PATCH v4 2/3] xsm: consolidate loading the policy buffer

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.06.2022 16:10, Daniel P. Smith wrote:
> On 6/7/22 09:58, Jan Beulich wrote:
>> On 07.06.2022 15:47, Daniel P. Smith wrote:
>>>
>>> On 6/2/22 05:47, Jan Beulich wrote:
>>>> On 31.05.2022 20:20, Daniel P. Smith wrote:
>>>>> Previously, initializing the policy buffer was split between two 
>>>>> functions,
>>>>> xsm_{multiboot,dt}_policy_init() and xsm_core_init(). The latter for 
>>>>> loading
>>>>> the policy from boot modules and the former for falling back to built-in
>>>>> policy.
>>>>>
>>>>> This patch moves all policy buffer initialization logic under the
>>>>> xsm_{multiboot,dt}_policy_init() functions. It then ensures that an error
>>>>> message is printed for every error condition that may occur in the 
>>>>> functions.
>>>>> With all policy buffer init contained and only called when the policy 
>>>>> buffer
>>>>> must be populated, the respective xsm_{mb,dt}_init() functions will panic 
>>>>> for
>>>>> all errors except ENOENT. An ENOENT signifies that a policy file could 
>>>>> not be
>>>>> located. Since it is not possible to know if late loading of the policy 
>>>>> file is
>>>>> intended, a warning is reported and XSM initialization is continued.
>>>>
>>>> Is it really not possible to know? flask_init() panics in the one case
>>>> where a policy is strictly required. And I'm not convinced it is
>>>> appropriate to issue both an error and a warning in most (all?) of the
>>>> other cases (and it should be at most one of the two anyway imo).
>>>
>>> With how XSM currently works, I do not see how without creating a
>>> layering violation, as you had mentioned before. It is possible for
>>> flask_init() to know because the flask= parameter belongs to the flask
>>> policy module and can be directly checked.
>>>
>>> I think we view this differently. A call to
>>> xsm_{multiboot,dt}_policy_init() is asking for a policy file to be
>>> loaded. If it is not able to do so is an error. This error is reported
>>> back to xsm_{multiboot,dt}_init() which is responsible for initializing
>>> the XSM framework. If it encounters an error for which inhibits it from
>>> initializing XSM would be an error whereas an error it encounters that
>>> does not block initialization should warn the user as such. While it is
>>> true that the only error for the xsm_multiboot_policy_init() currently
>>> is a failure to locate a policy file, ENOENT, I don't see how that
>>> changes the understanding.
>>
>> Well, I think that to avoid the layering violation the decision whether
>> an error is severe enough to warrant a warning (or is even fatal) needs
>> to be left to the specific model (i.e. Flask in this case).
> 
> Except that it is not the policy module that loads the policy, where the
> error could occur. As you pointed out for MISRA compliance, you cannot
> have unhandled errors. So either, the errors must be ignored where they
> occur and wait for a larger, non-specific panic, or a nesting of
> handling/reporting the errors needs to be provided for a user to see in
> the log as to why they ended up at the panic.

Right - I was thinking that the error code could be propagated to Flask
instead of (or, less desirable, along with) the NULL pointer indicating
the absence of a policy module. That still would satisfy the "error
indications need to be checked for" MISRA requirement.

Jan