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

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

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.

>> --- a/xen/include/xsm/xsm.h
>> +++ b/xen/include/xsm/xsm.h
>> @@ -775,7 +775,7 @@ int xsm_multiboot_init(
>>      unsigned long *module_map, const multiboot_info_t *mbi);
>>  int xsm_multiboot_policy_init(
>>      unsigned long *module_map, const multiboot_info_t *mbi,
>> -    void **policy_buffer, size_t *policy_size);
>> +    const unsigned char *policy_buffer[], size_t *policy_size);
> 
> See my v3 comment on the use of [] here. And that comment was actually
> made before you sent v4 (unlike the later comment on patch 1). Oh,
> actually you did change this in the function definition, just not here.

ack

>> @@ -32,14 +32,21 @@
>>  #ifdef CONFIG_MULTIBOOT
>>  int __init xsm_multiboot_policy_init(
>>      unsigned long *module_map, const multiboot_info_t *mbi,
>> -    void **policy_buffer, size_t *policy_size)
>> +    const unsigned char **policy_buffer, size_t *policy_size)
>>  {
>>      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. 
>> */
> 
> Nit: "overridden"

ack

v/r,
dps