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.coop...@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
