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

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

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@xxxxxxxxxxxxxxxxxxxx>
> ---
>  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@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
> +
>      /*
>       * 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;