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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxxxx/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