[PATCH v2 3/3] Disallow most command-line options when lockdown mode is enabled

[PATCH v2 3/3] Disallow most command-line options when lockdown mode is enabled

[Kevin Lampis]

A subset of command-line parameters that are specifically safe to use when
lockdown mode is enabled are annotated as such.

These are commonly used parameters which have been audited to ensure they
cannot be used to undermine the integrity of the system when booted in
Secure Boot mode.

Signed-off-by: Kevin Lampis <kevin.lampis@cloud.com>
Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
Changes in v2:
- Add more information about the safe parameters
- Add lockdown section to the command line doc
---
 docs/misc/xen-command-line.pandoc     | 16 +++++++++
 xen/arch/arm/domain_build.c           |  4 +--
 xen/arch/x86/acpi/cpu_idle.c          |  2 +-
 xen/arch/x86/cpu/amd.c                |  2 +-
 xen/arch/x86/cpu/mcheck/mce.c         |  2 +-
 xen/arch/x86/cpu/microcode/core.c     |  2 +-
 xen/arch/x86/dom0_build.c             |  4 +--
 xen/arch/x86/hvm/hvm.c                |  2 +-
 xen/arch/x86/irq.c                    |  2 +-
 xen/arch/x86/nmi.c                    |  2 +-
 xen/arch/x86/setup.c                  |  2 +-
 xen/arch/x86/traps.c                  |  2 +-
 xen/arch/x86/x86_64/mmconfig-shared.c |  2 +-
 xen/common/domain.c                   |  2 +-
 xen/common/kernel.c                   | 11 +++++-
 xen/common/kexec.c                    |  2 +-
 xen/common/numa.c                     |  2 +-
 xen/common/page_alloc.c               |  2 +-
 xen/common/shutdown.c                 |  2 +-
 xen/drivers/char/console.c            |  2 +-
 xen/drivers/char/ns16550.c            |  4 +--
 xen/drivers/video/vga.c               |  2 +-
 xen/include/xen/param.h               | 49 +++++++++++++++++++++------
 23 files changed, 87 insertions(+), 35 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index b0eadd2c5d..7916875f22 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -1798,6 +1798,22 @@ immediately. Specifying `0` will disable all testing of illegal lock nesting.
 
 This option is available for hypervisors built with CONFIG_DEBUG_LOCKS only.
 
+### lockdown
+> `= <boolean>`
+
+> Default: `false`
+
+The intention of lockdown mode is to prevent attacks from a rogue dom0
+userspace from compromising the system. It is also enabled automatically
+when Secure Boot is enabled and it cannot be disabled in that case.
+
+After lockdown mode is enabled some unsafe command line options will be
+ignored by Xen.
+
+If enabling lockdown mode via the command line then ensure it is positioned as
+the first option in the command line string otherwise Xen may process unsafe
+options before reaching the lockdown parameter.
+
 ### loglvl
 > `= <level>[/<rate-limited level>]` where level is `none | error | warning | info | debug | all`
 
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index b189a7cfae..ef1cba8f0f 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -41,7 +41,7 @@
 #include <xen/serial.h>
 
 static unsigned int __initdata opt_dom0_max_vcpus;
-integer_param("dom0_max_vcpus", opt_dom0_max_vcpus);
+integer_secure_param("dom0_max_vcpus", opt_dom0_max_vcpus);
 
 /*
  * If true, the extended regions support is enabled for dom0 and
@@ -61,7 +61,7 @@ static int __init parse_dom0_mem(const char *s)
 
     return *s ? -EINVAL : 0;
 }
-custom_param("dom0_mem", parse_dom0_mem);
+custom_secure_param("dom0_mem", parse_dom0_mem);
 
 int __init parse_arch_dom0_param(const char *s, const char *e)
 {
diff --git a/xen/arch/x86/acpi/cpu_idle.c b/xen/arch/x86/acpi/cpu_idle.c
index 1dbf15b01e..431fd0c997 100644
--- a/xen/arch/x86/acpi/cpu_idle.c
+++ b/xen/arch/x86/acpi/cpu_idle.c
@@ -113,7 +113,7 @@ static int __init cf_check parse_cstate(const char *s)
         max_csubstate = simple_strtoul(s + 1, NULL, 0);
     return 0;
 }
-custom_param("max_cstate", parse_cstate);
+custom_secure_param("max_cstate", parse_cstate);
 
 static bool __read_mostly local_apic_timer_c2_ok;
 boolean_param("lapic_timer_c2_ok", local_apic_timer_c2_ok);
diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 27ae167808..9aab3d05e1 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -47,7 +47,7 @@ integer_param("cpuid_mask_thermal_ecx", opt_cpuid_mask_thermal_ecx);
 
 /* 1 = allow, 0 = don't allow guest creation, -1 = don't allow boot */
 int8_t __read_mostly opt_allow_unsafe;
-boolean_param("allow_unsafe", opt_allow_unsafe);
+boolean_secure_param("allow_unsafe", opt_allow_unsafe);
 
 /* Signal whether the ACPI C1E quirk is required. */
 bool __read_mostly amd_acpi_c1e_quirk;
diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
index 1c348e557d..a229af6fd3 100644
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -31,7 +31,7 @@
 #include "vmce.h"
 
 bool __read_mostly opt_mce = true;
-boolean_param("mce", opt_mce);
+boolean_secure_param("mce", opt_mce);
 bool __read_mostly mce_broadcast;
 bool is_mc_panic;
 DEFINE_PER_CPU_READ_MOSTLY(unsigned int, nr_mce_banks);
diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index 34a94cd25b..b5b7304ae7 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -160,7 +160,7 @@ static int __init cf_check parse_ucode(const char *s)
 
     return rc;
 }
-custom_param("ucode", parse_ucode);
+custom_secure_param("ucode", parse_ucode);
 
 static struct microcode_ops __ro_after_init ucode_ops;
 
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index 0b467fd4a4..6d42acb661 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -142,7 +142,7 @@ static int __init cf_check parse_dom0_mem(const char *s)
 
     return s[-1] ? -EINVAL : ret;
 }
-custom_param("dom0_mem", parse_dom0_mem);
+custom_secure_param("dom0_mem", parse_dom0_mem);
 
 static unsigned int __initdata opt_dom0_max_vcpus_min = 1;
 static unsigned int __initdata opt_dom0_max_vcpus_max = UINT_MAX;
@@ -164,7 +164,7 @@ static int __init cf_check parse_dom0_max_vcpus(const char *s)
 
     return *s ? -EINVAL : 0;
 }
-custom_param("dom0_max_vcpus", parse_dom0_max_vcpus);
+custom_secure_param("dom0_max_vcpus", parse_dom0_max_vcpus);
 
 static __initdata unsigned int dom0_nr_pxms;
 static __initdata unsigned int dom0_pxms[MAX_NUMNODES] =
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 4cb2e13046..97afb274fe 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -87,7 +87,7 @@ unsigned long __section(".bss.page_aligned") __aligned(PAGE_SIZE)
 
 /* Xen command-line option to enable HAP */
 static bool __initdata opt_hap_enabled = true;
-boolean_param("hap", opt_hap_enabled);
+boolean_secure_param("hap", opt_hap_enabled);
 
 #ifndef opt_hvm_fep
 /* Permit use of the Forced Emulation Prefix in HVM guests */
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 556134f85a..44f39f6ece 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -34,7 +34,7 @@
 
 /* opt_noirqbalance: If true, software IRQ balancing/affinity is disabled. */
 bool __read_mostly opt_noirqbalance;
-boolean_param("noirqbalance", opt_noirqbalance);
+boolean_secure_param("noirqbalance", opt_noirqbalance);
 
 unsigned int __read_mostly nr_irqs_gsi = NR_ISA_IRQS;
 unsigned int __read_mostly nr_irqs;
diff --git a/xen/arch/x86/nmi.c b/xen/arch/x86/nmi.c
index 9793fa2316..3735f22e88 100644
--- a/xen/arch/x86/nmi.c
+++ b/xen/arch/x86/nmi.c
@@ -73,7 +73,7 @@ static int __init cf_check parse_watchdog(const char *s)
 
     return 0;
 }
-custom_param("watchdog", parse_watchdog);
+custom_secure_param("watchdog", parse_watchdog);
 
 /* opt_watchdog_timeout: Number of seconds to wait before panic. */
 static unsigned int opt_watchdog_timeout = 5;
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 276957c4ed..1018cdb771 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -70,7 +70,7 @@
 
 /* opt_nosmp: If true, secondary processors are ignored. */
 static bool __initdata opt_nosmp;
-boolean_param("nosmp", opt_nosmp);
+boolean_secure_param("nosmp", opt_nosmp);
 
 /* maxcpus: maximum number of CPUs to activate. */
 static unsigned int __initdata max_cpus;
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index c94779b4ad..7b628d9dc8 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -86,7 +86,7 @@ static char __read_mostly opt_nmi[10] = "dom0";
 #else
 static char __read_mostly opt_nmi[10] = "fatal";
 #endif
-string_param("nmi", opt_nmi);
+string_secure_param("nmi", opt_nmi);
 
 DEFINE_PER_CPU(uint64_t, efer);
 static DEFINE_PER_CPU(unsigned long, last_extable_addr);
diff --git a/xen/arch/x86/x86_64/mmconfig-shared.c b/xen/arch/x86/x86_64/mmconfig-shared.c
index f1a3d42c5b..80cdca7d77 100644
--- a/xen/arch/x86/x86_64/mmconfig-shared.c
+++ b/xen/arch/x86/x86_64/mmconfig-shared.c
@@ -60,7 +60,7 @@ static int __init cf_check parse_mmcfg(const char *s)
 
     return rc;
 }
-custom_param("mmcfg", parse_mmcfg);
+custom_secure_param("mmcfg", parse_mmcfg);
 
 static const char *__init cf_check pci_mmcfg_e7520(void)
 {
diff --git a/xen/common/domain.c b/xen/common/domain.c
index abf1969e60..c95988c067 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -55,7 +55,7 @@ unsigned int xen_processor_pmbits = XEN_PROCESSOR_PM_PX;
 
 /* opt_dom0_vcpus_pin: If true, dom0 VCPUs are pinned. */
 bool opt_dom0_vcpus_pin;
-boolean_param("dom0_vcpus_pin", opt_dom0_vcpus_pin);
+boolean_secure_param("dom0_vcpus_pin", opt_dom0_vcpus_pin);
 
 /* Protect updates/reads (resp.) of domain_list and domain_hash. */
 DEFINE_SPINLOCK(domlist_update_lock);
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 3538f467ad..923ea43cee 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -14,6 +14,8 @@
 #include <xen/guest_access.h>
 #include <xen/hypercall.h>
 #include <xen/hypfs.h>
+#include <xen/efi.h>
+#include <xen/lockdown.h>
 #include <xsm/xsm.h>
 #include <asm/current.h>
 #include <public/version.h>
@@ -135,9 +137,16 @@ static int parse_params(const char *cmdline, const struct kernel_param *start,
                 }
                 continue;
             }
+            found = true;
+
+            if ( !param->is_lockdown_safe && is_locked_down() )
+            {
+                printk("Ignoring unsafe cmdline option %s in lockdown mode\n",
+                       param->name);
+                break;
+            }
 
             rctmp = 0;
-            found = true;
             switch ( param->type )
             {
             case OPT_STR:
diff --git a/xen/common/kexec.c b/xen/common/kexec.c
index 84fe8c3597..790839657d 100644
--- a/xen/common/kexec.c
+++ b/xen/common/kexec.c
@@ -189,7 +189,7 @@ static int __init cf_check parse_crashkernel(const char *str)
 
     return rc;
 }
-custom_param("crashkernel", parse_crashkernel);
+custom_secure_param("crashkernel", parse_crashkernel);
 
 /* Parse command lines in the format:
  *
diff --git a/xen/common/numa.c b/xen/common/numa.c
index ad75955a16..c4981f2ff1 100644
--- a/xen/common/numa.c
+++ b/xen/common/numa.c
@@ -687,7 +687,7 @@ static int __init cf_check numa_setup(const char *opt)
 
     return 0;
 }
-custom_param("numa", numa_setup);
+custom_secure_param("numa", numa_setup);
 
 static void cf_check dump_numa(unsigned char key)
 {
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index e57a287133..a07690d8fd 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -235,7 +235,7 @@ static int __init cf_check parse_bootscrub_param(const char *s)
 
     return 0;
 }
-custom_param("bootscrub", parse_bootscrub_param);
+custom_secure_param("bootscrub", parse_bootscrub_param);
 
 /*
  * bootscrub_chunk -> Amount of bytes to scrub lockstep on non-SMT CPUs
diff --git a/xen/common/shutdown.c b/xen/common/shutdown.c
index c47341b977..231de1454a 100644
--- a/xen/common/shutdown.c
+++ b/xen/common/shutdown.c
@@ -13,7 +13,7 @@
 
 /* opt_noreboot: If true, machine will need manual reset on error. */
 bool __ro_after_init opt_noreboot;
-boolean_param("noreboot", opt_noreboot);
+boolean_secure_param("noreboot", opt_noreboot);
 
 static void noreturn reboot_or_halt(void)
 {
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index c3150fbdb7..45a35903fe 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -43,7 +43,7 @@
 
 /* console: comma-separated list of console outputs. */
 static char __initdata opt_console[30] = OPT_CONSOLE_STR;
-string_param("console", opt_console);
+string_secure_param("console", opt_console);
 
 /* conswitch: a character pair controlling console switching. */
 /* Char 1: CTRL+<char1> is used to switch console input between Xen and DOM0 */
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index eaeb0e09d0..fae509cbd8 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -1390,8 +1390,8 @@ static void enable_exar_enhanced_bits(const struct ns16550 *uart)
  */
 static char __initdata opt_com1[128] = "";
 static char __initdata opt_com2[128] = "";
-string_param("com1", opt_com1);
-string_param("com2", opt_com2);
+string_secure_param("com1", opt_com1);
+string_secure_param("com2", opt_com2);
 
 enum serial_param_type {
     baud_rate,
diff --git a/xen/drivers/video/vga.c b/xen/drivers/video/vga.c
index b577b24619..abc6e56aa3 100644
--- a/xen/drivers/video/vga.c
+++ b/xen/drivers/video/vga.c
@@ -48,7 +48,7 @@ void (*video_puts)(const char *s, size_t nr) = vga_noop_puts;
  * control of the console to domain 0.
  */
 static char __initdata opt_vga[30] = "";
-string_param("vga", opt_vga);
+string_secure_param("vga", opt_vga);
 
 /* VGA text-mode definitions. */
 static unsigned int columns, lines;
diff --git a/xen/include/xen/param.h b/xen/include/xen/param.h
index 1bdbab34ab..31e7326d88 100644
--- a/xen/include/xen/param.h
+++ b/xen/include/xen/param.h
@@ -25,6 +25,7 @@ struct kernel_param {
         void *var;
         int (*func)(const char *s);
     } par;
+    bool is_lockdown_safe;
 };
 
 /* Maximum length of a single parameter string. */
@@ -44,46 +45,72 @@ extern const struct kernel_param __setup_start[], __setup_end[];
 #define _TEMP_NAME(base, line) __TEMP_NAME(base, line)
 #define TEMP_NAME(base) _TEMP_NAME(base, __LINE__)
 
-#define custom_param(_name, _var) \
+#define custom_param_(_name, _var, _sec) \
     __setup_str __setup_str_##_var[] = (_name); \
     __kparam __setup_##_var = \
         { .name = __setup_str_##_var, \
           .type = OPT_CUSTOM, \
-          .par.func = (_var) }
-#define boolean_param(_name, _var) \
+          .par.func = (_var), \
+          .is_lockdown_safe = (_sec) }
+#define custom_param(_name, _var) \
+    custom_param_(_name, _var, false)
+#define custom_secure_param(_name, _var) \
+    custom_param_(_name, _var, true)
+#define boolean_param_(_name, _var, _sec) \
     __setup_str __setup_str_##_var[] = (_name); \
     __kparam __setup_##_var = \
         { .name = __setup_str_##_var, \
           .type = OPT_BOOL, \
           .len = sizeof(_var) + \
                  BUILD_BUG_ON_ZERO(sizeof(_var) != sizeof(bool)), \
-          .par.var = &(_var) }
-#define integer_param(_name, _var) \
+          .par.var = &(_var), \
+          .is_lockdown_safe = (_sec) }
+#define boolean_param(_name, _var) \
+    boolean_param_(_name, _var, false)
+#define boolean_secure_param(_name, _var) \
+    boolean_param_(_name, _var, true)
+#define integer_param_(_name, _var, _sec) \
     __setup_str __setup_str_##_var[] = (_name); \
     __kparam __setup_##_var = \
         { .name = __setup_str_##_var, \
           .type = OPT_UINT, \
           .len = sizeof(_var), \
-          .par.var = &(_var) }
-#define size_param(_name, _var) \
+          .par.var = &(_var), \
+          .is_lockdown_safe = (_sec) }
+#define integer_param(_name, _var) \
+    integer_param_(_name, _var, false)
+#define integer_secure_param(_name, _var) \
+    integer_param_(_name, _var, true)
+#define size_param_(_name, _var, _sec) \
     __setup_str __setup_str_##_var[] = (_name); \
     __kparam __setup_##_var = \
         { .name = __setup_str_##_var, \
           .type = OPT_SIZE, \
           .len = sizeof(_var), \
-          .par.var = &(_var) }
-#define string_param(_name, _var) \
+          .par.var = &(_var), \
+          .is_lockdown_safe = (_sec) }
+#define size_param(_name, _var) \
+    size_param_(_name, _var, false)
+#define size_secure_param(_name, _var) \
+    size_param_(_name, _var, true)
+#define string_param_(_name, _var, _sec) \
     __setup_str __setup_str_##_var[] = (_name); \
     __kparam __setup_##_var = \
         { .name = __setup_str_##_var, \
           .type = OPT_STR, \
           .len = sizeof(_var), \
-          .par.var = &(_var) }
+          .par.var = &(_var), \
+          .is_lockdown_safe = (_sec) }
+#define string_param(_name, _var) \
+  string_param_(_name, _var, false)
+#define string_secure_param(_name, _var) \
+  string_param_(_name, _var, true)
 #define ignore_param(_name)                 \
     __setup_str TEMP_NAME(__setup_str_ign)[] = (_name);  \
     __kparam TEMP_NAME(__setup_ign) =                    \
         { .name = TEMP_NAME(__setup_str_ign),            \
-          .type = OPT_IGNORE }
+          .type = OPT_IGNORE,                            \
+          .is_lockdown_safe = true }
 
 #ifdef CONFIG_HYPFS
 
-- 
2.42.0

[PATCH v2 0/3] Add lockdown mode

[Kevin Lampis]

The intention of lockdown mode is to prevent attacks from a rogue dom0
userspace from compromising the system. Lockdown mode can be controlled by a
Kconfig option and a command-line parameter. It is also enabled automatically
when Secure Boot is enabled and it cannot be disabled in that case.

Ross Lagerwall (2):
  efi: Add a function to check if Secure Boot mode is enabled
  Add lockdown mode

Kevin Lampis (1):
  Disallow most command-line options when lockdown mode is enabled

 docs/misc/xen-command-line.pandoc     | 16 ++++++++
 xen/arch/arm/domain_build.c           |  4 +-
 xen/arch/x86/acpi/cpu_idle.c          |  2 +-
 xen/arch/x86/cpu/amd.c                |  2 +-
 xen/arch/x86/cpu/mcheck/mce.c         |  2 +-
 xen/arch/x86/cpu/microcode/core.c     |  2 +-
 xen/arch/x86/dom0_build.c             |  4 +-
 xen/arch/x86/hvm/hvm.c                |  2 +-
 xen/arch/x86/irq.c                    |  2 +-
 xen/arch/x86/nmi.c                    |  2 +-
 xen/arch/x86/setup.c                  |  3 +-
 xen/arch/x86/traps.c                  |  2 +-
 xen/arch/x86/x86_64/mmconfig-shared.c |  2 +-
 xen/common/Kconfig                    |  8 ++++
 xen/common/Makefile                   |  1 +
 xen/common/domain.c                   |  2 +-
 xen/common/efi/boot.c                 | 23 ++++++++++++
 xen/common/efi/runtime.c              |  3 ++
 xen/common/kernel.c                   | 17 ++++++++-
 xen/common/kexec.c                    |  2 +-
 xen/common/lockdown.c                 | 54 +++++++++++++++++++++++++++
 xen/common/numa.c                     |  2 +-
 xen/common/page_alloc.c               |  2 +-
 xen/common/shutdown.c                 |  2 +-
 xen/drivers/char/console.c            |  2 +-
 xen/drivers/char/ns16550.c            |  4 +-
 xen/drivers/video/vga.c               |  2 +-
 xen/include/xen/efi.h                 |  6 +++
 xen/include/xen/lockdown.h            | 11 ++++++
 xen/include/xen/param.h               | 49 ++++++++++++++++++------
 30 files changed, 200 insertions(+), 35 deletions(-)
 create mode 100644 xen/common/lockdown.c
 create mode 100644 xen/include/xen/lockdown.h

-- 
2.42.0

[PATCH v2 1/3] efi: Add a function to check if Secure Boot mode is enabled

[Kevin Lampis]

From: Ross Lagerwall <ross.lagerwall@citrix.com>

Also cache it to avoid needing to repeatedly ask the firmware.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Signed-off-by: Kevin Lampis <kevin.lampis@cloud.com>
---
Changes in v2:
- None
---
 xen/common/efi/boot.c    | 23 +++++++++++++++++++++++
 xen/common/efi/runtime.c |  3 +++
 xen/include/xen/efi.h    |  6 ++++++
 3 files changed, 32 insertions(+)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index e39fbc3529..7c528cd5dd 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -870,6 +870,27 @@ static void __init pre_parse(const struct file *file)
                    " last line will be ignored.\r\n");
 }
 
+static void __init init_secure_boot_mode(void)
+{
+    EFI_STATUS status;
+    EFI_GUID gv_uuid = EFI_GLOBAL_VARIABLE;
+    uint8_t data = 0;
+    UINTN size = sizeof(data);
+    UINT32 attr = 0;
+    status = efi_rs->GetVariable((CHAR16 *)L"SecureBoot", &gv_uuid, &attr,
+                                 &size, &data);
+
+    if ( status == EFI_NOT_FOUND ||
+         (status == EFI_SUCCESS &&
+          attr == (EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS) &&
+          size == 1 && data == 0) )
+        /* Platform does not support Secure Boot or it's disabled. */
+        efi_secure_boot = false;
+    else
+        /* Everything else play it safe and assume enabled. */
+        efi_secure_boot = true;
+}
+
 static void __init efi_init(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
 {
     efi_ih = ImageHandle;
@@ -884,6 +905,8 @@ static void __init efi_init(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTabl
 
     StdOut = SystemTable->ConOut;
     StdErr = SystemTable->StdErr ?: StdOut;
+
+    init_secure_boot_mode();
 }
 
 static void __init efi_console_set_mode(void)
diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
index 7e1fce291d..b63d21f16c 100644
--- a/xen/common/efi/runtime.c
+++ b/xen/common/efi/runtime.c
@@ -40,6 +40,9 @@ void efi_rs_leave(struct efi_rs_state *state);
 unsigned int __read_mostly efi_num_ct;
 const EFI_CONFIGURATION_TABLE *__read_mostly efi_ct;
 
+#if defined(CONFIG_X86) && !defined(CONFIG_PV_SHIM)
+bool __ro_after_init efi_secure_boot;
+#endif
 unsigned int __read_mostly efi_version;
 unsigned int __read_mostly efi_fw_revision;
 const CHAR16 *__read_mostly efi_fw_vendor;
diff --git a/xen/include/xen/efi.h b/xen/include/xen/efi.h
index 160804e294..ae10ac62d0 100644
--- a/xen/include/xen/efi.h
+++ b/xen/include/xen/efi.h
@@ -40,6 +40,12 @@ static inline bool efi_enabled(unsigned int feature)
 }
 #endif
 
+#if defined(CONFIG_X86) && !defined(CONFIG_PV_SHIM)
+extern bool efi_secure_boot;
+#else
+#define efi_secure_boot false
+#endif
+
 void efi_init_memory(void);
 bool efi_boot_mem_unused(unsigned long *start, unsigned long *end);
 bool efi_rs_using_pgtables(void);
-- 
2.42.0

[PATCH v2 2/3] Add lockdown mode

[Kevin Lampis]

From: Ross Lagerwall <ross.lagerwall@citrix.com>

The intention of lockdown mode is to prevent attacks from a rogue dom0
userspace from compromising the system. Lockdown mode can be controlled by a
Kconfig option and a command-line parameter. It is also enabled automatically
when Secure Boot is enabled and it cannot be disabled in that case.

If enabled from the command-line then it is required to be first in the
list otherwise Xen may process some insecure parameters before reaching
the lockdown parameter.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Signed-off-by: Kevin Lampis <kevin.lampis@cloud.com>
---
Changes in v2:
- Remove custom command line parsing
- Print warning if lockdown is not first on command line
---
 xen/arch/x86/setup.c       |  1 +
 xen/common/Kconfig         |  8 ++++++
 xen/common/Makefile        |  1 +
 xen/common/kernel.c        |  7 +++++
 xen/common/lockdown.c      | 54 ++++++++++++++++++++++++++++++++++++++
 xen/include/xen/lockdown.h | 11 ++++++++
 6 files changed, 82 insertions(+)
 create mode 100644 xen/common/lockdown.c
 create mode 100644 xen/include/xen/lockdown.h

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 1f5cb67bd0..efeed5eafc 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -15,6 +15,7 @@
 #include <xen/kexec.h>
 #include <xen/keyhandler.h>
 #include <xen/lib.h>
+#include <xen/lockdown.h>
 #include <xen/multiboot.h>
 #include <xen/nodemask.h>
 #include <xen/numa.h>
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 0951d4c2f2..33cd669110 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -587,4 +587,12 @@ config BUDDY_ALLOCATOR_SIZE
 	  Amount of memory reserved for the buddy allocator to serve Xen heap,
 	  working alongside the colored one.
 
+config LOCKDOWN_DEFAULT
+       bool "Enable lockdown mode by default"
+       default n
+       help
+         Lockdown mode prevents attacks from a rogue dom0 userspace from
+         compromising the system. This is automatically enabled when Secure
+         Boot is enabled.
+
 endmenu
diff --git a/xen/common/Makefile b/xen/common/Makefile
index 98f0873056..b00a8a925a 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_KEXEC) += kexec.o
 obj-$(CONFIG_KEXEC) += kimage.o
 obj-$(CONFIG_LIVEPATCH) += livepatch.o livepatch_elf.o
 obj-$(CONFIG_LLC_COLORING) += llc-coloring.o
+obj-y += lockdown.o
 obj-$(CONFIG_VM_EVENT) += mem_access.o
 obj-y += memory.o
 obj-y += multicall.o
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 8b63ca55f1..7280da987d 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -14,6 +14,7 @@
 #include <xen/guest_access.h>
 #include <xen/hypercall.h>
 #include <xen/hypfs.h>
+#include <xen/lockdown.h>
 #include <xsm/xsm.h>
 #include <asm/current.h>
 #include <public/version.h>
@@ -199,6 +200,8 @@ static int parse_params(const char *cmdline, const struct kernel_param *start,
             printk("parameter \"%s\" unknown!\n", key);
             final_rc = -EINVAL;
         }
+
+        lockdown_clear_first_flag();
     }
 
     return final_rc;
@@ -216,6 +219,9 @@ static void __init _cmdline_parse(const char *cmdline)
  */
 void __init cmdline_parse(const char *cmdline)
 {
+    /* Call this early since it affects command-line parsing */
+    lockdown_init(cmdline);
+
     if ( opt_builtin_cmdline[0] )
     {
         printk("Built-in command line: %s\n", opt_builtin_cmdline);
@@ -227,6 +233,7 @@ void __init cmdline_parse(const char *cmdline)
         return;
 
     safe_strcpy(saved_cmdline, cmdline);
+    lockdown_set_first_flag();
     _cmdline_parse(cmdline);
 #endif
 }
diff --git a/xen/common/lockdown.c b/xen/common/lockdown.c
new file mode 100644
index 0000000000..84eabe9c83
--- /dev/null
+++ b/xen/common/lockdown.c
@@ -0,0 +1,54 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#include <xen/efi.h>
+#include <xen/lockdown.h>
+#include <xen/param.h>
+
+#define FIRST_ARG_FLAG 2
+
+static int __ro_after_init lockdown = IS_ENABLED(CONFIG_LOCKDOWN_DEFAULT);
+
+void __init lockdown_set_first_flag(void)
+{
+    lockdown |= FIRST_ARG_FLAG;
+}
+
+void __init lockdown_clear_first_flag(void)
+{
+    lockdown &= ~FIRST_ARG_FLAG;
+}
+
+static int __init parse_lockdown_opt(const char *s)
+{
+    if ( strncmp("no", s, 2) == 0 )
+        if ( efi_secure_boot )
+            printk("lockdown can't be disabled because Xen booted in Secure Boot mode\n");
+        else
+            lockdown = 0;
+    else
+    {
+        if ( !(lockdown & FIRST_ARG_FLAG) )
+            printk("lockdown was not the first argument, unsafe arguments may have been already processed\n");
+
+        lockdown = 1;
+    }
+
+    return 0;
+}
+custom_param("lockdown", parse_lockdown_opt);
+
+bool is_locked_down(void)
+{
+    return lockdown & ~FIRST_ARG_FLAG;
+}
+
+void __init lockdown_init(const char *cmdline)
+{
+    if ( efi_secure_boot )
+    {
+        printk("Enabling lockdown mode because Secure Boot is enabled\n");
+        lockdown = 1;
+    }
+
+    printk("Lockdown mode is %s\n", is_locked_down() ? "enabled" : "disabled");
+}
diff --git a/xen/include/xen/lockdown.h b/xen/include/xen/lockdown.h
new file mode 100644
index 0000000000..6ae97f9d5f
--- /dev/null
+++ b/xen/include/xen/lockdown.h
@@ -0,0 +1,11 @@
+#ifndef XEN__LOCKDOWN_H
+#define XEN__LOCKDOWN_H
+
+#include <xen/types.h>
+
+void lockdown_set_first_flag(void);
+void lockdown_clear_first_flag(void);
+bool is_locked_down(void);
+void lockdown_init(const char *cmdline);
+
+#endif /* XEN__LOCKDOWN_H */
-- 
2.42.0

[PATCH v2 3/3] Disallow most command-line options when lockdown mode is enabled

[Kevin Lampis]

A subset of command-line parameters that are specifically safe to use when
lockdown mode is enabled are annotated as such.

These are commonly used parameters which have been audited to ensure they
cannot be used to undermine the integrity of the system when booted in
Secure Boot mode.

Signed-off-by: Kevin Lampis <kevin.lampis@cloud.com>
Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
Changes in v2:
- Add more information about the safe parameters
- Add lockdown section to the command line doc
---
 docs/misc/xen-command-line.pandoc     | 16 +++++++++
 xen/arch/arm/domain_build.c           |  4 +--
 xen/arch/x86/acpi/cpu_idle.c          |  2 +-
 xen/arch/x86/cpu/amd.c                |  2 +-
 xen/arch/x86/cpu/mcheck/mce.c         |  2 +-
 xen/arch/x86/cpu/microcode/core.c     |  2 +-
 xen/arch/x86/dom0_build.c             |  4 +--
 xen/arch/x86/hvm/hvm.c                |  2 +-
 xen/arch/x86/irq.c                    |  2 +-
 xen/arch/x86/nmi.c                    |  2 +-
 xen/arch/x86/setup.c                  |  2 +-
 xen/arch/x86/traps.c                  |  2 +-
 xen/arch/x86/x86_64/mmconfig-shared.c |  2 +-
 xen/common/domain.c                   |  2 +-
 xen/common/kernel.c                   | 10 +++++-
 xen/common/kexec.c                    |  2 +-
 xen/common/lockdown.c                 |  2 +-
 xen/common/numa.c                     |  2 +-
 xen/common/page_alloc.c               |  2 +-
 xen/common/shutdown.c                 |  2 +-
 xen/drivers/char/console.c            |  2 +-
 xen/drivers/char/ns16550.c            |  4 +--
 xen/drivers/video/vga.c               |  2 +-
 xen/include/xen/param.h               | 49 +++++++++++++++++++++------
 24 files changed, 87 insertions(+), 36 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index b0eadd2c5d..7916875f22 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -1798,6 +1798,22 @@ immediately. Specifying `0` will disable all testing of illegal lock nesting.
 
 This option is available for hypervisors built with CONFIG_DEBUG_LOCKS only.
 
+### lockdown
+> `= <boolean>`
+
+> Default: `false`
+
+The intention of lockdown mode is to prevent attacks from a rogue dom0
+userspace from compromising the system. It is also enabled automatically
+when Secure Boot is enabled and it cannot be disabled in that case.
+
+After lockdown mode is enabled some unsafe command line options will be
+ignored by Xen.
+
+If enabling lockdown mode via the command line then ensure it is positioned as
+the first option in the command line string otherwise Xen may process unsafe
+options before reaching the lockdown parameter.
+
 ### loglvl
 > `= <level>[/<rate-limited level>]` where level is `none | error | warning | info | debug | all`
 
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index b189a7cfae..ef1cba8f0f 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -41,7 +41,7 @@
 #include <xen/serial.h>
 
 static unsigned int __initdata opt_dom0_max_vcpus;
-integer_param("dom0_max_vcpus", opt_dom0_max_vcpus);
+integer_secure_param("dom0_max_vcpus", opt_dom0_max_vcpus);
 
 /*
  * If true, the extended regions support is enabled for dom0 and
@@ -61,7 +61,7 @@ static int __init parse_dom0_mem(const char *s)
 
     return *s ? -EINVAL : 0;
 }
-custom_param("dom0_mem", parse_dom0_mem);
+custom_secure_param("dom0_mem", parse_dom0_mem);
 
 int __init parse_arch_dom0_param(const char *s, const char *e)
 {
diff --git a/xen/arch/x86/acpi/cpu_idle.c b/xen/arch/x86/acpi/cpu_idle.c
index 1dbf15b01e..431fd0c997 100644
--- a/xen/arch/x86/acpi/cpu_idle.c
+++ b/xen/arch/x86/acpi/cpu_idle.c
@@ -113,7 +113,7 @@ static int __init cf_check parse_cstate(const char *s)
         max_csubstate = simple_strtoul(s + 1, NULL, 0);
     return 0;
 }
-custom_param("max_cstate", parse_cstate);
+custom_secure_param("max_cstate", parse_cstate);
 
 static bool __read_mostly local_apic_timer_c2_ok;
 boolean_param("lapic_timer_c2_ok", local_apic_timer_c2_ok);
diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 27ae167808..9aab3d05e1 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -47,7 +47,7 @@ integer_param("cpuid_mask_thermal_ecx", opt_cpuid_mask_thermal_ecx);
 
 /* 1 = allow, 0 = don't allow guest creation, -1 = don't allow boot */
 int8_t __read_mostly opt_allow_unsafe;
-boolean_param("allow_unsafe", opt_allow_unsafe);
+boolean_secure_param("allow_unsafe", opt_allow_unsafe);
 
 /* Signal whether the ACPI C1E quirk is required. */
 bool __read_mostly amd_acpi_c1e_quirk;
diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
index 1c348e557d..a229af6fd3 100644
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -31,7 +31,7 @@
 #include "vmce.h"
 
 bool __read_mostly opt_mce = true;
-boolean_param("mce", opt_mce);
+boolean_secure_param("mce", opt_mce);
 bool __read_mostly mce_broadcast;
 bool is_mc_panic;
 DEFINE_PER_CPU_READ_MOSTLY(unsigned int, nr_mce_banks);
diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index 34a94cd25b..b5b7304ae7 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -160,7 +160,7 @@ static int __init cf_check parse_ucode(const char *s)
 
     return rc;
 }
-custom_param("ucode", parse_ucode);
+custom_secure_param("ucode", parse_ucode);
 
 static struct microcode_ops __ro_after_init ucode_ops;
 
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index 0b467fd4a4..6d42acb661 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -142,7 +142,7 @@ static int __init cf_check parse_dom0_mem(const char *s)
 
     return s[-1] ? -EINVAL : ret;
 }
-custom_param("dom0_mem", parse_dom0_mem);
+custom_secure_param("dom0_mem", parse_dom0_mem);
 
 static unsigned int __initdata opt_dom0_max_vcpus_min = 1;
 static unsigned int __initdata opt_dom0_max_vcpus_max = UINT_MAX;
@@ -164,7 +164,7 @@ static int __init cf_check parse_dom0_max_vcpus(const char *s)
 
     return *s ? -EINVAL : 0;
 }
-custom_param("dom0_max_vcpus", parse_dom0_max_vcpus);
+custom_secure_param("dom0_max_vcpus", parse_dom0_max_vcpus);
 
 static __initdata unsigned int dom0_nr_pxms;
 static __initdata unsigned int dom0_pxms[MAX_NUMNODES] =
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 4cb2e13046..97afb274fe 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -87,7 +87,7 @@ unsigned long __section(".bss.page_aligned") __aligned(PAGE_SIZE)
 
 /* Xen command-line option to enable HAP */
 static bool __initdata opt_hap_enabled = true;
-boolean_param("hap", opt_hap_enabled);
+boolean_secure_param("hap", opt_hap_enabled);
 
 #ifndef opt_hvm_fep
 /* Permit use of the Forced Emulation Prefix in HVM guests */
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 556134f85a..44f39f6ece 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -34,7 +34,7 @@
 
 /* opt_noirqbalance: If true, software IRQ balancing/affinity is disabled. */
 bool __read_mostly opt_noirqbalance;
-boolean_param("noirqbalance", opt_noirqbalance);
+boolean_secure_param("noirqbalance", opt_noirqbalance);
 
 unsigned int __read_mostly nr_irqs_gsi = NR_ISA_IRQS;
 unsigned int __read_mostly nr_irqs;
diff --git a/xen/arch/x86/nmi.c b/xen/arch/x86/nmi.c
index 9793fa2316..3735f22e88 100644
--- a/xen/arch/x86/nmi.c
+++ b/xen/arch/x86/nmi.c
@@ -73,7 +73,7 @@ static int __init cf_check parse_watchdog(const char *s)
 
     return 0;
 }
-custom_param("watchdog", parse_watchdog);
+custom_secure_param("watchdog", parse_watchdog);
 
 /* opt_watchdog_timeout: Number of seconds to wait before panic. */
 static unsigned int opt_watchdog_timeout = 5;
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index efeed5eafc..adfaf8667b 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -71,7 +71,7 @@
 
 /* opt_nosmp: If true, secondary processors are ignored. */
 static bool __initdata opt_nosmp;
-boolean_param("nosmp", opt_nosmp);
+boolean_secure_param("nosmp", opt_nosmp);
 
 /* maxcpus: maximum number of CPUs to activate. */
 static unsigned int __initdata max_cpus;
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 092c7e4197..1a1ce541c3 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -61,7 +61,7 @@ static char __read_mostly opt_nmi[10] = "dom0";
 #else
 static char __read_mostly opt_nmi[10] = "fatal";
 #endif
-string_param("nmi", opt_nmi);
+string_secure_param("nmi", opt_nmi);
 
 DEFINE_PER_CPU(uint64_t, efer);
 static DEFINE_PER_CPU(unsigned long, last_extable_addr);
diff --git a/xen/arch/x86/x86_64/mmconfig-shared.c b/xen/arch/x86/x86_64/mmconfig-shared.c
index f1a3d42c5b..80cdca7d77 100644
--- a/xen/arch/x86/x86_64/mmconfig-shared.c
+++ b/xen/arch/x86/x86_64/mmconfig-shared.c
@@ -60,7 +60,7 @@ static int __init cf_check parse_mmcfg(const char *s)
 
     return rc;
 }
-custom_param("mmcfg", parse_mmcfg);
+custom_secure_param("mmcfg", parse_mmcfg);
 
 static const char *__init cf_check pci_mmcfg_e7520(void)
 {
diff --git a/xen/common/domain.c b/xen/common/domain.c
index abf1969e60..c95988c067 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -55,7 +55,7 @@ unsigned int xen_processor_pmbits = XEN_PROCESSOR_PM_PX;
 
 /* opt_dom0_vcpus_pin: If true, dom0 VCPUs are pinned. */
 bool opt_dom0_vcpus_pin;
-boolean_param("dom0_vcpus_pin", opt_dom0_vcpus_pin);
+boolean_secure_param("dom0_vcpus_pin", opt_dom0_vcpus_pin);
 
 /* Protect updates/reads (resp.) of domain_list and domain_hash. */
 DEFINE_SPINLOCK(domlist_update_lock);
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 7280da987d..923ea43cee 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -14,6 +14,7 @@
 #include <xen/guest_access.h>
 #include <xen/hypercall.h>
 #include <xen/hypfs.h>
+#include <xen/efi.h>
 #include <xen/lockdown.h>
 #include <xsm/xsm.h>
 #include <asm/current.h>
@@ -136,9 +137,16 @@ static int parse_params(const char *cmdline, const struct kernel_param *start,
                 }
                 continue;
             }
+            found = true;
+
+            if ( !param->is_lockdown_safe && is_locked_down() )
+            {
+                printk("Ignoring unsafe cmdline option %s in lockdown mode\n",
+                       param->name);
+                break;
+            }
 
             rctmp = 0;
-            found = true;
             switch ( param->type )
             {
             case OPT_STR:
diff --git a/xen/common/kexec.c b/xen/common/kexec.c
index 84fe8c3597..790839657d 100644
--- a/xen/common/kexec.c
+++ b/xen/common/kexec.c
@@ -189,7 +189,7 @@ static int __init cf_check parse_crashkernel(const char *str)
 
     return rc;
 }
-custom_param("crashkernel", parse_crashkernel);
+custom_secure_param("crashkernel", parse_crashkernel);
 
 /* Parse command lines in the format:
  *
diff --git a/xen/common/lockdown.c b/xen/common/lockdown.c
index 84eabe9c83..cd3deeb63e 100644
--- a/xen/common/lockdown.c
+++ b/xen/common/lockdown.c
@@ -35,7 +35,7 @@ static int __init parse_lockdown_opt(const char *s)
 
     return 0;
 }
-custom_param("lockdown", parse_lockdown_opt);
+custom_secure_param("lockdown", parse_lockdown_opt);
 
 bool is_locked_down(void)
 {
diff --git a/xen/common/numa.c b/xen/common/numa.c
index ad75955a16..c4981f2ff1 100644
--- a/xen/common/numa.c
+++ b/xen/common/numa.c
@@ -687,7 +687,7 @@ static int __init cf_check numa_setup(const char *opt)
 
     return 0;
 }
-custom_param("numa", numa_setup);
+custom_secure_param("numa", numa_setup);
 
 static void cf_check dump_numa(unsigned char key)
 {
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index e57a287133..a07690d8fd 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -235,7 +235,7 @@ static int __init cf_check parse_bootscrub_param(const char *s)
 
     return 0;
 }
-custom_param("bootscrub", parse_bootscrub_param);
+custom_secure_param("bootscrub", parse_bootscrub_param);
 
 /*
  * bootscrub_chunk -> Amount of bytes to scrub lockstep on non-SMT CPUs
diff --git a/xen/common/shutdown.c b/xen/common/shutdown.c
index c47341b977..231de1454a 100644
--- a/xen/common/shutdown.c
+++ b/xen/common/shutdown.c
@@ -13,7 +13,7 @@
 
 /* opt_noreboot: If true, machine will need manual reset on error. */
 bool __ro_after_init opt_noreboot;
-boolean_param("noreboot", opt_noreboot);
+boolean_secure_param("noreboot", opt_noreboot);
 
 static void noreturn reboot_or_halt(void)
 {
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 30701ae0b0..4f4f4aab19 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -65,7 +65,7 @@ static void console_send(const char *str, size_t len, unsigned int flags);
 
 /* console: comma-separated list of console outputs. */
 static char __initdata opt_console[30] = OPT_CONSOLE_STR;
-string_param("console", opt_console);
+string_secure_param("console", opt_console);
 
 /* conswitch: a character pair controlling console switching. */
 /* Char 1: CTRL+<char1> is used to switch console input between Xen and DOM0 */
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index eaeb0e09d0..fae509cbd8 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -1390,8 +1390,8 @@ static void enable_exar_enhanced_bits(const struct ns16550 *uart)
  */
 static char __initdata opt_com1[128] = "";
 static char __initdata opt_com2[128] = "";
-string_param("com1", opt_com1);
-string_param("com2", opt_com2);
+string_secure_param("com1", opt_com1);
+string_secure_param("com2", opt_com2);
 
 enum serial_param_type {
     baud_rate,
diff --git a/xen/drivers/video/vga.c b/xen/drivers/video/vga.c
index b577b24619..abc6e56aa3 100644
--- a/xen/drivers/video/vga.c
+++ b/xen/drivers/video/vga.c
@@ -48,7 +48,7 @@ void (*video_puts)(const char *s, size_t nr) = vga_noop_puts;
  * control of the console to domain 0.
  */
 static char __initdata opt_vga[30] = "";
-string_param("vga", opt_vga);
+string_secure_param("vga", opt_vga);
 
 /* VGA text-mode definitions. */
 static unsigned int columns, lines;
diff --git a/xen/include/xen/param.h b/xen/include/xen/param.h
index 1bdbab34ab..31e7326d88 100644
--- a/xen/include/xen/param.h
+++ b/xen/include/xen/param.h
@@ -25,6 +25,7 @@ struct kernel_param {
         void *var;
         int (*func)(const char *s);
     } par;
+    bool is_lockdown_safe;
 };
 
 /* Maximum length of a single parameter string. */
@@ -44,46 +45,72 @@ extern const struct kernel_param __setup_start[], __setup_end[];
 #define _TEMP_NAME(base, line) __TEMP_NAME(base, line)
 #define TEMP_NAME(base) _TEMP_NAME(base, __LINE__)
 
-#define custom_param(_name, _var) \
+#define custom_param_(_name, _var, _sec) \
     __setup_str __setup_str_##_var[] = (_name); \
     __kparam __setup_##_var = \
         { .name = __setup_str_##_var, \
           .type = OPT_CUSTOM, \
-          .par.func = (_var) }
-#define boolean_param(_name, _var) \
+          .par.func = (_var), \
+          .is_lockdown_safe = (_sec) }
+#define custom_param(_name, _var) \
+    custom_param_(_name, _var, false)
+#define custom_secure_param(_name, _var) \
+    custom_param_(_name, _var, true)
+#define boolean_param_(_name, _var, _sec) \
     __setup_str __setup_str_##_var[] = (_name); \
     __kparam __setup_##_var = \
         { .name = __setup_str_##_var, \
           .type = OPT_BOOL, \
           .len = sizeof(_var) + \
                  BUILD_BUG_ON_ZERO(sizeof(_var) != sizeof(bool)), \
-          .par.var = &(_var) }
-#define integer_param(_name, _var) \
+          .par.var = &(_var), \
+          .is_lockdown_safe = (_sec) }
+#define boolean_param(_name, _var) \
+    boolean_param_(_name, _var, false)
+#define boolean_secure_param(_name, _var) \
+    boolean_param_(_name, _var, true)
+#define integer_param_(_name, _var, _sec) \
     __setup_str __setup_str_##_var[] = (_name); \
     __kparam __setup_##_var = \
         { .name = __setup_str_##_var, \
           .type = OPT_UINT, \
           .len = sizeof(_var), \
-          .par.var = &(_var) }
-#define size_param(_name, _var) \
+          .par.var = &(_var), \
+          .is_lockdown_safe = (_sec) }
+#define integer_param(_name, _var) \
+    integer_param_(_name, _var, false)
+#define integer_secure_param(_name, _var) \
+    integer_param_(_name, _var, true)
+#define size_param_(_name, _var, _sec) \
     __setup_str __setup_str_##_var[] = (_name); \
     __kparam __setup_##_var = \
         { .name = __setup_str_##_var, \
           .type = OPT_SIZE, \
           .len = sizeof(_var), \
-          .par.var = &(_var) }
-#define string_param(_name, _var) \
+          .par.var = &(_var), \
+          .is_lockdown_safe = (_sec) }
+#define size_param(_name, _var) \
+    size_param_(_name, _var, false)
+#define size_secure_param(_name, _var) \
+    size_param_(_name, _var, true)
+#define string_param_(_name, _var, _sec) \
     __setup_str __setup_str_##_var[] = (_name); \
     __kparam __setup_##_var = \
         { .name = __setup_str_##_var, \
           .type = OPT_STR, \
           .len = sizeof(_var), \
-          .par.var = &(_var) }
+          .par.var = &(_var), \
+          .is_lockdown_safe = (_sec) }
+#define string_param(_name, _var) \
+  string_param_(_name, _var, false)
+#define string_secure_param(_name, _var) \
+  string_param_(_name, _var, true)
 #define ignore_param(_name)                 \
     __setup_str TEMP_NAME(__setup_str_ign)[] = (_name);  \
     __kparam TEMP_NAME(__setup_ign) =                    \
         { .name = TEMP_NAME(__setup_str_ign),            \
-          .type = OPT_IGNORE }
+          .type = OPT_IGNORE,                            \
+          .is_lockdown_safe = true }
 
 #ifdef CONFIG_HYPFS
 
-- 
2.42.0

Re: [PATCH v2 3/3] Disallow most command-line options when lockdown mode is enabled

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 3810 bytes --]

On Mon, Jun 02, 2025 at 02:46:56PM +0100, Kevin Lampis wrote:
> A subset of command-line parameters that are specifically safe to use when
> lockdown mode is enabled are annotated as such.
> 
> These are commonly used parameters which have been audited to ensure they
> cannot be used to undermine the integrity of the system when booted in
> Secure Boot mode.
> 
> Signed-off-by: Kevin Lampis <kevin.lampis@cloud.com>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
> Changes in v2:
> - Add more information about the safe parameters
> - Add lockdown section to the command line doc
> ---
>  docs/misc/xen-command-line.pandoc     | 16 +++++++++
>  xen/arch/arm/domain_build.c           |  4 +--
>  xen/arch/x86/acpi/cpu_idle.c          |  2 +-
>  xen/arch/x86/cpu/amd.c                |  2 +-
>  xen/arch/x86/cpu/mcheck/mce.c         |  2 +-
>  xen/arch/x86/cpu/microcode/core.c     |  2 +-
>  xen/arch/x86/dom0_build.c             |  4 +--
>  xen/arch/x86/hvm/hvm.c                |  2 +-
>  xen/arch/x86/irq.c                    |  2 +-
>  xen/arch/x86/nmi.c                    |  2 +-
>  xen/arch/x86/setup.c                  |  2 +-
>  xen/arch/x86/traps.c                  |  2 +-
>  xen/arch/x86/x86_64/mmconfig-shared.c |  2 +-
>  xen/common/domain.c                   |  2 +-
>  xen/common/kernel.c                   | 10 +++++-
>  xen/common/kexec.c                    |  2 +-
>  xen/common/lockdown.c                 |  2 +-
>  xen/common/numa.c                     |  2 +-
>  xen/common/page_alloc.c               |  2 +-
>  xen/common/shutdown.c                 |  2 +-
>  xen/drivers/char/console.c            |  2 +-
>  xen/drivers/char/ns16550.c            |  4 +--
>  xen/drivers/video/vga.c               |  2 +-
>  xen/include/xen/param.h               | 49 +++++++++++++++++++++------
>  24 files changed, 87 insertions(+), 36 deletions(-)
> 
> diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
> index b0eadd2c5d..7916875f22 100644
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1798,6 +1798,22 @@ immediately. Specifying `0` will disable all testing of illegal lock nesting.
>  
>  This option is available for hypervisors built with CONFIG_DEBUG_LOCKS only.
>  
> +### lockdown
> +> `= <boolean>`
> +
> +> Default: `false`

This belongs to the 2/3 patch, no?

> +
> +The intention of lockdown mode is to prevent attacks from a rogue dom0
> +userspace from compromising the system. It is also enabled automatically
> +when Secure Boot is enabled and it cannot be disabled in that case.
> +
> +After lockdown mode is enabled some unsafe command line options will be
> +ignored by Xen.
> +
> +If enabling lockdown mode via the command line then ensure it is positioned as
> +the first option in the command line string otherwise Xen may process unsafe
> +options before reaching the lockdown parameter.
> +
>  ### loglvl
>  > `= <level>[/<rate-limited level>]` where level is `none | error | warning | info | debug | all`
>  
...
> diff --git a/xen/common/lockdown.c b/xen/common/lockdown.c
> index 84eabe9c83..cd3deeb63e 100644
> --- a/xen/common/lockdown.c
> +++ b/xen/common/lockdown.c
> @@ -35,7 +35,7 @@ static int __init parse_lockdown_opt(const char *s)
>  
>      return 0;
>  }
> -custom_param("lockdown", parse_lockdown_opt);
> +custom_secure_param("lockdown", parse_lockdown_opt);

Is that really a good idea? It means `lockdown=yes lockdown=no` would
still disable it in the end. This may matter more if for example the
`lockdown=yes` part is in the built-in cmdline (possibly with other
integrity protection than UEFI SB).

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/3] Add lockdown mode

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 2275 bytes --]

On Mon, Jun 02, 2025 at 02:46:55PM +0100, Kevin Lampis wrote:
> From: Ross Lagerwall <ross.lagerwall@citrix.com>
> 
> The intention of lockdown mode is to prevent attacks from a rogue dom0
> userspace from compromising the system. Lockdown mode can be controlled by a
> Kconfig option and a command-line parameter. It is also enabled automatically
> when Secure Boot is enabled and it cannot be disabled in that case.
> 
> If enabled from the command-line then it is required to be first in the
> list otherwise Xen may process some insecure parameters before reaching
> the lockdown parameter.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> Signed-off-by: Kevin Lampis <kevin.lampis@cloud.com>
> ---
> Changes in v2:
> - Remove custom command line parsing
> - Print warning if lockdown is not first on command line
> ---
...

> diff --git a/xen/common/lockdown.c b/xen/common/lockdown.c
> new file mode 100644
> index 0000000000..84eabe9c83
> --- /dev/null
> +++ b/xen/common/lockdown.c
> @@ -0,0 +1,54 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#include <xen/efi.h>
> +#include <xen/lockdown.h>
> +#include <xen/param.h>
> +
> +#define FIRST_ARG_FLAG 2
> +
> +static int __ro_after_init lockdown = IS_ENABLED(CONFIG_LOCKDOWN_DEFAULT);
> +
> +void __init lockdown_set_first_flag(void)
> +{
> +    lockdown |= FIRST_ARG_FLAG;
> +}
> +
> +void __init lockdown_clear_first_flag(void)
> +{
> +    lockdown &= ~FIRST_ARG_FLAG;
> +}
> +
> +static int __init parse_lockdown_opt(const char *s)
> +{
> +    if ( strncmp("no", s, 2) == 0 )

This is rather inconsistent with other bool options. I think you want to
use parse_bool() here.

> +        if ( efi_secure_boot )
> +            printk("lockdown can't be disabled because Xen booted in Secure Boot mode\n");
> +        else
> +            lockdown = 0;
> +    else
> +    {
> +        if ( !(lockdown & FIRST_ARG_FLAG) )
> +            printk("lockdown was not the first argument, unsafe arguments may have been already processed\n");
> +
> +        lockdown = 1;
> +    }
> +
> +    return 0;
> +}
> +custom_param("lockdown", parse_lockdown_opt);

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 3/3] Disallow most command-line options when lockdown mode is enabled

[Jan Beulich]

On 02.06.2025 16:16, Marek Marczykowski-Górecki wrote:
> On Mon, Jun 02, 2025 at 02:46:56PM +0100, Kevin Lampis wrote:
>> --- a/xen/common/lockdown.c
>> +++ b/xen/common/lockdown.c
>> @@ -35,7 +35,7 @@ static int __init parse_lockdown_opt(const char *s)
>>  
>>      return 0;
>>  }
>> -custom_param("lockdown", parse_lockdown_opt);
>> +custom_secure_param("lockdown", parse_lockdown_opt);
> 
> Is that really a good idea? It means `lockdown=yes lockdown=no` would
> still disable it in the end. This may matter more if for example the
> `lockdown=yes` part is in the built-in cmdline (possibly with other
> integrity protection than UEFI SB).

But having a way to override an earlier "lockdown" by "lockdown=no" is
intended? E.g. when your xen.cfg has the former, but you don't really
want that (for, say, an experiment).

Jan

Re: [PATCH v2 2/3] Add lockdown mode

[Kevin Lampis]

On Mon, Jun 2, 2025 at 3:20 PM Marek Marczykowski-Górecki
<marmarek@invisiblethingslab.com> wrote:
>
> This is rather inconsistent with other bool options. I think you want to
> use parse_bool() here.

Thank you. I will use that instead.

Re: [PATCH v2 1/3] efi: Add a function to check if Secure Boot mode is enabled

[Andrew Cooper]

On 02/06/2025 2:46 pm, Kevin Lampis wrote:
> From: Ross Lagerwall <ross.lagerwall@citrix.com>
>
> Also cache it to avoid needing to repeatedly ask the firmware.
>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> Signed-off-by: Kevin Lampis <kevin.lampis@cloud.com>

You must CC the maintainers on patches.  If in doubt, use
./scripts/get_maintainer.pl.

> diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
> index 7e1fce291d..b63d21f16c 100644
> --- a/xen/common/efi/runtime.c
> +++ b/xen/common/efi/runtime.c
> @@ -40,6 +40,9 @@ void efi_rs_leave(struct efi_rs_state *state);
>  unsigned int __read_mostly efi_num_ct;
>  const EFI_CONFIGURATION_TABLE *__read_mostly efi_ct;
>  
> +#if defined(CONFIG_X86) && !defined(CONFIG_PV_SHIM)
> +bool __ro_after_init efi_secure_boot;
> +#endif

This doesn't build on ARM

arch/arm/efi/boot.c: In function ‘init_secure_boot_mode’:
arch/arm/efi/boot.c:888:25: error: lvalue required as left operand of
assignment
  888 |         efi_secure_boot = false;
      |                         ^
arch/arm/efi/boot.c:891:25: error: lvalue required as left operand of
assignment
  891 |         efi_secure_boot = true;
      |                         ^
make[3]: *** [Rules.mk:249: arch/arm/efi/boot.o] Error 1

I also don't see an answer to why there's a CONFIG_PV_SHIM special
case.  Shim has nothing to do with this.

~Andrew

Re: [PATCH v2 3/3] Disallow most command-line options when lockdown mode is enabled

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 1599 bytes --]

On Mon, Jun 02, 2025 at 04:22:06PM +0200, Jan Beulich wrote:
> On 02.06.2025 16:16, Marek Marczykowski-Górecki wrote:
> > On Mon, Jun 02, 2025 at 02:46:56PM +0100, Kevin Lampis wrote:
> >> --- a/xen/common/lockdown.c
> >> +++ b/xen/common/lockdown.c
> >> @@ -35,7 +35,7 @@ static int __init parse_lockdown_opt(const char *s)
> >>  
> >>      return 0;
> >>  }
> >> -custom_param("lockdown", parse_lockdown_opt);
> >> +custom_secure_param("lockdown", parse_lockdown_opt);
> > 
> > Is that really a good idea? It means `lockdown=yes lockdown=no` would
> > still disable it in the end. This may matter more if for example the
> > `lockdown=yes` part is in the built-in cmdline (possibly with other
> > integrity protection than UEFI SB).
> 
> But having a way to override an earlier "lockdown" by "lockdown=no" is
> intended? E.g. when your xen.cfg has the former, but you don't really
> want that (for, say, an experiment).

Ok, I guess those are conflicting use cases: using "lockdown" option to
restrict what user can set via bootloader menu (even without
secureboot), vs giving the local user full control (developer case). But
in that latter case, maybe you can simply remove the "lockdown" option
instead of adding "lockdown=no" (granted, more work with xen.cfg or
built-in cmdline...) ? 

Anyway, what really matters here is the behavior for UEFI SecureBoot,
and this one is okay. The behavior outside of SB is secondary, and if
that's the intention, I'm okay with the current version too.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/3] Add lockdown mode

[Andrew Cooper]

On 02/06/2025 2:46 pm, Kevin Lampis wrote:
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 1f5cb67bd0..efeed5eafc 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -15,6 +15,7 @@
>  #include <xen/kexec.h>
>  #include <xen/keyhandler.h>
>  #include <xen/lib.h>
> +#include <xen/lockdown.h>
>  #include <xen/multiboot.h>
>  #include <xen/nodemask.h>
>  #include <xen/numa.h>

As the only modification to setup.c, this hunk surely isn't in the right
patch.

> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index 0951d4c2f2..33cd669110 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -587,4 +587,12 @@ config BUDDY_ALLOCATOR_SIZE
>  	  Amount of memory reserved for the buddy allocator to serve Xen heap,
>  	  working alongside the colored one.
>  
> +config LOCKDOWN_DEFAULT
> +       bool "Enable lockdown mode by default"
> +       default n

default n is redundant.  Please drop it.

> +       help
> +         Lockdown mode prevents attacks from a rogue dom0 userspace from
> +         compromising the system. This is automatically enabled when Secure
> +         Boot is enabled.

It's more than just rogue dom0 userspace.  But, are we using lockdown
mode for anything more than just cmdline filtering?

> diff --git a/xen/common/kernel.c b/xen/common/kernel.c
> index 8b63ca55f1..7280da987d 100644
> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -199,6 +200,8 @@ static int parse_params(const char *cmdline, const struct kernel_param *start,
>              printk("parameter \"%s\" unknown!\n", key);
>              final_rc = -EINVAL;
>          }
> +
> +        lockdown_clear_first_flag();

You're calling an __init function from a non-__init one.

I've submitted
https://lore.kernel.org/xen-devel/20250603125215.2716132-1-andrew.cooper3@citrix.com/T/#u
to fix it.

But honestly, given 3 function calls for trivial operations but with
complicated semantics, I'm not sure splitting out lockdown.c out of
kernel.c is a good move.

>      }
>  
>      return final_rc;
> @@ -216,6 +219,9 @@ static void __init _cmdline_parse(const char *cmdline)
>   */
>  void __init cmdline_parse(const char *cmdline)
>  {
> +    /* Call this early since it affects command-line parsing */
> +    lockdown_init(cmdline);
> +
>      if ( opt_builtin_cmdline[0] )
>      {
>          printk("Built-in command line: %s\n", opt_builtin_cmdline);

Even from just this hunk, the positioning looks suspicious.  Existing
UEFI-SB support in Xen relies on the builtin cmdline to provide
configuration, seeing as it ends up part of the signed whole.

Beyond that, I don't see what the fuss is over argument order.  The only
case where it matters is if Xen defaults to 0 and a user explicitly
wants to activate lockdown mode on the cmdline, at which point warning
them that their order of arguments was problematic is a) a problem in an
of itself, and b) unworkable when e.g. placeholder is in use.

> @@ -227,6 +233,7 @@ void __init cmdline_parse(const char *cmdline)
>          return;
>  
>      safe_strcpy(saved_cmdline, cmdline);
> +    lockdown_set_first_flag();
>      _cmdline_parse(cmdline);
>  #endif
>  }
> diff --git a/xen/common/lockdown.c b/xen/common/lockdown.c
> new file mode 100644
> index 0000000000..84eabe9c83
> --- /dev/null
> +++ b/xen/common/lockdown.c
> @@ -0,0 +1,54 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#include <xen/efi.h>
> +#include <xen/lockdown.h>
> +#include <xen/param.h>
> +
> +#define FIRST_ARG_FLAG 2
> +
> +static int __ro_after_init lockdown = IS_ENABLED(CONFIG_LOCKDOWN_DEFAULT);
> +
> +void __init lockdown_set_first_flag(void)
> +{
> +    lockdown |= FIRST_ARG_FLAG;
> +}
> +
> +void __init lockdown_clear_first_flag(void)
> +{
> +    lockdown &= ~FIRST_ARG_FLAG;
> +}
> +
> +static int __init parse_lockdown_opt(const char *s)

You need a cf_check attribute too.  This only doesn't explode in XenRT
because it runs before activating CET, but it will fail in CI.

> +{
> +    if ( strncmp("no", s, 2) == 0 )
> +        if ( efi_secure_boot )
> +            printk("lockdown can't be disabled because Xen booted in Secure Boot mode\n");
> +        else
> +            lockdown = 0;

Braces please.  This is dangerously close to being a buggy expression.

> +    else
> +    {
> +        if ( !(lockdown & FIRST_ARG_FLAG) )
> +            printk("lockdown was not the first argument, unsafe arguments may have been already processed\n");
> +
> +        lockdown = 1;
> +    }
> +
> +    return 0;
> +}
> +custom_param("lockdown", parse_lockdown_opt);
> +
> +bool is_locked_down(void)
> +{
> +    return lockdown & ~FIRST_ARG_FLAG;
> +}
> +
> +void __init lockdown_init(const char *cmdline)
> +{
> +    if ( efi_secure_boot )
> +    {
> +        printk("Enabling lockdown mode because Secure Boot is enabled\n");
> +        lockdown = 1;
> +    }

This wants setting by init_secure_boot_mode().  Nothing good can come of
there being a window where efi_secure_boot is set but lockdown is not.

Why is there a cmdline parameter?  It doesn't appear to be used.

> +
> +    printk("Lockdown mode is %s\n", is_locked_down() ? "enabled" : "disabled");
> +}

~Andrew

Re: [PATCH v2 2/3] Add lockdown mode

[Ross Lagerwall]

On Tue, Jun 3, 2025 at 5:29 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 02/06/2025 2:46 pm, Kevin Lampis wrote:
> > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> > index 1f5cb67bd0..efeed5eafc 100644
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -15,6 +15,7 @@
> >  #include <xen/kexec.h>
> >  #include <xen/keyhandler.h>
> >  #include <xen/lib.h>
> > +#include <xen/lockdown.h>
> >  #include <xen/multiboot.h>
> >  #include <xen/nodemask.h>
> >  #include <xen/numa.h>
>
> As the only modification to setup.c, this hunk surely isn't in the right
> patch.
>
> > diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> > index 0951d4c2f2..33cd669110 100644
> > --- a/xen/common/Kconfig
> > +++ b/xen/common/Kconfig
> > @@ -587,4 +587,12 @@ config BUDDY_ALLOCATOR_SIZE
> >         Amount of memory reserved for the buddy allocator to serve Xen heap,
> >         working alongside the colored one.
> >
> > +config LOCKDOWN_DEFAULT
> > +       bool "Enable lockdown mode by default"
> > +       default n
>
> default n is redundant.  Please drop it.
>
> > +       help
> > +         Lockdown mode prevents attacks from a rogue dom0 userspace from
> > +         compromising the system. This is automatically enabled when Secure
> > +         Boot is enabled.
>
> It's more than just rogue dom0 userspace.  But, are we using lockdown
> mode for anything more than just cmdline filtering?

Not as part of this series, but it is expected that lockdown mode will
eventually be tied into certain other functionality. E.g. requiring live
patches to be signed when it is enabled.

Ross

Re: [PATCH v2 3/3] Disallow most command-line options when lockdown mode is enabled

[Jan Beulich]

On 02.06.2025 15:46, Kevin Lampis wrote:
> A subset of command-line parameters that are specifically safe to use when
> lockdown mode is enabled are annotated as such.
> 
> These are commonly used parameters which have been audited to ensure they
> cannot be used to undermine the integrity of the system when booted in
> Secure Boot mode.

It's still being left entirely unclear what the criteria are by which an
option can / cannot be marked "safe". For example ...

> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -47,7 +47,7 @@ integer_param("cpuid_mask_thermal_ecx", opt_cpuid_mask_thermal_ecx);
>  
>  /* 1 = allow, 0 = don't allow guest creation, -1 = don't allow boot */
>  int8_t __read_mostly opt_allow_unsafe;
> -boolean_param("allow_unsafe", opt_allow_unsafe);
> +boolean_secure_param("allow_unsafe", opt_allow_unsafe);

... why's this being marked such, when already by its name its use is going
to render the system unsafe.

> --- a/xen/arch/x86/cpu/mcheck/mce.c
> +++ b/xen/arch/x86/cpu/mcheck/mce.c
> @@ -31,7 +31,7 @@
>  #include "vmce.h"
>  
>  bool __read_mostly opt_mce = true;
> -boolean_param("mce", opt_mce);
> +boolean_secure_param("mce", opt_mce);

Similarly I don't think it's a good idea to allow turning of MCE.

I won't go any further until clarification on the criteria was written
down.

Jan

Re: [PATCH v2 3/3] Disallow most command-line options when lockdown mode is enabled

[Kevin Lampis]

On Tue, Jun 10, 2025 at 4:56 PM Jan Beulich <jbeulich@suse.com> wrote:
>
>It's still being left entirely unclear what the criteria are by which an
>option can / cannot be marked "safe".

The purpose of lockdown mode is to protect Xen from unauthorized code execution
in Secure Boot mode. Xen especially needs protection from dom0 userland which
I understand has traditionally been considered fully trusted.

>... why's this being marked such, when already by its name its use is going
>to render the system unsafe.
>Similarly I don't think it's a good idea to allow turning off MCE.

I believe these are both denial of service issues which is out of scope for
lockdown mode / Secure Boot.

>I won't go any further until clarification on the criteria was written
>down.
I understand your feedback. Picking safe comandline options and explaining why
they are safe requires more work here.