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, <mdle...@epoch.ncsc.mil> > * George Coker, <gsco...@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