linux-kernel - Re: [PATCH] x86/boot: Don't propagate uninitialized boot_params->cc_blob

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 23 Aug 2022 19:11:17 +0200
From:   Borislav Petkov <bp@...en8.de>
To:     Michael Roth <michael.roth@....com>
Cc:     linux-kernel@...r.kernel.org, stable@...r.kernel.org,
        x86@...nel.org, watnuss@....de,
        Jeremi Piotrowski <jpiotrowski@...ux.microsoft.com>,
        Tom Lendacky <thomas.lendacky@....com>
Subject: Re: [PATCH] x86/boot: Don't propagate uninitialized
 boot_params->cc_blob_address

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: stable@...r.kernel.org
> Reported-by: Jeremi Piotrowski <jpiotrowski@...ux.microsoft.com>
> Reported-by: watnuss@....de
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216387
> Signed-off-by: Michael Roth <michael.roth@....com>
> ---
>  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 <michael.roth@....com>
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 <jpiotrowski@...ux.microsoft.com>
Reported-by: watnuss@....de
Signed-off-by: Michael Roth <michael.roth@....com>
Signed-off-by: Borislav Petkov <bp@...e.de>
Cc: stable@...r.kernel.org
Link: https://bugzilla.kernel.org/show_bug.cgi?id=216387
Link: https://lore.kernel.org/r/20220823160734.89036-1-michael.roth@amd.com
---
 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