In some cases bootloaders will leave boot_params->cc_blob_address
uninitialized rather than zero'ing it out. This field is only meant to
be set by the boot/compressed kernel to pass information to the
uncompressed kernel when SEV-SNP support is enabled, so there are no
cases where the bootloader-provided values should be treated as
anything other than garbage. Otherwise, the uncompressed kernel may
attempt to access this bogus address, leading to a crash during early
boot.
Normally sanitize_boot_params() would be used to clear out such fields,
but that happens too late: sev_enable() may have already initialized it
to a valid value that should not be zero'd out. Instead, have
sev_enable() zero it out unconditionally beforehand.
Also ensure this happens for !CONFIG_AMD_MEM_ENCRYPT as well by also
including this handling in the sev_enable() stub function.
Fixes: b190a043c49a ("x86/sev: Add SEV-SNP feature detection/setup")
Cc: [email protected]
Reported-by: Jeremi Piotrowski <[email protected]>
Reported-by: [email protected]
Link: https://bugzilla.kernel.org/show_bug.cgi?id=216387
Signed-off-by: Michael Roth <[email protected]>
---
arch/x86/boot/compressed/misc.h | 11 ++++++++++-
arch/x86/boot/compressed/sev.c | 8 ++++++++
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index 4910bf230d7b..aa7889751abc 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -132,7 +132,16 @@ void snp_set_page_private(unsigned long paddr);
void snp_set_page_shared(unsigned long paddr);
void sev_prep_identity_maps(unsigned long top_level_pgt);
#else
-static inline void sev_enable(struct boot_params *bp) { }
+static inline void sev_enable(struct boot_params *bp)
+{
+ /*
+ * bp->cc_blob_address should only be set by boot/compressed kernel.
+ * Initialize it to 0 to ensure that uninitialized values from
+ * buggy bootloaders aren't propagated.
+ */
+ if (bp)
+ bp->cc_blob_address = 0;
+}
static inline void sev_es_shutdown_ghcb(void) { }
static inline bool sev_es_check_ghcb_fault(unsigned long address)
{
diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
index 52f989f6acc2..c93930d5ccbd 100644
--- a/arch/x86/boot/compressed/sev.c
+++ b/arch/x86/boot/compressed/sev.c
@@ -276,6 +276,14 @@ void sev_enable(struct boot_params *bp)
struct msr m;
bool snp;
+ /*
+ * bp->cc_blob_address should only be set by boot/compressed kernel.
+ * Initialize it to 0 to ensure that uninitialized values from
+ * buggy bootloaders aren't propagated.
+ */
+ if (bp)
+ bp->cc_blob_address = 0;
+
/*
* Setup/preliminary detection of SNP. This will be sanity-checked
* against CPUID/MSR values later.
--
2.25.1
On Tue, Aug 23, 2022 at 11:07:34AM -0500, Michael Roth wrote:
> In some cases bootloaders will leave boot_params->cc_blob_address
> uninitialized rather than zero'ing it out. This field is only meant to
> be set by the boot/compressed kernel to pass information to the
> uncompressed kernel when SEV-SNP support is enabled, so there are no
> cases where the bootloader-provided values should be treated as
> anything other than garbage. Otherwise, the uncompressed kernel may
> attempt to access this bogus address, leading to a crash during early
> boot.
>
> Normally sanitize_boot_params() would be used to clear out such fields,
> but that happens too late: sev_enable() may have already initialized it
> to a valid value that should not be zero'd out. Instead, have
> sev_enable() zero it out unconditionally beforehand.
>
> Also ensure this happens for !CONFIG_AMD_MEM_ENCRYPT as well by also
> including this handling in the sev_enable() stub function.
>
> Fixes: b190a043c49a ("x86/sev: Add SEV-SNP feature detection/setup")
> Cc: [email protected]
> Reported-by: Jeremi Piotrowski <[email protected]>
> Reported-by: [email protected]
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216387
> Signed-off-by: Michael Roth <[email protected]>
> ---
> arch/x86/boot/compressed/misc.h | 11 ++++++++++-
> arch/x86/boot/compressed/sev.c | 8 ++++++++
> 2 files changed, 18 insertions(+), 1 deletion(-)
I extended the stub comment too so that it is clear that yes, the stub
is supposed to do it too and not someone later wonders why there's code
in the stub function.
---
From 4065a25f2b287e42642fe31509304cfd0a1ee125 Mon Sep 17 00:00:00 2001
From: Michael Roth <[email protected]>
Date: Tue, 23 Aug 2022 11:07:34 -0500
Subject: [PATCH] x86/boot: Don't propagate uninitialized
boot_params->cc_blob_address
In some cases, bootloaders will leave boot_params->cc_blob_address
uninitialized rather than zeroing it out. This field is only meant to be
set by the boot/compressed kernel in order to pass information to the
uncompressed kernel when SEV-SNP support is enabled.
Therefore, there are no cases where the bootloader-provided values
should be treated as anything other than garbage. Otherwise, the
uncompressed kernel may attempt to access this bogus address, leading to
a crash during early boot.
Normally, sanitize_boot_params() would be used to clear out such fields
but that happens too late: sev_enable() may have already initialized
it to a valid value that should not be zeroed out. Instead, have
sev_enable() zero it out unconditionally beforehand.
Also ensure this happens for !CONFIG_AMD_MEM_ENCRYPT as well by also
including this handling in the sev_enable() stub function.
[ bp: Massage commit message and comments. ]
Fixes: b190a043c49a ("x86/sev: Add SEV-SNP feature detection/setup")
Reported-by: Jeremi Piotrowski <[email protected]>
Reported-by: [email protected]
Signed-off-by: Michael Roth <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Cc: [email protected]
Link: https://bugzilla.kernel.org/show_bug.cgi?id=216387
Link: https://lore.kernel.org/r/[email protected]
---
arch/x86/boot/compressed/misc.h | 12 +++++++++++-
arch/x86/boot/compressed/sev.c | 8 ++++++++
2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index 4910bf230d7b..62208ec04ca4 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -132,7 +132,17 @@ void snp_set_page_private(unsigned long paddr);
void snp_set_page_shared(unsigned long paddr);
void sev_prep_identity_maps(unsigned long top_level_pgt);
#else
-static inline void sev_enable(struct boot_params *bp) { }
+static inline void sev_enable(struct boot_params *bp)
+{
+ /*
+ * bp->cc_blob_address should only be set by boot/compressed kernel.
+ * Initialize it to 0 unconditionally (thus here in this stub too) to
+ * ensure that uninitialized values from buggy bootloaders aren't
+ * propagated.
+ */
+ if (bp)
+ bp->cc_blob_address = 0;
+}
static inline void sev_es_shutdown_ghcb(void) { }
static inline bool sev_es_check_ghcb_fault(unsigned long address)
{
diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
index 52f989f6acc2..c93930d5ccbd 100644
--- a/arch/x86/boot/compressed/sev.c
+++ b/arch/x86/boot/compressed/sev.c
@@ -276,6 +276,14 @@ void sev_enable(struct boot_params *bp)
struct msr m;
bool snp;
+ /*
+ * bp->cc_blob_address should only be set by boot/compressed kernel.
+ * Initialize it to 0 to ensure that uninitialized values from
+ * buggy bootloaders aren't propagated.
+ */
+ if (bp)
+ bp->cc_blob_address = 0;
+
/*
* Setup/preliminary detection of SNP. This will be sanity-checked
* against CPUID/MSR values later.
--
2.35.1
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
The following commit has been merged into the x86/urgent branch of tip:
Commit-ID: 4b1c742407571eff58b6de9881889f7ca7c4b4dc
Gitweb: https://git.kernel.org/tip/4b1c742407571eff58b6de9881889f7ca7c4b4dc
Author: Michael Roth <[email protected]>
AuthorDate: Tue, 23 Aug 2022 11:07:34 -05:00
Committer: Borislav Petkov <[email protected]>
CommitterDate: Wed, 24 Aug 2022 09:03:04 +02:00
x86/boot: Don't propagate uninitialized boot_params->cc_blob_address
In some cases, bootloaders will leave boot_params->cc_blob_address
uninitialized rather than zeroing it out. This field is only meant to be
set by the boot/compressed kernel in order to pass information to the
uncompressed kernel when SEV-SNP support is enabled.
Therefore, there are no cases where the bootloader-provided values
should be treated as anything other than garbage. Otherwise, the
uncompressed kernel may attempt to access this bogus address, leading to
a crash during early boot.
Normally, sanitize_boot_params() would be used to clear out such fields
but that happens too late: sev_enable() may have already initialized
it to a valid value that should not be zeroed out. Instead, have
sev_enable() zero it out unconditionally beforehand.
Also ensure this happens for !CONFIG_AMD_MEM_ENCRYPT as well by also
including this handling in the sev_enable() stub function.
[ bp: Massage commit message and comments. ]
Fixes: b190a043c49a ("x86/sev: Add SEV-SNP feature detection/setup")
Reported-by: Jeremi Piotrowski <[email protected]>
Reported-by: [email protected]
Signed-off-by: Michael Roth <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Cc: [email protected]
Link: https://bugzilla.kernel.org/show_bug.cgi?id=216387
Link: https://lore.kernel.org/r/[email protected]
---
arch/x86/boot/compressed/misc.h | 12 +++++++++++-
arch/x86/boot/compressed/sev.c | 8 ++++++++
2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index 4910bf2..62208ec 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -132,7 +132,17 @@ void snp_set_page_private(unsigned long paddr);
void snp_set_page_shared(unsigned long paddr);
void sev_prep_identity_maps(unsigned long top_level_pgt);
#else
-static inline void sev_enable(struct boot_params *bp) { }
+static inline void sev_enable(struct boot_params *bp)
+{
+ /*
+ * bp->cc_blob_address should only be set by boot/compressed kernel.
+ * Initialize it to 0 unconditionally (thus here in this stub too) to
+ * ensure that uninitialized values from buggy bootloaders aren't
+ * propagated.
+ */
+ if (bp)
+ bp->cc_blob_address = 0;
+}
static inline void sev_es_shutdown_ghcb(void) { }
static inline bool sev_es_check_ghcb_fault(unsigned long address)
{
diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
index 52f989f..c93930d 100644
--- a/arch/x86/boot/compressed/sev.c
+++ b/arch/x86/boot/compressed/sev.c
@@ -277,6 +277,14 @@ void sev_enable(struct boot_params *bp)
bool snp;
/*
+ * bp->cc_blob_address should only be set by boot/compressed kernel.
+ * Initialize it to 0 to ensure that uninitialized values from
+ * buggy bootloaders aren't propagated.
+ */
+ if (bp)
+ bp->cc_blob_address = 0;
+
+ /*
* Setup/preliminary detection of SNP. This will be sanity-checked
* against CPUID/MSR values later.
*/