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, <[email protected]>
> * George Coker, <[email protected]>
> - *
> + *
> * 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
