[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)

[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)

[kernel test robot]

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
CC: linux-kernel@vger.kernel.org
CC: x86@kernel.org
TO: Ard Biesheuvel <ardb@kernel.org>
CC: Ingo Molnar <mingo@kernel.org>

Hi Ard,

FYI, the error/warning was bisected to this commit, please ignore it if it's irrelevant.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/boot
head:   ed4d95d033e359f9445e85bf5a768a5859a5830b
commit: ed4d95d033e359f9445e85bf5a768a5859a5830b [10/10] x86/sev: Disentangle #VC handling code from startup code
:::::: branch date: 5 days ago
:::::: commit date: 5 days ago
config: x86_64-randconfig-161-20250510 (https://download.01.org/0day-ci/archive/20250510/202505100719.9pE7wDfB-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202505100719.9pE7wDfB-lkp@intel.com/

smatch warnings:
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)

vim +/boot_ghcb +104 arch/x86/boot/compressed/sev-handle-vc.c

ed4d95d033e359 Ard Biesheuvel 2025-05-04   95  
ed4d95d033e359 Ard Biesheuvel 2025-05-04   96  void do_boot_stage2_vc(struct pt_regs *regs, unsigned long exit_code)
ed4d95d033e359 Ard Biesheuvel 2025-05-04   97  {
ed4d95d033e359 Ard Biesheuvel 2025-05-04   98  	struct es_em_ctxt ctxt;
ed4d95d033e359 Ard Biesheuvel 2025-05-04   99  	enum es_result result;
ed4d95d033e359 Ard Biesheuvel 2025-05-04  100  
ed4d95d033e359 Ard Biesheuvel 2025-05-04 @101  	if (!boot_ghcb && !early_setup_ghcb())
ed4d95d033e359 Ard Biesheuvel 2025-05-04  102  		sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SEV_ES_GEN_REQ);
ed4d95d033e359 Ard Biesheuvel 2025-05-04  103  
ed4d95d033e359 Ard Biesheuvel 2025-05-04 @104  	vc_ghcb_invalidate(boot_ghcb);

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[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)

[Dan Carpenter]

Hi Ard,

FYI, the error/warning was bisected to this commit, please ignore it if it's irrelevant.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/boot
head:   ed4d95d033e359f9445e85bf5a768a5859a5830b
commit: ed4d95d033e359f9445e85bf5a768a5859a5830b [10/10] x86/sev: Disentangle #VC handling code from startup code
config: x86_64-randconfig-161-20250510 (https://download.01.org/0day-ci/archive/20250510/202505100719.9pE7wDfB-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202505100719.9pE7wDfB-lkp@intel.com/

smatch warnings:
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)

vim +/boot_ghcb +104 arch/x86/boot/compressed/sev-handle-vc.c

ed4d95d033e359 Ard Biesheuvel 2025-05-04   96  void do_boot_stage2_vc(struct pt_regs *regs, unsigned long exit_code)
ed4d95d033e359 Ard Biesheuvel 2025-05-04   97  {
ed4d95d033e359 Ard Biesheuvel 2025-05-04   98  	struct es_em_ctxt ctxt;
ed4d95d033e359 Ard Biesheuvel 2025-05-04   99  	enum es_result result;
ed4d95d033e359 Ard Biesheuvel 2025-05-04  100  
ed4d95d033e359 Ard Biesheuvel 2025-05-04 @101  	if (!boot_ghcb && !early_setup_ghcb())
                                                    ^^^^^^^^^^
Check for NULL.  Should the && be ||?

ed4d95d033e359 Ard Biesheuvel 2025-05-04  102  		sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SEV_ES_GEN_REQ);
ed4d95d033e359 Ard Biesheuvel 2025-05-04  103  
ed4d95d033e359 Ard Biesheuvel 2025-05-04 @104  	vc_ghcb_invalidate(boot_ghcb);
                                                                   ^^^^^^^^^
Unchecked dereference.

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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)

[Ard Biesheuvel]

Hi Dan,

On Sat, 10 May 2025 at 09:43, Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> Hi Ard,
>
> FYI, the error/warning was bisected to this commit, please ignore it if it's irrelevant.
>
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/boot
> head:   ed4d95d033e359f9445e85bf5a768a5859a5830b
> commit: ed4d95d033e359f9445e85bf5a768a5859a5830b [10/10] x86/sev: Disentangle #VC handling code from startup code
> config: x86_64-randconfig-161-20250510 (https://download.01.org/0day-ci/archive/20250510/202505100719.9pE7wDfB-lkp@intel.com/config)
> compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> | Closes: https://lore.kernel.org/r/202505100719.9pE7wDfB-lkp@intel.com/
>
> smatch warnings:
> 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)
>
> vim +/boot_ghcb +104 arch/x86/boot/compressed/sev-handle-vc.c
>
> ed4d95d033e359 Ard Biesheuvel 2025-05-04   96  void do_boot_stage2_vc(struct pt_regs *regs, unsigned long exit_code)
> ed4d95d033e359 Ard Biesheuvel 2025-05-04   97  {
> ed4d95d033e359 Ard Biesheuvel 2025-05-04   98   struct es_em_ctxt ctxt;
> ed4d95d033e359 Ard Biesheuvel 2025-05-04   99   enum es_result result;
> ed4d95d033e359 Ard Biesheuvel 2025-05-04  100
> ed4d95d033e359 Ard Biesheuvel 2025-05-04 @101   if (!boot_ghcb && !early_setup_ghcb())
>                                                     ^^^^^^^^^^
> Check for NULL.  Should the && be ||?
>
> ed4d95d033e359 Ard Biesheuvel 2025-05-04  102           sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SEV_ES_GEN_REQ);
> ed4d95d033e359 Ard Biesheuvel 2025-05-04  103
> ed4d95d033e359 Ard Biesheuvel 2025-05-04 @104   vc_ghcb_invalidate(boot_ghcb);
>                                                                    ^^^^^^^^^
> Unchecked dereference.
>

On success, early_setup_ghcb() will assign boot_ghcb, and so it is
only called if it was unset.

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(...);
}

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)

[Dan Carpenter]

On Sat, May 10, 2025 at 09:57:23AM +0200, Ard Biesheuvel wrote:
> Hi Dan,
> 
> On Sat, 10 May 2025 at 09:43, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> >
> > Hi Ard,
> >
> > FYI, the error/warning was bisected to this commit, please ignore it if it's irrelevant.
> >
> > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/boot
> > head:   ed4d95d033e359f9445e85bf5a768a5859a5830b
> > commit: ed4d95d033e359f9445e85bf5a768a5859a5830b [10/10] x86/sev: Disentangle #VC handling code from startup code
> > config: x86_64-randconfig-161-20250510 (https://download.01.org/0day-ci/archive/20250510/202505100719.9pE7wDfB-lkp@intel.com/config)
> > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> >
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > | Closes: https://lore.kernel.org/r/202505100719.9pE7wDfB-lkp@intel.com/
> >
> > smatch warnings:
> > 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)
> >
> > vim +/boot_ghcb +104 arch/x86/boot/compressed/sev-handle-vc.c
> >
> > ed4d95d033e359 Ard Biesheuvel 2025-05-04   96  void do_boot_stage2_vc(struct pt_regs *regs, unsigned long exit_code)
> > ed4d95d033e359 Ard Biesheuvel 2025-05-04   97  {
> > ed4d95d033e359 Ard Biesheuvel 2025-05-04   98   struct es_em_ctxt ctxt;
> > ed4d95d033e359 Ard Biesheuvel 2025-05-04   99   enum es_result result;
> > ed4d95d033e359 Ard Biesheuvel 2025-05-04  100
> > ed4d95d033e359 Ard Biesheuvel 2025-05-04 @101   if (!boot_ghcb && !early_setup_ghcb())
> >                                                     ^^^^^^^^^^
> > Check for NULL.  Should the && be ||?
> >
> > ed4d95d033e359 Ard Biesheuvel 2025-05-04  102           sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SEV_ES_GEN_REQ);
> > ed4d95d033e359 Ard Biesheuvel 2025-05-04  103
> > ed4d95d033e359 Ard Biesheuvel 2025-05-04 @104   vc_ghcb_invalidate(boot_ghcb);
> >                                                                    ^^^^^^^^^
> > Unchecked dereference.
> >
> 
> On success, early_setup_ghcb() will assign boot_ghcb, and so it is
> only called if it was unset.
> 
> 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(...);
> }

Ah.  Thanks.  I didn't even think that early_setup_ghcb() might
set boot_ghcb().

regards,
dan carpenter

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)

[Borislav Petkov]

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@alien8.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@linaro.org>
Suggested-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.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

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)

[Tom Lendacky]

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@alien8.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@linaro.org>
> Suggested-by: Ard Biesheuvel <ardb@kernel.org>
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.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,

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)

[Borislav Petkov]

On Fri, Sep 12, 2025 at 01:34:09PM -0500, Tom Lendacky wrote:
> Shouldn't this routine then delete the line that assigns boot_ghcb and
> instead return &boot_ghcb_page?

Yeah.

Thx.

-- 
Regards/Gruss,
    Boris.

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