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