[v2,3/3] xsm: properly handle error from XSM init

Commit Message

Daniel P. Smith May 31, 2022, 2:39 a.m. UTC

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 <[e-mail]>
---
 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(-)

Comments

Bertrand Marquis May 31, 2022, 9:10 a.m. UTC | #1

Hi Daniel,

> On 31 May 2022, at 03:39, Daniel P. Smith <[e-mail]> wrote:
> 
> 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 <[e-mail]>

For the arm part:
Reviewed-by: Bertrand Marquis <[e-mail]>

Cheers
Bertrand

Patch

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 8f6c3de8a6..6377895e1e 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"
 
@@ -190,7 +184,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;
 }
 
 /**