linux-kernel - Re: [RFT PATCH v3 12/21] x86/sev: Unify SEV-SNP hypervisor feature check

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: <20250530111645.GAaDmTneZG7KOX0ApR@fat_crate.local>
Date: Fri, 30 May 2025 13:16:45 +0200
From: Borislav Petkov <bp@...en8.de>
To: Ard Biesheuvel <ardb+git@...gle.com>
Cc: linux-kernel@...r.kernel.org, linux-efi@...r.kernel.org, x86@...nel.org,
	Ard Biesheuvel <ardb@...nel.org>, Ingo Molnar <mingo@...nel.org>,
	Dionna Amalie Glaze <dionnaglaze@...gle.com>,
	Kevin Loughlin <kevinloughlin@...gle.com>,
	Tom Lendacky <thomas.lendacky@....com>
Subject: Re: [RFT PATCH v3 12/21] x86/sev: Unify SEV-SNP hypervisor feature
 check

On Mon, May 12, 2025 at 09:08:47PM +0200, Ard Biesheuvel wrote:
> diff --git a/arch/x86/boot/startup/sev-shared.c b/arch/x86/boot/startup/sev-shared.c
> index 70ad9a0aa023..560985ef8df6 100644
> --- a/arch/x86/boot/startup/sev-shared.c
> +++ b/arch/x86/boot/startup/sev-shared.c
> @@ -66,16 +66,10 @@ sev_es_terminate(unsigned int set, unsigned int reason)
>  		asm volatile("hlt\n" : : : "memory");
>  }
>  
> -/*
> - * The hypervisor features are available from GHCB version 2 onward.
> - */
> -u64 get_hv_features(void)
> +static u64 __head get_hv_features(void)
>  {
>  	u64 val;
>  
> -	if (ghcb_version < 2)
> -		return 0;

Why?

> -
>  	sev_es_wr_ghcb_msr(GHCB_MSR_HV_FT_REQ);
>  	VMGEXIT();
>  
> @@ -86,6 +80,31 @@ u64 get_hv_features(void)
>  	return GHCB_MSR_HV_FT_RESP_VAL(val);
>  }
>  
> +u64 __head snp_check_hv_features(void)
> +{
> +	/*
> +	 * SNP is supported in v2 of the GHCB spec which mandates support for HV
> +	 * features.
> +	 */
> +	if (sev_status & MSR_AMD64_SEV_SNP_ENABLED) {
> +		u64 hv_features;
> +
> +		hv_features = get_hv_features();
> +		if (!(hv_features & GHCB_HV_FT_SNP))
> +			sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
> +
> +		/*
> +		 * Running at VMPL0 is required unless an SVSM is present and
> +		 * the hypervisor supports the required SVSM GHCB events.
> +		 */
> +		if (snp_vmpl > 0 && !(hv_features & GHCB_HV_FT_SNP_MULTI_VMPL))
> +			sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_NOT_VMPL0);
> +
> +		return hv_features;
> +	}
> +	return 0;
> +}
> +
>  int svsm_perform_msr_protocol(struct svsm_call *call)
>  {
>  	u8 pending = 0;
> diff --git a/arch/x86/boot/startup/sme.c b/arch/x86/boot/startup/sme.c
> index 753cd2094080..0ae04e333f51 100644
> --- a/arch/x86/boot/startup/sme.c
> +++ b/arch/x86/boot/startup/sme.c
> @@ -533,6 +533,8 @@ void __head sme_enable(struct boot_params *bp)
>  	if (snp_en ^ !!(msr & MSR_AMD64_SEV_SNP_ENABLED))
>  		snp_abort();
>  
> +	sev_hv_features = snp_check_hv_features();

Is this writing the sev_hv_features declared in

arch/x86/boot/startup/sev-startup.c

?

arch/x86/boot/startup/sev-startup.c:45:u64 sev_hv_features __ro_after_init;
arch/x86/boot/startup/sme.c:536:        sev_hv_features = snp_check_hv_features();
arch/x86/coco/sev/core.c:980:   if (!(sev_hv_features & GHCB_HV_FT_SNP_AP_CREATION))
arch/x86/include/asm/sev-internal.h:5:extern u64 sev_hv_features;
arch/x86/include/asm/sev.h:428:extern u64 sev_hv_features;

I'm confused.

If sev_hv_features is startup code, why isn't it accessed this way...?

/me goes and looks forward in the set...

oh my, that is coming.

> +
>  	/* Check if memory encryption is enabled */
>  	if (feature_mask == AMD_SME_BIT) {
>  		if (!(bp->hdr.xloadflags & XLF_MEM_ENCRYPTION))
> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
> index fa7fdd11a45b..fc4f6f188d42 100644
> --- a/arch/x86/coco/sev/core.c
> +++ b/arch/x86/coco/sev/core.c
> @@ -1264,17 +1264,6 @@ void __init sev_es_init_vc_handling(void)
>  	if (!sev_es_check_cpu_features())
>  		panic("SEV-ES CPU Features missing");
>  
> -	/*
> -	 * SNP is supported in v2 of the GHCB spec which mandates support for HV
> -	 * features.
> -	 */
> -	if (cc_platform_has(CC_ATTR_GUEST_SEV_SNP)) {
> -		sev_hv_features = get_hv_features();
> -
> -		if (!(sev_hv_features & GHCB_HV_FT_SNP))
> -			sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
> -	}

I guess we would've terminated earlier anyway so that's probably ok...

> -
>  	/* Initialize per-cpu GHCB pages */
>  	for_each_possible_cpu(cpu) {
>  		alloc_runtime_data(cpu);

-- 
Regards/Gruss,
    Boris.

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