Re: [PATCH v4 3/3] xsm: properly handle error from XSM init
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 3/3] xsm: properly handle error from XSM init
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
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 3/3] xsm: properly handle error from XSM init
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
[PATCH v4 3/3] xsm: properly handle error from XSM init
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
Reviewed-by: Bertrand Marquis # 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
#include
#include
+#include
#include
#include
#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
#include
+#include
+#include
#include
#include
-
-#include
+#include
#include
-#ifdef CONFIG_XSM
-
-#ifdef CONFIG_MULTIBOOT
#include
-#endif
-#ifdef CONFIG_HAS_DEVICE_TREE
-#include
-#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
