[PATCH v4 0/3] xsm: refactor and optimize policy loading

[PATCH v4 0/3] xsm: refactor and optimize policy loading

[Daniel P. Smith]

This series was born out of some cleanup work done while crafting the
hyperlaunch boot modules patch series. The primary purpose of this series was
to stop walking all boot modules when it was not necessary, but the scope
creeped during review.

Changes in v4:
* rewroked xsm_{multiboot,dt}_init() to support flask=late
* applied the correct types to the policy_buffer for DT
* change type from array to pointer on xsm_multiboot_policy_init()

Changes in v3:
* added arm Rb for patch #3
* mainly a resend due to fubar on sending v2

Changes in v2:
* changed init_policy to policy_file_required
* split the patch into a series
* corrected casting of policy buffer
* use IS_ENABLED() instead of #ifdef sequence
* moved #ifdef inside of braces for xsm_dt_policy_init()
* addressed lack of error handling of xsm{mb,dt}_init()

Daniel P. Smith (3):
  xsm: only search for a policy file when needed
  xsm: consolidate loading the policy buffer
  xsm: properly handle error from XSM init

 xen/arch/arm/setup.c  | 10 ++---
 xen/arch/x86/setup.c  |  9 ++++-
 xen/include/xsm/xsm.h |  2 +-
 xen/xsm/xsm_core.c    | 89 ++++++++++++++++++++++++-------------------
 xen/xsm/xsm_policy.c  | 34 ++++++++++++++---
 5 files changed, 91 insertions(+), 53 deletions(-)

-- 
2.20.1

[PATCH v4 1/3] xsm: only search for a policy file when needed

[Daniel P. Smith]

It is possible to select a few different build configurations that results in
the unnecessary walking of the boot module list looking for a policy module.
This specifically occurs when the flask policy is enabled but either the dummy
or the SILO policy is selected as the enforcing policy. This is not ideal for
configurations like hyperlaunch and dom0less when there could be a number of
modules to be walked or doing an unnecessary device tree lookup.

This patch introduces the policy_file_required flag for tracking when an XSM
policy module requires a policy file. Only when the policy_file_required flag
is set to true, will XSM search the boot modules for a policy file.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
 xen/xsm/xsm_core.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c
index 2286a502e3..675e4f552c 100644
--- a/xen/xsm/xsm_core.c
+++ b/xen/xsm/xsm_core.c
@@ -55,19 +55,31 @@ static enum xsm_bootparam __initdata xsm_bootparam =
     XSM_BOOTPARAM_DUMMY;
 #endif
 
+static bool __initdata policy_file_required =
+    IS_ENABLED(CONFIG_XSM_FLASK_DEFAULT);
+
 static int __init cf_check parse_xsm_param(const char *s)
 {
     int rc = 0;
 
     if ( !strcmp(s, "dummy") )
+    {
         xsm_bootparam = XSM_BOOTPARAM_DUMMY;
+        policy_file_required = false;
+    }
 #ifdef CONFIG_XSM_FLASK
     else if ( !strcmp(s, "flask") )
+    {
         xsm_bootparam = XSM_BOOTPARAM_FLASK;
+        policy_file_required = true;
+    }
 #endif
 #ifdef CONFIG_XSM_SILO
     else if ( !strcmp(s, "silo") )
+    {
         xsm_bootparam = XSM_BOOTPARAM_SILO;
+        policy_file_required = false;
+    }
 #endif
     else
         rc = -EINVAL;
@@ -148,7 +160,7 @@ int __init xsm_multiboot_init(
 
     printk("XSM Framework v" XSM_FRAMEWORK_VERSION " initialized\n");
 
-    if ( XSM_MAGIC )
+    if ( policy_file_required )
     {
         ret = xsm_multiboot_policy_init(module_map, mbi, &policy_buffer,
                                         &policy_size);
@@ -176,7 +188,7 @@ int __init xsm_dt_init(void)
 
     printk("XSM Framework v" XSM_FRAMEWORK_VERSION " initialized\n");
 
-    if ( XSM_MAGIC )
+    if ( policy_file_required )
     {
         ret = xsm_dt_policy_init(&policy_buffer, &policy_size);
         if ( ret )
-- 
2.20.1

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

[Daniel P. Smith]

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)
 {
     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;
-- 
2.20.1

[PATCH v4 3/3] xsm: properly handle error from XSM init

[Daniel P. Smith]

This commit is to move towards providing a uniform interface across
architectures to initialize the XSM framework. Specifically, it provides a
common handling of initialization failure by providing the printing of a
warning message.

For Arm, xsm_dt_init() was tailored to have an Arm specific expansion of the
return values. This expansion added a value to reflect whether the security
supported XSM policy module was the enforcing policy module. This was then used
to determine if a warning message would be printed. Despite this expansion,
like x86, Arm does not address any XSM initialization errors that may have
occurred.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> # arm
---
 xen/arch/arm/setup.c | 10 +++++-----
 xen/arch/x86/setup.c |  9 +++++++--
 xen/xsm/xsm_core.c   | 22 +++++++++++-----------
 3 files changed, 23 insertions(+), 18 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index ea1f5ee3d3..6bf71e1064 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -967,11 +967,11 @@ void __init start_xen(unsigned long boot_phys_offset,
 
     tasklet_subsys_init();
 
-    if ( xsm_dt_init() != 1 )
-        warning_add("WARNING: SILO mode is not enabled.\n"
-                    "It has implications on the security of the system,\n"
-                    "unless the communications have been forbidden between\n"
-                    "untrusted domains.\n");
+    if ( xsm_dt_init() )
+        warning_add("WARNING: XSM failed to initialize.\n"
+                    "This has implications on the security of the system,\n"
+                    "as uncontrolled communications between trusted and\n"
+                    "untrusted domains may occur.\n");
 
     init_maintenance_interrupt();
     init_timer_interrupt();
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 53a73010e0..ed67b50c9d 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -24,6 +24,7 @@
 #include <xen/pfn.h>
 #include <xen/nodemask.h>
 #include <xen/virtual_region.h>
+#include <xen/warning.h>
 #include <xen/watchdog.h>
 #include <public/version.h>
 #ifdef CONFIG_COMPAT
@@ -1690,7 +1691,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 
     open_softirq(NEW_TLBFLUSH_CLOCK_PERIOD_SOFTIRQ, new_tlbflush_clock_period);
 
-    if ( opt_watchdog ) 
+    if ( opt_watchdog )
         nmi_watchdog = NMI_LOCAL_APIC;
 
     find_smp_config();
@@ -1700,7 +1701,11 @@ void __init noreturn __start_xen(unsigned long mbi_p)
     mmio_ro_ranges = rangeset_new(NULL, "r/o mmio ranges",
                                   RANGESETF_prettyprint_hex);
 
-    xsm_multiboot_init(module_map, mbi);
+    if ( xsm_multiboot_init(module_map, mbi) )
+        warning_add("WARNING: XSM failed to initialize.\n"
+                    "This has implications on the security of the system,\n"
+                    "as uncontrolled communications between trusted and\n"
+                    "untrusted domains may occur.\n");
 
     /*
      * IOMMU-related ACPI table parsing may require some of the system domains
diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c
index a3715fa239..fa17401a5f 100644
--- a/xen/xsm/xsm_core.c
+++ b/xen/xsm/xsm_core.c
@@ -10,23 +10,17 @@
  *  as published by the Free Software Foundation.
  */
 
-#include <xen/init.h>
 #include <xen/errno.h>
+#include <xen/hypercall.h>
+#include <xen/init.h>
 #include <xen/lib.h>
 #include <xen/param.h>
-
-#include <xen/hypercall.h>
+#include <xen/warning.h>
 #include <xsm/xsm.h>
 
-#ifdef CONFIG_XSM
-
-#ifdef CONFIG_MULTIBOOT
 #include <asm/setup.h>
-#endif
 
-#ifdef CONFIG_HAS_DEVICE_TREE
-#include <asm/setup.h>
-#endif
+#ifdef CONFIG_XSM
 
 #define XSM_FRAMEWORK_VERSION    "1.0.1"
 
@@ -199,7 +193,13 @@ int __init xsm_dt_init(void)
 
     xfree(policy_buffer);
 
-    return ret ?: (xsm_bootparam == XSM_BOOTPARAM_SILO);
+    if ( xsm_bootparam != XSM_BOOTPARAM_SILO )
+        warning_add("WARNING: SILO mode is not enabled.\n"
+                    "It has implications on the security of the system,\n"
+                    "unless the communications have been forbidden between\n"
+                    "untrusted domains.\n");
+
+    return ret;
 }
 
 /**
-- 
2.20.1

Re: [PATCH v4 1/3] xsm: only search for a policy file when needed

[Jason Andryuk]

On Tue, May 31, 2022 at 2:22 PM Daniel P. Smith
<dpsmith@apertussolutions.com> wrote:
>
> It is possible to select a few different build configurations that results in
> the unnecessary walking of the boot module list looking for a policy module.
> This specifically occurs when the flask policy is enabled but either the dummy
> or the SILO policy is selected as the enforcing policy. This is not ideal for
> configurations like hyperlaunch and dom0less when there could be a number of
> modules to be walked or doing an unnecessary device tree lookup.
>
> This patch introduces the policy_file_required flag for tracking when an XSM
> policy module requires a policy file. Only when the policy_file_required flag
> is set to true, will XSM search the boot modules for a policy file.
>
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Jason Andryuk <jandryuk@gmail.com>

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

[Daniel P. Smith]

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;

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

[Andrew Cooper]

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

Re: [PATCH v4 3/3] xsm: properly handle error from XSM init

[Andrew Cooper]

On 31/05/2022 19:20, Daniel P. Smith wrote:
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 53a73010e0..ed67b50c9d 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1700,7 +1701,11 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>      mmio_ro_ranges = rangeset_new(NULL, "r/o mmio ranges",
>                                    RANGESETF_prettyprint_hex);
>  
> -    xsm_multiboot_init(module_map, mbi);
> +    if ( xsm_multiboot_init(module_map, mbi) )
> +        warning_add("WARNING: XSM failed to initialize.\n"
> +                    "This has implications on the security of the system,\n"
> +                    "as uncontrolled communications between trusted and\n"
> +                    "untrusted domains may occur.\n");

The problem with this approach is that it forces each architecture to
opencode the failure string, in a function which is very busy with other
things too.

Couldn't xsm_{multiboot,dt}_init() be void, and the warning_add() move
into them, like the SLIO warning for ARM already?

That would simplify both ARM and x86's __start_xen(), and be an
improvement for the RISC-V series just posted to xen-devel...

~Andrew

Re: [PATCH v4 3/3] xsm: properly handle error from XSM init

[Jan Beulich]

On 31.05.2022 21:18, Andrew Cooper wrote:
> On 31/05/2022 19:20, Daniel P. Smith wrote:
>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index 53a73010e0..ed67b50c9d 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -1700,7 +1701,11 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>      mmio_ro_ranges = rangeset_new(NULL, "r/o mmio ranges",
>>                                    RANGESETF_prettyprint_hex);
>>  
>> -    xsm_multiboot_init(module_map, mbi);
>> +    if ( xsm_multiboot_init(module_map, mbi) )
>> +        warning_add("WARNING: XSM failed to initialize.\n"
>> +                    "This has implications on the security of the system,\n"
>> +                    "as uncontrolled communications between trusted and\n"
>> +                    "untrusted domains may occur.\n");
> 
> The problem with this approach is that it forces each architecture to
> opencode the failure string, in a function which is very busy with other
> things too.
> 
> Couldn't xsm_{multiboot,dt}_init() be void, and the warning_add() move
> into them, like the SLIO warning for ARM already?

I, too, was considering to suggest this (but then didn't on v3). Furthermore
the warning_add() could then be wrapped in a trivial helper function to be
used by both MB and DT.

Jan

Re: [PATCH v4 1/3] xsm: only search for a policy file when needed

[Jan Beulich]

On 31.05.2022 20:20, Daniel P. Smith wrote:
> It is possible to select a few different build configurations that results in
> the unnecessary walking of the boot module list looking for a policy module.
> This specifically occurs when the flask policy is enabled but either the dummy
> or the SILO policy is selected as the enforcing policy. This is not ideal for
> configurations like hyperlaunch and dom0less when there could be a number of
> modules to be walked or doing an unnecessary device tree lookup.
> 
> This patch introduces the policy_file_required flag for tracking when an XSM
> policy module requires a policy file. Only when the policy_file_required flag
> is set to true, will XSM search the boot modules for a policy file.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> ---
>  xen/xsm/xsm_core.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)

FTAOD my (later) v3 comments still apply; v4 simply appeared too quickly.

Jan

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

[Jan Beulich]

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

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

[Daniel P. Smith]

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

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

[Daniel P. Smith]

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

Re: [PATCH v4 3/3] xsm: properly handle error from XSM init

[Daniel P. Smith]

On 5/31/22 15:18, Andrew Cooper wrote:
> On 31/05/2022 19:20, Daniel P. Smith wrote:
>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index 53a73010e0..ed67b50c9d 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -1700,7 +1701,11 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>      mmio_ro_ranges = rangeset_new(NULL, "r/o mmio ranges",
>>                                    RANGESETF_prettyprint_hex);
>>  
>> -    xsm_multiboot_init(module_map, mbi);
>> +    if ( xsm_multiboot_init(module_map, mbi) )
>> +        warning_add("WARNING: XSM failed to initialize.\n"
>> +                    "This has implications on the security of the system,\n"
>> +                    "as uncontrolled communications between trusted and\n"
>> +                    "untrusted domains may occur.\n");
> 
> The problem with this approach is that it forces each architecture to
> opencode the failure string, in a function which is very busy with other
> things too.
> 
> Couldn't xsm_{multiboot,dt}_init() be void, and the warning_add() move
> into them, like the SLIO warning for ARM already?
> 
> That would simplify both ARM and x86's __start_xen(), and be an
> improvement for the RISC-V series just posted to xen-devel...

I was trying to address the MISRA review comment by handling the
unhandled return while trying to provide a uniform implementation across
arch. Moving the xsm_{multiboot,dt}_init() to void will address both and
as you point out make it simpler overall.

v/r,
dps

Re: [PATCH v4 3/3] xsm: properly handle error from XSM init

[Daniel P. Smith]

On 6/1/22 02:49, Jan Beulich wrote:
> On 31.05.2022 21:18, Andrew Cooper wrote:
>> On 31/05/2022 19:20, Daniel P. Smith wrote:
>>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>>> index 53a73010e0..ed67b50c9d 100644
>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>> @@ -1700,7 +1701,11 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>>      mmio_ro_ranges = rangeset_new(NULL, "r/o mmio ranges",
>>>                                    RANGESETF_prettyprint_hex);
>>>  
>>> -    xsm_multiboot_init(module_map, mbi);
>>> +    if ( xsm_multiboot_init(module_map, mbi) )
>>> +        warning_add("WARNING: XSM failed to initialize.\n"
>>> +                    "This has implications on the security of the system,\n"
>>> +                    "as uncontrolled communications between trusted and\n"
>>> +                    "untrusted domains may occur.\n");
>>
>> The problem with this approach is that it forces each architecture to
>> opencode the failure string, in a function which is very busy with other
>> things too.
>>
>> Couldn't xsm_{multiboot,dt}_init() be void, and the warning_add() move
>> into them, like the SLIO warning for ARM already?
> 
> I, too, was considering to suggest this (but then didn't on v3). Furthermore
> the warning_add() could then be wrapped in a trivial helper function to be
> used by both MB and DT.

Re: helper function, ack.

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

[Jan Beulich]

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

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

[Daniel P. Smith]

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

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

[Jan Beulich]

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

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

[Jason Andryuk]

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