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: <77049b07-9e13-4c6a-a7bb-70e9e74a662f@amd.com>
Date: Fri, 12 Sep 2025 13:34:09 -0500
From: Tom Lendacky <thomas.lendacky@....com>
To: Borislav Petkov <bp@...en8.de>, Ard Biesheuvel <ardb@...nel.org>
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 9/12/25 13:26, Borislav Petkov wrote:
> 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;

Shouldn't this routine then delete the line that assigns boot_ghcb and
instead return &boot_ghcb_page?

Thanks,
Tom

>  }
>  
>  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,