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

Commit Message

Daniel P. Smith May 31, 2022, 6:20 p.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]>
Reviewed-by: Bertrand Marquis <[e-mail]> # 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(-)

Comments

Andrew Cooper May 31, 2022, 7:18 p.m. UTC | #1

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

Jan Beulich June 1, 2022, 6:49 a.m. UTC | #2

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

Daniel P. Smith June 7, 2022, 1:55 p.m. UTC | #3

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

Daniel P. Smith June 7, 2022, 1:56 p.m. UTC | #4

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.

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 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;
 }
 
 /**