linux-kernel - Re: [tip:x86/boot 10/10] arch/x86/boot/compressed/sev-handle-vc.c:104 do_boot_stage2_vc() error: we previously assumed 'boot

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]

Message-ID: <20250912182642.GCaMRl4nu7R9C8uP6R@fat_crate.local>
Date: Fri, 12 Sep 2025 20:26:42 +0200
From: Borislav Petkov <bp@...en8.de>
To: Ard Biesheuvel <ardb@...nel.org>,
	Tom Lendacky <thomas.lendacky@....com>
Cc: Dan Carpenter <dan.carpenter@...aro.org>, oe-kbuild@...ts.linux.dev,
	lkp@...el.com, oe-kbuild-all@...ts.linux.dev,
	linux-kernel@...r.kernel.org, x86@...nel.org,
	Ingo Molnar <mingo@...nel.org>
Subject: Re: [tip:x86/boot 10/10]
 arch/x86/boot/compressed/sev-handle-vc.c:104 do_boot_stage2_vc() error: we
 previously assumed 'boot_ghcb' could be null (see line 101)

On Sat, May 10, 2025 at 09:57:23AM +0200, Ard Biesheuvel wrote:
> The logic is a bit clunky here: for clarity, it could be rewritten as
> 
> if (!boot_ghcb) {
>   early_setup_ghcb();
>   if (!boot_ghcb)
>     sev_es_terminate(...);
> }

I like that. Untested patch below:

---

From: "Borislav Petkov (AMD)" <bp@...en8.de>
Date: Fri, 12 Sep 2025 20:21:39 +0200
Subject: [PATCH] x86/sev: Clean up boot_ghcb assignment

Make it more obvious that early_setup_ghcb() assigns the boot_ghcb
pointer by simply returning it. This way the code becomes a bit
more readable and comprehensible.

No functional changes intended.

Reported-by: Dan Carpenter <dan.carpenter@...aro.org>
Suggested-by: Ard Biesheuvel <ardb@...nel.org>
Signed-off-by: Borislav Petkov (AMD) <bp@...en8.de>
Link: https://lore.kernel.org/r/202505100719.9pE7wDfB-lkp@intel.com
---
 arch/x86/boot/compressed/misc.h          |  2 +-
 arch/x86/boot/compressed/sev-handle-vc.c |  5 ++++-
 arch/x86/boot/compressed/sev.c           | 11 +++++++----
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index db1048621ea2..648f751538de 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -150,7 +150,7 @@ void sev_prep_identity_maps(unsigned long top_level_pgt);
 enum es_result vc_decode_insn(struct es_em_ctxt *ctxt);
 bool insn_has_rep_prefix(struct insn *insn);
 void sev_insn_decode_init(void);
-bool early_setup_ghcb(void);
+struct ghcb *early_setup_ghcb(void);
 #else
 static inline void sev_enable(struct boot_params *bp)
 {
diff --git a/arch/x86/boot/compressed/sev-handle-vc.c b/arch/x86/boot/compressed/sev-handle-vc.c
index 7530ad8b768b..66b29fbb9f57 100644
--- a/arch/x86/boot/compressed/sev-handle-vc.c
+++ b/arch/x86/boot/compressed/sev-handle-vc.c
@@ -101,7 +101,10 @@ void do_boot_stage2_vc(struct pt_regs *regs, unsigned long exit_code)
 	struct es_em_ctxt ctxt;
 	enum es_result result;
 
-	if (!boot_ghcb && !early_setup_ghcb())
+	if (!boot_ghcb)
+		boot_ghcb = early_setup_ghcb();
+
+	if (!boot_ghcb)
 		sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SEV_ES_GEN_REQ);
 
 	vc_ghcb_invalidate(boot_ghcb);
diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
index 6e5c32a53d03..f9dcd1b795d7 100644
--- a/arch/x86/boot/compressed/sev.c
+++ b/arch/x86/boot/compressed/sev.c
@@ -75,10 +75,10 @@ void snp_set_page_shared(unsigned long paddr)
 	__page_state_change(paddr, paddr, &d);
 }
 
-bool early_setup_ghcb(void)
+struct ghcb *early_setup_ghcb(void)
 {
 	if (set_page_decrypted((unsigned long)&boot_ghcb_page))
-		return false;
+		return NULL;
 
 	/* Page is now mapped decrypted, clear it */
 	memset(&boot_ghcb_page, 0, sizeof(boot_ghcb_page));
@@ -92,7 +92,7 @@ bool early_setup_ghcb(void)
 	if (sev_snp_enabled())
 		snp_register_ghcb_early(__pa(&boot_ghcb_page));
 
-	return true;
+	return boot_ghcb;
 }
 
 void snp_accept_memory(phys_addr_t start, phys_addr_t end)
@@ -216,6 +216,9 @@ void snp_check_features(void)
 {
 	u64 unsupported;
 
+	if (!boot_ghcb)
+		boot_ghcb = early_setup_ghcb();
+
 	/*
 	 * Terminate the boot if hypervisor has enabled any feature lacking
 	 * guest side implementation. Pass on the unsupported features mask through
@@ -224,7 +227,7 @@ void snp_check_features(void)
 	 */
 	unsupported = snp_get_unsupported_features(sev_status);
 	if (unsupported) {
-		if (ghcb_version < 2 || (!boot_ghcb && !early_setup_ghcb()))
+		if (ghcb_version < 2 || !boot_ghcb)
 			sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
 
 		sev_es_ghcb_terminate(boot_ghcb, SEV_TERM_SET_GEN,
-- 
2.51.0



-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette