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

[PATCH v3 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 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    | 56 +++++++++++++++++++++----------------------
 xen/xsm/xsm_policy.c  | 31 ++++++++++++++++++++----
 5 files changed, 68 insertions(+), 40 deletions(-)

-- 
2.20.1

[PATCH v3 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>
---
 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..4a29ee9558 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 && XSM_MAGIC )
     {
         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 && XSM_MAGIC )
     {
         ret = xsm_dt_policy_init(&policy_buffer, &policy_size);
         if ( ret )
-- 
2.20.1

[PATCH v3 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 if an
error occurs attempting to populate the policy buffer.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
 xen/include/xsm/xsm.h |  2 +-
 xen/xsm/xsm_core.c    | 18 +++---------------
 xen/xsm/xsm_policy.c  | 31 +++++++++++++++++++++++++++----
 3 files changed, 31 insertions(+), 20 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 4a29ee9558..8f6c3de8a6 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
@@ -155,7 +147,7 @@ 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");
@@ -167,8 +159,7 @@ int __init xsm_multiboot_init(
         if ( ret )
         {
             bootstrap_map(NULL);
-            printk(XENLOG_ERR "Error %d initializing XSM policy\n", ret);
-            return -EINVAL;
+            panic(XENLOG_ERR "Error %d initializing XSM policy\n", ret);
         }
     }
 
@@ -192,10 +183,7 @@ int __init xsm_dt_init(void)
     {
         ret = xsm_dt_policy_init(&policy_buffer, &policy_size);
         if ( ret )
-        {
-            printk(XENLOG_ERR "Error %d initializing XSM policy\n", ret);
-            return -EINVAL;
-        }
+            panic(XENLOG_ERR "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..6a4f769aec 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,6 +76,9 @@ 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
@@ -79,7 +90,16 @@ int __init xsm_dt_policy_init(void **policy_buffer, size_t *policy_size)
     paddr_t paddr, len;
 
     if ( !mod || !mod->size )
+    {
+#ifdef CONFIG_XSM_FLASK_POLICY
+        *policy_buffer = (void *)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 +115,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 v3 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 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;
 }
 
 /**
-- 
2.20.1

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

[Jan Beulich]

On 31.05.2022 17:08, 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>

Looks technically okay, so
Reviewed-by: Jan Beulich <jbeulich@suse.com>
but couldn't you ...

> @@ -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 && XSM_MAGIC )
>      {
>          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 && XSM_MAGIC )
>      {
>          ret = xsm_dt_policy_init(&policy_buffer, &policy_size);
>          if ( ret )

... drop the two "&& XSM_MAGIC" here at this time? Afaict policy_file_required
cannot be true when XSM_MAGIC is zero.

Jan

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

[Jan Beulich]

On 31.05.2022 17:08, 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 if an
> error occurs attempting to populate the policy buffer.

"flask=late" is also a mode where, afaict, no policy is required. I can't,
however, see how you're taking care of that (but maybe I'm overlooking
something); inspecting flask_bootparam in generic XSM code would actually
be a layering violation.

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

I don't think we're dealing with an array here, so const unsigned char **
would seem the more correct representation to me.

Also - what about the DT counterpart function?

Jan

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

[Daniel P. Smith]

On 5/31/22 11:51, Jan Beulich wrote:
> On 31.05.2022 17:08, 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>
> 
> Looks technically okay, so
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> but couldn't you ...
> 
>> @@ -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 && XSM_MAGIC )
>>      {
>>          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 && XSM_MAGIC )
>>      {
>>          ret = xsm_dt_policy_init(&policy_buffer, &policy_size);
>>          if ( ret )
> 
> ... drop the two "&& XSM_MAGIC" here at this time? Afaict policy_file_required
> cannot be true when XSM_MAGIC is zero.

I was on the fence about this, as it should be rendered as redundant as
you point out. I am good with dropping on next spin.

v/r,
dps

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

[Daniel P. Smith]

On 5/31/22 12:05, Jan Beulich wrote:
> On 31.05.2022 17:08, 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 if an
>> error occurs attempting to populate the policy buffer.
> 
> "flask=late" is also a mode where, afaict, no policy is required. I can't,
> however, see how you're taking care of that (but maybe I'm overlooking
> something); inspecting flask_bootparam in generic XSM code would actually
> be a layering violation.

Good point, flask=late is meant to be enforcing with a late loading of a
policy file. I will address it.

>> --- 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);
> 
> I don't think we're dealing with an array here, so const unsigned char **
> would seem the more correct representation to me.
> 
> Also - what about the DT counterpart function?

Ack.

v/r
dps

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

[Jan Beulich]

On 31.05.2022 18:15, Daniel P. Smith wrote:
> 
> On 5/31/22 11:51, Jan Beulich wrote:
>> On 31.05.2022 17:08, 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>
>>
>> Looks technically okay, so
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> but couldn't you ...
>>
>>> @@ -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 && XSM_MAGIC )
>>>      {
>>>          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 && XSM_MAGIC )
>>>      {
>>>          ret = xsm_dt_policy_init(&policy_buffer, &policy_size);
>>>          if ( ret )
>>
>> ... drop the two "&& XSM_MAGIC" here at this time? Afaict policy_file_required
>> cannot be true when XSM_MAGIC is zero.
> 
> I was on the fence about this, as it should be rendered as redundant as
> you point out. I am good with dropping on next spin.

I'd also be okay dropping this while committing, unless a v4 appears
first ...

Jan

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

[Jan Beulich]

On 31.05.2022 17:08, 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.

In light of the "flask=late" aspect of patch 2, I'd like to suggest to
slightly alter wording here: "... requires looking for a policy file."

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

The variable may then also want renaming, to e.g. "find_policy_file".

Jan

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

[Jan Beulich]

On 31.05.2022 17:08, Daniel P. Smith wrote:
> @@ -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();

Please omit formatting changes to entirely unrelated pieces of code.

> @@ -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");

Uncontrolled communication isn't the only thing that could occur, aiui.
So at the very least "e.g." or some such would want adding imo.

Now that return values are checked, I think that in addition to what
you already do the two function declarations may want decorating with
__must_check.

Jan

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

[Daniel P. Smith]

On 6/1/22 02:04, Jan Beulich wrote:
> On 31.05.2022 18:15, Daniel P. Smith wrote:
>>
>> On 5/31/22 11:51, Jan Beulich wrote:
>>> On 31.05.2022 17:08, 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>
>>>
>>> Looks technically okay, so
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>> but couldn't you ...
>>>
>>>> @@ -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 && XSM_MAGIC )
>>>>      {
>>>>          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 && XSM_MAGIC )
>>>>      {
>>>>          ret = xsm_dt_policy_init(&policy_buffer, &policy_size);
>>>>          if ( ret )
>>>
>>> ... drop the two "&& XSM_MAGIC" here at this time? Afaict policy_file_required
>>> cannot be true when XSM_MAGIC is zero.
>>
>> I was on the fence about this, as it should be rendered as redundant as
>> you point out. I am good with dropping on next spin.
> 
> I'd also be okay dropping this while committing, unless a v4 appears
> first ...

ack

v/r,
dps

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

[Daniel P. Smith]

On 6/1/22 02:08, Jan Beulich wrote:
> On 31.05.2022 17:08, 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.
> 
> In light of the "flask=late" aspect of patch 2, I'd like to suggest to
> slightly alter wording here: "... requires looking for a policy file."

ack

>> --- 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);
> 
> The variable may then also want renaming, to e.g. "find_policy_file".

ack

v/r,
dps

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

[Daniel P. Smith]

On 6/1/22 02:14, Jan Beulich wrote:
> On 31.05.2022 17:08, Daniel P. Smith wrote:
>> @@ -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();
> 
> Please omit formatting changes to entirely unrelated pieces of code.

Ack. this was in simliar vein of the other patches where I cleaned
nearby trailing space.

>> @@ -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");
> 
> Uncontrolled communication isn't the only thing that could occur, aiui.
> So at the very least "e.g." or some such would want adding imo.

Agreed, this was a reuse of the existing message and honestly I would
like to believe this was the original author's attempt at brevity versus
writing a detailed message of every implication to the security of the
system.

> Now that return values are checked, I think that in addition to what
> you already do the two function declarations may want decorating with
> __must_check.

Understood but likely not necessary based on Andy's review suggestion to
move these functions to void.

v/r,
dps

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

[Jan Beulich]

On 07.06.2022 14:14, Daniel P. Smith wrote:
> On 6/1/22 02:14, Jan Beulich wrote:
>> Now that return values are checked, I think that in addition to what
>> you already do the two function declarations may want decorating with
>> __must_check.
> 
> Understood but likely not necessary based on Andy's review suggestion to
> move these functions to void.

Of course - once they return void, they cannot be __must_check.

Jan