[v3,02/19] x86/boot: Move mem_encrypt= parsing to the decompressor
Commit Message
From: Ard Biesheuvel <ardb@kernel.org>
The early SME/SEV code parses the command line very early, in order to
decide whether or not memory encryption should be enabled, which needs
to occur even before the initial page tables are created.
This is problematic for a number of reasons:
- this early code runs from the 1:1 mapping provided by the decompressor
or firmware, which uses a different translation than the one assumed by
the linker, and so the code needs to be built in a special way;
- parsing external input while the entire kernel image is still mapped
writable is a bad idea in general, and really does not belong in
security minded code;
- the current code ignores the built-in command line entirely (although
this appears to be the case for the entire decompressor)
Given that the decompressor/EFI stub is an intrinsic part of the x86
bootable kernel image, move the command line parsing there and out of
the core kernel. This removes the need to build lib/cmdline.o in a
special way, or to use RIP-relative LEA instructions in inline asm
blocks.
This involves a pair of new xloadflags in the setup header to indicate
that a) mem_encrypt= was provided, and b) whether it was set to on or
off. What this actually means in terms of default behavior when the
command line parameter is omitted is left up to the existing logic -
this permits the same flags to be reused if the need arises.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
arch/x86/boot/compressed/misc.c | 22 ++++++++++
arch/x86/include/uapi/asm/bootparam.h | 2 +
arch/x86/lib/Makefile | 13 ------
arch/x86/mm/mem_encrypt_identity.c | 45 +++-----------------
drivers/firmware/efi/libstub/x86-stub.c | 6 +++
5 files changed, 37 insertions(+), 51 deletions(-)
Comments
On Mon, Jan 29, 2024 at 07:05:05PM +0100, Ard Biesheuvel wrote:
> +/*
> + * Set the memory encryption xloadflag based on the mem_encrypt= command line
> + * parameter, if provided. If not, the consumer of the flag decides what the
> + * default behavior should be.
> + */
> +static void set_mem_encrypt_flag(struct setup_header *hdr)
parse_mem_encrypt
> +{
> + hdr->xloadflags &= ~(XLF_MEM_ENCRYPTION | XLF_MEM_ENCRYPTION_ENABLED);
> +
> + if (IS_ENABLED(CONFIG_ARCH_HAS_MEM_ENCRYPT)) {
That's unconditionally enabled on x86:
select ARCH_HAS_MEM_ENCRYPT
in x86/Kconfig.
Which sounds like you need a single XLF_MEM_ENCRYPT and simplify this
more.
> + int on = cmdline_find_option_bool("mem_encrypt=on");
> + int off = cmdline_find_option_bool("mem_encrypt=off");
> +
> + if (on || off)
> + hdr->xloadflags |= XLF_MEM_ENCRYPTION;
> + if (on > off)
> + hdr->xloadflags |= XLF_MEM_ENCRYPTION_ENABLED;
> + }
> +}
Otherwise, I like the simplification.
Thx.
On Wed, Jan 31, 2024 at 9:35 AM Borislav Petkov <bp@alien8.de> wrote:
>
> On Mon, Jan 29, 2024 at 07:05:05PM +0100, Ard Biesheuvel wrote:
> > +/*
> > + * Set the memory encryption xloadflag based on the mem_encrypt= command line
> > + * parameter, if provided. If not, the consumer of the flag decides what the
> > + * default behavior should be.
> > + */
> > +static void set_mem_encrypt_flag(struct setup_header *hdr)
>
> parse_mem_encrypt
>
OK
> > +{
> > + hdr->xloadflags &= ~(XLF_MEM_ENCRYPTION | XLF_MEM_ENCRYPTION_ENABLED);
> > +
> > + if (IS_ENABLED(CONFIG_ARCH_HAS_MEM_ENCRYPT)) {
>
> That's unconditionally enabled on x86:
>
> select ARCH_HAS_MEM_ENCRYPT
>
> in x86/Kconfig.
>
> Which sounds like you need a single XLF_MEM_ENCRYPT and simplify this
> more.
>
OK, but that only means I can drop the if().
The reason we need two flags is because there is no default value to
use when the command line param is absent.
There is CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT but that one is AMD
specific. There is CONFIG_X86_MEM_ENCRYPT which is shared between
SME/SEV and TDX, which has no default setting.
> > + int on = cmdline_find_option_bool("mem_encrypt=on");
> > + int off = cmdline_find_option_bool("mem_encrypt=off");
> > +
> > + if (on || off)
> > + hdr->xloadflags |= XLF_MEM_ENCRYPTION;
> > + if (on > off)
> > + hdr->xloadflags |= XLF_MEM_ENCRYPTION_ENABLED;
> > + }
> > +}
>
> Otherwise, I like the simplification.
>
Cheers.
On Wed, Jan 31, 2024 at 10:12:13AM +0100, Ard Biesheuvel wrote:
> The reason we need two flags is because there is no default value to
> use when the command line param is absent.
I think absent means memory encryption disabled like with every other
option which is not present...
> There is CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT but that one is AMD
.. yes, and I'm thinking that it is time we kill this. I don't think
anything uses it. It was meant well at the time.
Let's wait for Tom to wake up first, though, as he might have some
objections...
Thx.
On Wed, 31 Jan 2024 at 10:30, Borislav Petkov <bp@alien8.de> wrote:
>
> On Wed, Jan 31, 2024 at 10:12:13AM +0100, Ard Biesheuvel wrote:
> > The reason we need two flags is because there is no default value to
> > use when the command line param is absent.
>
> I think absent means memory encryption disabled like with every other
> option which is not present...
>
> > There is CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT but that one is AMD
>
> ... yes, and I'm thinking that it is time we kill this. I don't think
> anything uses it. It was meant well at the time.
>
> Let's wait for Tom to wake up first, though, as he might have some
> objections...
>
OK, yeah, that would help.
AIUI this is for SME only anyway - SEV ignores this, and I suppose TDX
will do the same.
On 1/31/24 03:29, Borislav Petkov wrote:
> On Wed, Jan 31, 2024 at 10:12:13AM +0100, Ard Biesheuvel wrote:
>> The reason we need two flags is because there is no default value to
>> use when the command line param is absent.
>
> I think absent means memory encryption disabled like with every other
> option which is not present...
>
>> There is CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT but that one is AMD
>
> ... yes, and I'm thinking that it is time we kill this. I don't think
> anything uses it. It was meant well at the time.
>
> Let's wait for Tom to wake up first, though, as he might have some
> objections...
I don't know if anyone is using the AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT
config option, but I don't have an issue removing it.
Thanks,
Tom
>
> Thx.
>
On Thu, 1 Feb 2024 at 15:17, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> On 1/31/24 03:29, Borislav Petkov wrote:
> > On Wed, Jan 31, 2024 at 10:12:13AM +0100, Ard Biesheuvel wrote:
> >> The reason we need two flags is because there is no default value to
> >> use when the command line param is absent.
> >
> > I think absent means memory encryption disabled like with every other
> > option which is not present...
> >
> >> There is CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT but that one is AMD
> >
> > ... yes, and I'm thinking that it is time we kill this. I don't think
> > anything uses it. It was meant well at the time.
> >
> > Let's wait for Tom to wake up first, though, as he might have some
> > objections...
>
> I don't know if anyone is using the AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT
> config option, but I don't have an issue removing it.
>
OK, I'll remove it in the next rev.
On Fri, 2 Feb 2024 at 17:35, Borislav Petkov <bp@alien8.de> wrote:
>
> On Thu, Feb 01, 2024 at 05:15:51PM +0100, Ard Biesheuvel wrote:
> > OK, I'll remove it in the next rev.
>
> Considering how it simplifies sme_enable() even more, I'd like to
> expedite this one.
>
> Thx.
>
> ---
> From: "Borislav Petkov (AMD)" <bp@alien8.de>
> Date: Fri, 2 Feb 2024 17:29:32 +0100
> Subject: [PATCH] x86/Kconfig: Remove CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT
>
> It was meant well at the time but nothing's using it so get rid of it.
>
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
> ---
> Documentation/admin-guide/kernel-parameters.txt | 4 +---
> Documentation/arch/x86/amd-memory-encryption.rst | 16 ++++++++--------
> arch/x86/Kconfig | 13 -------------
> arch/x86/mm/mem_encrypt_identity.c | 11 +----------
> 4 files changed, 10 insertions(+), 34 deletions(-)
>
Works for me.
Acked-by: Ard Biesheuvel <ardb@kernel.org>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 31b3a25680d0..2cb70a384af8 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -3320,9 +3320,7 @@
>
> mem_encrypt= [X86-64] AMD Secure Memory Encryption (SME) control
> Valid arguments: on, off
> - Default (depends on kernel configuration option):
> - on (CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT=y)
> - off (CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT=n)
> + Default: off
> mem_encrypt=on: Activate SME
> mem_encrypt=off: Do not activate SME
>
> diff --git a/Documentation/arch/x86/amd-memory-encryption.rst b/Documentation/arch/x86/amd-memory-encryption.rst
> index 07caa8fff852..414bc7402ae7 100644
> --- a/Documentation/arch/x86/amd-memory-encryption.rst
> +++ b/Documentation/arch/x86/amd-memory-encryption.rst
> @@ -87,14 +87,14 @@ The state of SME in the Linux kernel can be documented as follows:
> kernel is non-zero).
>
> SME can also be enabled and activated in the BIOS. If SME is enabled and
> -activated in the BIOS, then all memory accesses will be encrypted and it will
> -not be necessary to activate the Linux memory encryption support. If the BIOS
> -merely enables SME (sets bit 23 of the MSR_AMD64_SYSCFG), then Linux can activate
> -memory encryption by default (CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT=y) or
> -by supplying mem_encrypt=on on the kernel command line. However, if BIOS does
> -not enable SME, then Linux will not be able to activate memory encryption, even
> -if configured to do so by default or the mem_encrypt=on command line parameter
> -is specified.
> +activated in the BIOS, then all memory accesses will be encrypted and it
> +will not be necessary to activate the Linux memory encryption support.
> +
> +If the BIOS merely enables SME (sets bit 23 of the MSR_AMD64_SYSCFG),
> +then memory encryption can be enabled by supplying mem_encrypt=on on the
> +kernel command line. However, if BIOS does not enable SME, then Linux
> +will not be able to activate memory encryption, even if configured to do
> +so by default or the mem_encrypt=on command line parameter is specified.
>
> Secure Nested Paging (SNP)
> ==========================
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 5edec175b9bf..58d3593bc4f2 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1539,19 +1539,6 @@ config AMD_MEM_ENCRYPT
> This requires an AMD processor that supports Secure Memory
> Encryption (SME).
>
> -config AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT
> - bool "Activate AMD Secure Memory Encryption (SME) by default"
> - depends on AMD_MEM_ENCRYPT
> - help
> - Say yes to have system memory encrypted by default if running on
> - an AMD processor that supports Secure Memory Encryption (SME).
> -
> - If set to Y, then the encryption of system memory can be
> - deactivated with the mem_encrypt=off command line option.
> -
> - If set to N, then the encryption of system memory can be
> - activated with the mem_encrypt=on command line option.
> -
> # Common NUMA Features
> config NUMA
> bool "NUMA Memory Allocation and Scheduler Support"
> diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
> index 7f72472a34d6..efe9f217fcf9 100644
> --- a/arch/x86/mm/mem_encrypt_identity.c
> +++ b/arch/x86/mm/mem_encrypt_identity.c
> @@ -97,7 +97,6 @@ static char sme_workarea[2 * PMD_SIZE] __section(".init.scratch");
>
> static char sme_cmdline_arg[] __initdata = "mem_encrypt";
> static char sme_cmdline_on[] __initdata = "on";
> -static char sme_cmdline_off[] __initdata = "off";
>
> static void __init sme_clear_pgd(struct sme_populate_pgd_data *ppd)
> {
> @@ -504,7 +503,7 @@ void __init sme_encrypt_kernel(struct boot_params *bp)
>
> void __init sme_enable(struct boot_params *bp)
> {
> - const char *cmdline_ptr, *cmdline_arg, *cmdline_on, *cmdline_off;
> + const char *cmdline_ptr, *cmdline_arg, *cmdline_on;
> unsigned int eax, ebx, ecx, edx;
> unsigned long feature_mask;
> unsigned long me_mask;
> @@ -587,12 +586,6 @@ void __init sme_enable(struct boot_params *bp)
> asm ("lea sme_cmdline_on(%%rip), %0"
> : "=r" (cmdline_on)
> : "p" (sme_cmdline_on));
> - asm ("lea sme_cmdline_off(%%rip), %0"
> - : "=r" (cmdline_off)
> - : "p" (sme_cmdline_off));
> -
> - if (IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT))
> - sme_me_mask = me_mask;
>
> cmdline_ptr = (const char *)((u64)bp->hdr.cmd_line_ptr |
> ((u64)bp->ext_cmd_line_ptr << 32));
> @@ -602,8 +595,6 @@ void __init sme_enable(struct boot_params *bp)
>
> if (!strncmp(buffer, cmdline_on, sizeof(buffer)))
> sme_me_mask = me_mask;
> - else if (!strncmp(buffer, cmdline_off, sizeof(buffer)))
> - sme_me_mask = 0;
>
> out:
> if (sme_me_mask) {
> --
> 2.43.0
>
>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
@@ -357,6 +357,26 @@ unsigned long decompress_kernel(unsigned char *outbuf, unsigned long virt_addr,
return entry;
}
+/*
+ * Set the memory encryption xloadflag based on the mem_encrypt= command line
+ * parameter, if provided. If not, the consumer of the flag decides what the
+ * default behavior should be.
+ */
+static void set_mem_encrypt_flag(struct setup_header *hdr)
+{
+ hdr->xloadflags &= ~(XLF_MEM_ENCRYPTION | XLF_MEM_ENCRYPTION_ENABLED);
+
+ if (IS_ENABLED(CONFIG_ARCH_HAS_MEM_ENCRYPT)) {
+ int on = cmdline_find_option_bool("mem_encrypt=on");
+ int off = cmdline_find_option_bool("mem_encrypt=off");
+
+ if (on || off)
+ hdr->xloadflags |= XLF_MEM_ENCRYPTION;
+ if (on > off)
+ hdr->xloadflags |= XLF_MEM_ENCRYPTION_ENABLED;
+ }
+}
+
/*
* The compressed kernel image (ZO), has been moved so that its position
* is against the end of the buffer used to hold the uncompressed kernel
@@ -387,6 +407,8 @@ asmlinkage __visible void *extract_kernel(void *rmode, unsigned char *output)
/* Clear flags intended for solely in-kernel use. */
boot_params_ptr->hdr.loadflags &= ~KASLR_FLAG;
+ set_mem_encrypt_flag(&boot_params_ptr->hdr);
+
sanitize_boot_params(boot_params_ptr);
if (boot_params_ptr->screen_info.orig_video_mode == 7) {
@@ -38,6 +38,8 @@
#define XLF_EFI_KEXEC (1<<4)
#define XLF_5LEVEL (1<<5)
#define XLF_5LEVEL_ENABLED (1<<6)
+#define XLF_MEM_ENCRYPTION (1<<7)
+#define XLF_MEM_ENCRYPTION_ENABLED (1<<8)
#ifndef __ASSEMBLY__
@@ -14,19 +14,6 @@ ifdef CONFIG_KCSAN
CFLAGS_REMOVE_delay.o = $(CC_FLAGS_FTRACE)
endif
-# Early boot use of cmdline; don't instrument it
-ifdef CONFIG_AMD_MEM_ENCRYPT
-KCOV_INSTRUMENT_cmdline.o := n
-KASAN_SANITIZE_cmdline.o := n
-KCSAN_SANITIZE_cmdline.o := n
-
-ifdef CONFIG_FUNCTION_TRACER
-CFLAGS_REMOVE_cmdline.o = -pg
-endif
-
-CFLAGS_cmdline.o := -fno-stack-protector -fno-jump-tables
-endif
-
inat_tables_script = $(srctree)/arch/x86/tools/gen-insn-attr-x86.awk
inat_tables_maps = $(srctree)/arch/x86/lib/x86-opcode-map.txt
quiet_cmd_inat_tables = GEN $@
@@ -43,7 +43,6 @@
#include <asm/setup.h>
#include <asm/sections.h>
-#include <asm/cmdline.h>
#include <asm/coco.h>
#include <asm/sev.h>
@@ -95,10 +94,6 @@ struct sme_populate_pgd_data {
*/
static char sme_workarea[2 * PMD_SIZE] __section(".init.scratch");
-static char sme_cmdline_arg[] __initdata = "mem_encrypt";
-static char sme_cmdline_on[] __initdata = "on";
-static char sme_cmdline_off[] __initdata = "off";
-
static void __init sme_clear_pgd(struct sme_populate_pgd_data *ppd)
{
unsigned long pgd_start, pgd_end, pgd_size;
@@ -504,11 +499,9 @@ void __init sme_encrypt_kernel(struct boot_params *bp)
void __init sme_enable(struct boot_params *bp)
{
- const char *cmdline_ptr, *cmdline_arg, *cmdline_on, *cmdline_off;
unsigned int eax, ebx, ecx, edx;
unsigned long feature_mask;
unsigned long me_mask;
- char buffer[16];
bool snp;
u64 msr;
@@ -570,42 +563,18 @@ void __init sme_enable(struct boot_params *bp)
msr = __rdmsr(MSR_AMD64_SYSCFG);
if (!(msr & MSR_AMD64_SYSCFG_MEM_ENCRYPT))
return;
+
+ if (bp->hdr.xloadflags & XLF_MEM_ENCRYPTION) {
+ if (bp->hdr.xloadflags & XLF_MEM_ENCRYPTION_ENABLED)
+ sme_me_mask = me_mask;
+ } else if (IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT)) {
+ sme_me_mask = me_mask;
+ }
} else {
/* SEV state cannot be controlled by a command line option */
sme_me_mask = me_mask;
- goto out;
}
- /*
- * Fixups have not been applied to phys_base yet and we're running
- * identity mapped, so we must obtain the address to the SME command
- * line argument data using rip-relative addressing.
- */
- asm ("lea sme_cmdline_arg(%%rip), %0"
- : "=r" (cmdline_arg)
- : "p" (sme_cmdline_arg));
- asm ("lea sme_cmdline_on(%%rip), %0"
- : "=r" (cmdline_on)
- : "p" (sme_cmdline_on));
- asm ("lea sme_cmdline_off(%%rip), %0"
- : "=r" (cmdline_off)
- : "p" (sme_cmdline_off));
-
- if (IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT))
- sme_me_mask = me_mask;
-
- cmdline_ptr = (const char *)((u64)bp->hdr.cmd_line_ptr |
- ((u64)bp->ext_cmd_line_ptr << 32));
-
- if (cmdline_find_option(cmdline_ptr, cmdline_arg, buffer, sizeof(buffer)) < 0)
- goto out;
-
- if (!strncmp(buffer, cmdline_on, sizeof(buffer)))
- sme_me_mask = me_mask;
- else if (!strncmp(buffer, cmdline_off, sizeof(buffer)))
- sme_me_mask = 0;
-
-out:
if (sme_me_mask) {
physical_mask &= ~sme_me_mask;
cc_vendor = CC_VENDOR_AMD;
@@ -879,6 +879,12 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
}
}
+ if (IS_ENABLED(CONFIG_ARCH_HAS_MEM_ENCRYPT) && efi_mem_encrypt) {
+ hdr->xloadflags |= XLF_MEM_ENCRYPTION;
+ if (efi_mem_encrypt > 0)
+ hdr->xloadflags |= XLF_MEM_ENCRYPTION_ENABLED;
+ }
+
status = efi_decompress_kernel(&kernel_entry);
if (status != EFI_SUCCESS) {
efi_err("Failed to decompress kernel\n");