| Message ID | 20220531182041.10640-3-dpsmith@apertussolutions.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xsm: refactor and optimize policy loading | expand |
On 5/31/22 14: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. > > Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> > --- > xen/include/xsm/xsm.h | 2 +- > xen/xsm/xsm_core.c | 51 ++++++++++++++++++++----------------------- > xen/xsm/xsm_policy.c | 34 ++++++++++++++++++++++++----- > 3 files changed, 54 insertions(+), 33 deletions(-) > > diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h > index 3e2b7fe3db..1676c261c9 100644 > --- 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); > #endif > > #ifdef CONFIG_HAS_DEVICE_TREE > diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c > index 675e4f552c..a3715fa239 100644 > --- a/xen/xsm/xsm_core.c > +++ b/xen/xsm/xsm_core.c > @@ -92,14 +92,6 @@ static int __init xsm_core_init(const void *policy_buffer, size_t policy_size) > { > const struct xsm_ops *ops = NULL; > > -#ifdef CONFIG_XSM_FLASK_POLICY > - if ( policy_size == 0 ) > - { > - policy_buffer = xsm_flask_init_policy; > - policy_size = xsm_flask_init_policy_size; > - } > -#endif > - > if ( xsm_ops_registered != XSM_OPS_UNREGISTERED ) > { > printk(XENLOG_ERR > @@ -154,28 +146,29 @@ static int __init xsm_core_init(const void *policy_buffer, size_t policy_size) > int __init xsm_multiboot_init( > unsigned long *module_map, const multiboot_info_t *mbi) > { > - int ret = 0; > - void *policy_buffer = NULL; > + const unsigned char *policy_buffer; > size_t policy_size = 0; > > printk("XSM Framework v" XSM_FRAMEWORK_VERSION " initialized\n"); > > if ( policy_file_required ) > { > - ret = xsm_multiboot_policy_init(module_map, mbi, &policy_buffer, > + int ret = xsm_multiboot_policy_init(module_map, mbi, &policy_buffer, > &policy_size); > - if ( ret ) > - { > - bootstrap_map(NULL); > - printk(XENLOG_ERR "Error %d initializing XSM policy\n", ret); > - return -EINVAL; > - } > + bootstrap_map(NULL); > + > + if ( ret == -ENOENT ) > + /* > + * The XSM module needs a policy file but one was not located. > + * Report as a warning and continue as the XSM module may late > + * load a policy file. > + */ > + printk(XENLOG_WARNING "xsm: starting without a policy loaded!\n"); > + else > + panic("Error %d initializing XSM policy\n", ret); > } > > - ret = xsm_core_init(policy_buffer, policy_size); > - bootstrap_map(NULL); > - > - return 0; > + return xsm_core_init(policy_buffer, policy_size); > } > #endif > > @@ -183,7 +176,7 @@ int __init xsm_multiboot_init( > int __init xsm_dt_init(void) > { > int ret = 0; > - void *policy_buffer = NULL; > + const unsigned char *policy_buffer; > size_t policy_size = 0; > > printk("XSM Framework v" XSM_FRAMEWORK_VERSION " initialized\n"); > @@ -191,11 +184,15 @@ int __init xsm_dt_init(void) > if ( policy_file_required ) > { > ret = xsm_dt_policy_init(&policy_buffer, &policy_size); > - if ( ret ) > - { > - printk(XENLOG_ERR "Error %d initializing XSM policy\n", ret); > - return -EINVAL; > - } > + if ( ret == -ENOENT ) > + /* > + * The XSM module needs a policy file but one was not located. > + * Report as a warning and continue as the XSM module may late > + * load a policy file. > + */ > + printk(XENLOG_WARNING "xsm: starting without a policy loaded!\n"); > + else > + panic("Error %d initializing XSM policy\n", ret); > } > > ret = xsm_core_init(policy_buffer, policy_size); > diff --git a/xen/xsm/xsm_policy.c b/xen/xsm/xsm_policy.c > index 8dafbc9381..690fd23e9f 100644 > --- a/xen/xsm/xsm_policy.c > +++ b/xen/xsm/xsm_policy.c > @@ -8,7 +8,7 @@ > * Contributors: > * Michael LeMay, <mdlemay@epoch.ncsc.mil> > * George Coker, <gscoker@alpha.ncsc.mil> > - * > + * > * This program is free software; you can redistribute it and/or modify > * it under the terms of the GNU General Public License version 2, > * as published by the Free Software Foundation. > @@ -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. */ > + *policy_buffer = xsm_flask_init_policy; > + *policy_size = xsm_flask_init_policy_size; > + rc = 0; > +#endif > + > /* > * Try all modules and see whichever could be the binary policy. > * Adjust module_map for the module that is the binary policy. > @@ -54,13 +61,14 @@ int __init xsm_multiboot_policy_init( > > if ( (xsm_magic_t)(*_policy_start) == XSM_MAGIC ) > { > - *policy_buffer = _policy_start; > + *policy_buffer = (unsigned char *)_policy_start; > *policy_size = _policy_len; > > printk("Policy len %#lx, start at %p.\n", > _policy_len,_policy_start); > > __clear_bit(i, module_map); > + rc = 0; > break; > > } > @@ -68,18 +76,31 @@ int __init xsm_multiboot_policy_init( > bootstrap_map(NULL); > } > > + if ( rc == -ENOENT ) > + printk(XENLOG_ERR "xsm: Unable to locate policy file\n"); > + > return rc; > } > #endif > > #ifdef CONFIG_HAS_DEVICE_TREE > -int __init xsm_dt_policy_init(void **policy_buffer, size_t *policy_size) > +int __init xsm_dt_policy_init( > + const unsigned char **policy_buffer, size_t *policy_size) I just caught that I missed the matching header declaration. ( ._.) I noticed there is a one-liner at the end of INSTALL for doing cross-compile. I will see if I can get that to work to incorporate in my build/test system. > { > struct bootmodule *mod = boot_module_find_by_kind(BOOTMOD_XSM); > paddr_t paddr, len; > > if ( !mod || !mod->size ) > + { > +#ifdef CONFIG_XSM_FLASK_POLICY > + *policy_buffer = xsm_flask_init_policy; > + *policy_size = xsm_flask_init_policy_size; > return 0; > +#else > + printk(XENLOG_ERR "xsm: Unable to locate policy file\n"); > + return -ENOENT; > +#endif > + } > > paddr = mod->start; > len = mod->size; > @@ -95,7 +116,10 @@ int __init xsm_dt_policy_init(void **policy_buffer, size_t *policy_size) > > *policy_buffer = xmalloc_bytes(len); > if ( !*policy_buffer ) > + { > + printk(XENLOG_ERR "xsm: Unable to allocate memory for XSM policy\n"); > return -ENOMEM; > + } > > copy_from_paddr(*policy_buffer, paddr, len); > *policy_size = len;
On 31/05/2022 19:20, Daniel P. Smith wrote: > diff --git a/xen/xsm/xsm_policy.c b/xen/xsm/xsm_policy.c > index 8dafbc9381..690fd23e9f 100644 > --- a/xen/xsm/xsm_policy.c > +++ b/xen/xsm/xsm_policy.c > @@ -8,7 +8,7 @@ > * Contributors: > * Michael LeMay, <mdlemay@epoch.ncsc.mil> > * George Coker, <gscoker@alpha.ncsc.mil> > - * > + * > * This program is free software; you can redistribute it and/or modify > * it under the terms of the GNU General Public License version 2, > * as published by the Free Software Foundation. > @@ -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. */ > + *policy_buffer = xsm_flask_init_policy; > + *policy_size = xsm_flask_init_policy_size; > + rc = 0; > +#endif Does if ( IS_ENABLED(CONFIG_XSM_FLASK_POLICY) ) { ... } compile properly? You'll need to drop the ifdefary in xsm.h, but this would be a better approach (more compiler coverage in normal builds). Same for the related hunk on the DT side. > + > /* > * Try all modules and see whichever could be the binary policy. > * Adjust module_map for the module that is the binary policy. > @@ -54,13 +61,14 @@ int __init xsm_multiboot_policy_init( > > if ( (xsm_magic_t)(*_policy_start) == XSM_MAGIC ) > { > - *policy_buffer = _policy_start; > + *policy_buffer = (unsigned char *)_policy_start; The existing logic is horrible. To start with, there's a buffer overrun for a module of fewer than 4 bytes. (Same on the DT side.) It would be slightly less horrible as for ( ... ) { void *ptr; if ( !test_bit(i, module_map) || mod[i].mod_end < sizeof(xsm_header) ) continue; ptr = bootstrap_map(...); if ( memcmp(ptr, XSM_MAGIC, sizeof(XSM_MAGIC)) == 0 ) { *policy_buffer = ptr; *policy_len = mod[i].mod_end; ... break; } bootstrap_map(NULL); } because at least this gets rid of the intermediate variables, the buffer overrun, and the awkward casting to find XSM_MAGIC. That said, it's still unclear what's going on, because has_xsm_magic() says the header is 16 bytes long, rather than 4 (assuming that it means uint32_t. policydb_read() uses uint32_t). Also, policydb_read() uses le32_to_cpu() so the multiboot/dt checks are wrong on big-endian systems. Also also, policydb_read() really doesn't need to make a temporary memory allocation to check a fixed string of fixed length. This is horrible. I suspect we're getting into "in a subsequent patch" territory here. ~Andrew
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). > --- 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. > @@ -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" Jan
On 5/31/22 15:07, Andrew Cooper wrote: > On 31/05/2022 19:20, Daniel P. Smith wrote: >> diff --git a/xen/xsm/xsm_policy.c b/xen/xsm/xsm_policy.c >> index 8dafbc9381..690fd23e9f 100644 >> --- a/xen/xsm/xsm_policy.c >> +++ b/xen/xsm/xsm_policy.c >> @@ -8,7 +8,7 @@ >> * Contributors: >> * Michael LeMay, <mdlemay@epoch.ncsc.mil> >> * George Coker, <gscoker@alpha.ncsc.mil> >> - * >> + * >> * This program is free software; you can redistribute it and/or modify >> * it under the terms of the GNU General Public License version 2, >> * as published by the Free Software Foundation. >> @@ -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. */ >> + *policy_buffer = xsm_flask_init_policy; >> + *policy_size = xsm_flask_init_policy_size; >> + rc = 0; >> +#endif > > Does > > if ( IS_ENABLED(CONFIG_XSM_FLASK_POLICY) ) > { > ... > } > > compile properly? You'll need to drop the ifdefary in xsm.h, but this > would be a better approach (more compiler coverage in normal builds). > > Same for the related hunk on the DT side. Yes, I know this pattern is used elsewhere, so it should work here fine. >> + >> /* >> * Try all modules and see whichever could be the binary policy. >> * Adjust module_map for the module that is the binary policy. >> @@ -54,13 +61,14 @@ int __init xsm_multiboot_policy_init( >> >> if ( (xsm_magic_t)(*_policy_start) == XSM_MAGIC ) >> { >> - *policy_buffer = _policy_start; >> + *policy_buffer = (unsigned char *)_policy_start; > > The existing logic is horrible. To start with, there's a buffer overrun > for a module of fewer than 4 bytes. (Same on the DT side.) > > It would be slightly less horrible as > > for ( ... ) > { > void *ptr; > > if ( !test_bit(i, module_map) || mod[i].mod_end < sizeof(xsm_header) ) > continue; > > ptr = bootstrap_map(...); > > if ( memcmp(ptr, XSM_MAGIC, sizeof(XSM_MAGIC)) == 0 ) > { > *policy_buffer = ptr; > *policy_len = mod[i].mod_end; > > ... > break; > } > > bootstrap_map(NULL); > } > > because at least this gets rid of the intermediate variables, the buffer > overrun, and the awkward casting to find XSM_MAGIC. Since you were kind enough to take the time to write out the fix, I can incorporate. > That said, it's still unclear what's going on, because has_xsm_magic() > says the header is 16 bytes long, rather than 4 (assuming that it means > uint32_t. policydb_read() uses uint32_t). > > Also, policydb_read() uses le32_to_cpu() so the multiboot/dt checks are > wrong on big-endian systems. > > Also also, policydb_read() really doesn't need to make a temporary > memory allocation to check a fixed string of fixed length. This is > horrible. > > I suspect we're getting into "in a subsequent patch" territory here. Unfortunately the scope of what this series started out to solve, not to walk all the boot modules when no policy file is needed, and what the reviewers have been requesting be addressed is continually diverging. Granted, with the technical debt currently present in XSM, I understand why this is occurring. Unfortunately, subsequent patch here means, going on to my list of things I would like to fix/work on in XSM. v/r, dps
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
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). Jan
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. v/r, dps
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
On Tue, Jun 7, 2022 at 9:16 AM Daniel P. Smith <dpsmith@apertussolutions.com> wrote: > Unfortunately the scope of what this series started out to solve, not to > walk all the boot modules when no policy file is needed, and what the > reviewers have been requesting be addressed is continually diverging. You only need patch 1/3 to skip the walk, AFAICT. So maybe you should just submit that independently. Regards, Jason
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h index 3e2b7fe3db..1676c261c9 100644 --- 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); #endif #ifdef CONFIG_HAS_DEVICE_TREE diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c index 675e4f552c..a3715fa239 100644 --- a/xen/xsm/xsm_core.c +++ b/xen/xsm/xsm_core.c @@ -92,14 +92,6 @@ static int __init xsm_core_init(const void *policy_buffer, size_t policy_size) { const struct xsm_ops *ops = NULL; -#ifdef CONFIG_XSM_FLASK_POLICY - if ( policy_size == 0 ) - { - policy_buffer = xsm_flask_init_policy; - policy_size = xsm_flask_init_policy_size; - } -#endif - if ( xsm_ops_registered != XSM_OPS_UNREGISTERED ) { printk(XENLOG_ERR @@ -154,28 +146,29 @@ static int __init xsm_core_init(const void *policy_buffer, size_t policy_size) int __init xsm_multiboot_init( unsigned long *module_map, const multiboot_info_t *mbi) { - int ret = 0; - void *policy_buffer = NULL; + const unsigned char *policy_buffer; size_t policy_size = 0; printk("XSM Framework v" XSM_FRAMEWORK_VERSION " initialized\n"); if ( policy_file_required ) { - ret = xsm_multiboot_policy_init(module_map, mbi, &policy_buffer, + int ret = xsm_multiboot_policy_init(module_map, mbi, &policy_buffer, &policy_size); - if ( ret ) - { - bootstrap_map(NULL); - printk(XENLOG_ERR "Error %d initializing XSM policy\n", ret); - return -EINVAL; - } + bootstrap_map(NULL); + + if ( ret == -ENOENT ) + /* + * The XSM module needs a policy file but one was not located. + * Report as a warning and continue as the XSM module may late + * load a policy file. + */ + printk(XENLOG_WARNING "xsm: starting without a policy loaded!\n"); + else + panic("Error %d initializing XSM policy\n", ret); } - ret = xsm_core_init(policy_buffer, policy_size); - bootstrap_map(NULL); - - return 0; + return xsm_core_init(policy_buffer, policy_size); } #endif @@ -183,7 +176,7 @@ int __init xsm_multiboot_init( int __init xsm_dt_init(void) { int ret = 0; - void *policy_buffer = NULL; + const unsigned char *policy_buffer; size_t policy_size = 0; printk("XSM Framework v" XSM_FRAMEWORK_VERSION " initialized\n"); @@ -191,11 +184,15 @@ int __init xsm_dt_init(void) if ( policy_file_required ) { ret = xsm_dt_policy_init(&policy_buffer, &policy_size); - if ( ret ) - { - printk(XENLOG_ERR "Error %d initializing XSM policy\n", ret); - return -EINVAL; - } + if ( ret == -ENOENT ) + /* + * The XSM module needs a policy file but one was not located. + * Report as a warning and continue as the XSM module may late + * load a policy file. + */ + printk(XENLOG_WARNING "xsm: starting without a policy loaded!\n"); + else + panic("Error %d initializing XSM policy\n", ret); } ret = xsm_core_init(policy_buffer, policy_size); diff --git a/xen/xsm/xsm_policy.c b/xen/xsm/xsm_policy.c index 8dafbc9381..690fd23e9f 100644 --- a/xen/xsm/xsm_policy.c +++ b/xen/xsm/xsm_policy.c @@ -8,7 +8,7 @@ * Contributors: * Michael LeMay, <mdlemay@epoch.ncsc.mil> * George Coker, <gscoker@alpha.ncsc.mil> - * + * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2, * as published by the Free Software Foundation. @@ -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. */ + *policy_buffer = xsm_flask_init_policy; + *policy_size = xsm_flask_init_policy_size; + rc = 0; +#endif + /* * Try all modules and see whichever could be the binary policy. * Adjust module_map for the module that is the binary policy. @@ -54,13 +61,14 @@ int __init xsm_multiboot_policy_init( if ( (xsm_magic_t)(*_policy_start) == XSM_MAGIC ) { - *policy_buffer = _policy_start; + *policy_buffer = (unsigned char *)_policy_start; *policy_size = _policy_len; printk("Policy len %#lx, start at %p.\n", _policy_len,_policy_start); __clear_bit(i, module_map); + rc = 0; break; } @@ -68,18 +76,31 @@ int __init xsm_multiboot_policy_init( bootstrap_map(NULL); } + if ( rc == -ENOENT ) + printk(XENLOG_ERR "xsm: Unable to locate policy file\n"); + return rc; } #endif #ifdef CONFIG_HAS_DEVICE_TREE -int __init xsm_dt_policy_init(void **policy_buffer, size_t *policy_size) +int __init xsm_dt_policy_init( + const unsigned char **policy_buffer, size_t *policy_size) { struct bootmodule *mod = boot_module_find_by_kind(BOOTMOD_XSM); paddr_t paddr, len; if ( !mod || !mod->size ) + { +#ifdef CONFIG_XSM_FLASK_POLICY + *policy_buffer = xsm_flask_init_policy; + *policy_size = xsm_flask_init_policy_size; return 0; +#else + printk(XENLOG_ERR "xsm: Unable to locate policy file\n"); + return -ENOENT; +#endif + } paddr = mod->start; len = mod->size; @@ -95,7 +116,10 @@ int __init xsm_dt_policy_init(void **policy_buffer, size_t *policy_size) *policy_buffer = xmalloc_bytes(len); if ( !*policy_buffer ) + { + printk(XENLOG_ERR "xsm: Unable to allocate memory for XSM policy\n"); return -ENOMEM; + } copy_from_paddr(*policy_buffer, paddr, len); *policy_size = len;
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. Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> --- xen/include/xsm/xsm.h | 2 +- xen/xsm/xsm_core.c | 51 ++++++++++++++++++++----------------------- xen/xsm/xsm_policy.c | 34 ++++++++++++++++++++++++----- 3 files changed, 54 insertions(+), 33 deletions(-)