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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

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@xxxxxxxxxxxxxx>
>   *  George Coker, <gscoker@xxxxxxxxxxxxxx>
> - *  
> + *
>   *  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