linux-kernel - Re: [PATCH] x86/sev: Add SEV-SNP guest feature negotiation support

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:   Thu, 17 Nov 2022 17:50:34 +0530
From:   "Nikunj A. Dadhania" <nikunj@....com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org, kvm@...r.kernel.org,
        mingo@...hat.com, tglx@...utronix.de, dave.hansen@...ux.intel.com,
        seanjc@...gle.com, pbonzini@...hat.com, thomas.lendacky@....com,
        michael.roth@....com, stable@...nel.org
Subject: Re: [PATCH] x86/sev: Add SEV-SNP guest feature negotiation support

Hi Boris,
On 17/11/22 16:11, Borislav Petkov wrote:
> On Thu, Nov 17, 2022 at 10:14:33AM +0530, Nikunj A Dadhania wrote:
>> SEV_STATUS indicates features that hypervisor has enabled. Guest
> 
> "... which the hypervisor has ..."

Sure.

> 
>> kernel may not support all the features that the hypervisor has
>> enabled. If the hypervisor has enabled an unsupported feature,
>> notify the hypervisor and terminate the boot.
> 
> This sentence needs expanding: why is the policy of the guest kernel
> such that it must terminate if the hypervisor has enabled unsupported
> features?
> 
> You allude to that in the comments below but this needs to be explained
> here too.

Sure

> 
>> More details in AMD64 APM[1] Vol 2: 15.34.10 SEV_STATUS MSR
>>
>> [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.amd.com%2Fsystem%2Ffiles%2FTechDocs%2F40332_4.05.pdf&amp;data=05%7C01%7Cnikunj.dadhania%40amd.com%7C44df93f0ef4349db849608dac8885136%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638042785223235199%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=RMoXNQZXTA2O%2F%2BJQHjmWCPMzkzITXt8F071UEYlOyIQ%3D&amp;reserved=0
>>
>> Fixes: cbd3d4f7c4e5 ("x86/sev: Check SEV-SNP features support")
>> CC: Michael Roth <michael.roth@....com>
>> CC: Tom Lendacky <thomas.lendacky@....com>
>> CC: <stable@...nel.org>
>> Signed-off-by: Nikunj A Dadhania <nikunj@....com>
>> ---
>>  arch/x86/boot/compressed/sev.c   | 18 +++++++++++++
>>  arch/x86/include/asm/msr-index.h | 46 +++++++++++++++++++++++++++++---
>>  2 files changed, 61 insertions(+), 3 deletions(-)
> 
> Btw, how did you test this patch?

<SNIP> 
> It seems like you're new to this kernel hacking business - please
> remember that it is absolutely mandatory that patches must be properly> tested before sending them upstream.

My bad, was on a separate tree and missed to test it on latest.

>> diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
>> index c93930d5ccbd..847d26e761a6 100644
>> --- a/arch/x86/boot/compressed/sev.c
>> +++ b/arch/x86/boot/compressed/sev.c
>> @@ -270,6 +270,17 @@ static void enforce_vmpl0(void)
>>  		sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_NOT_VMPL0);
>>  }
>>  
>> +static bool snp_guest_feature_supported(void)
>> +{
>> +	u64 guest_support = SNP_GUEST_SUPPORT_REQUIRED & ~SNP_GUEST_SUPPORT_AVAILABLE;
>> +
>> +	/*
>> +	 * Return true when SEV features that hypervisor has enabled are
>> +	 * also supported by SNP guest kernel
>> +	 */
> 
> That comment is kinda obvious.
> 
>> +	return !(sev_status & guest_support);
>> +}
>> +
>>  void sev_enable(struct boot_params *bp)
>>  {
>>  	unsigned int eax, ebx, ecx, edx;
>> @@ -335,6 +346,13 @@ void sev_enable(struct boot_params *bp)
>>  		if (!(get_hv_features() & GHCB_HV_FT_SNP))
>>  			sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
>>  
>> +		/*
>> +		 * Terminate the boot if hypervisor has enabled a feature
>> +		 * unsupported by the guest.
>> +		 */
>> +		if (!snp_guest_feature_supported())
>> +			sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
>> +
>>  		enforce_vmpl0();
>>  	}
>>  
>> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
>> index 4a2af82553e4..d33691b4cb24 100644
>> --- a/arch/x86/include/asm/msr-index.h
>> +++ b/arch/x86/include/asm/msr-index.h
>> @@ -567,9 +567,49 @@
>>  #define MSR_AMD64_SEV_ENABLED_BIT	0
>>  #define MSR_AMD64_SEV_ES_ENABLED_BIT	1
>>  #define MSR_AMD64_SEV_SNP_ENABLED_BIT	2
>> -#define MSR_AMD64_SEV_ENABLED		BIT_ULL(MSR_AMD64_SEV_ENABLED_BIT)
>> -#define MSR_AMD64_SEV_ES_ENABLED	BIT_ULL(MSR_AMD64_SEV_ES_ENABLED_BIT)
>> -#define MSR_AMD64_SEV_SNP_ENABLED	BIT_ULL(MSR_AMD64_SEV_SNP_ENABLED_BIT)
>> +#define MSR_AMD64_SEV_ENABLED				BIT_ULL(MSR_AMD64_SEV_ENABLED_BIT)
>> +#define MSR_AMD64_SEV_ES_ENABLED			BIT_ULL(MSR_AMD64_SEV_ES_ENABLED_BIT)
>> +#define MSR_AMD64_SEV_SNP_ENABLED			BIT_ULL(MSR_AMD64_SEV_SNP_ENABLED_BIT)
>> +#define MSR_AMD64_SNP_VTOM_ENABLED			BIT_ULL(3)
>> +#define MSR_AMD64_SNP_REFLECT_VC_ENABLED		BIT_ULL(4)
>> +#define MSR_AMD64_SNP_RESTRICTED_INJ_ENABLED		BIT_ULL(5)
>> +#define MSR_AMD64_SNP_ALT_INJ_ENABLED			BIT_ULL(6)
>> +#define MSR_AMD64_SNP_DEBUG_SWAP_ENABLED		BIT_ULL(7)
>> +#define MSR_AMD64_SNP_PREVENT_HOST_IBS_ENABLED		BIT_ULL(8)
>> +#define MSR_AMD64_SNP_BTB_ISOLATION_ENABLED		BIT_ULL(9)
>> +#define MSR_AMD64_SNP_VMPL_SSS_ENABLED			BIT_ULL(10)
>> +#define MSR_AMD64_SNP_SECURE_TSC_ENABLED		BIT_ULL(11)
>> +#define MSR_AMD64_SNP_VMGEXIT_PARAM_ENABLED		BIT_ULL(12)
>> +#define MSR_AMD64_SNP_IBS_VIRT_ENABLED			BIT_ULL(14)
>> +#define MSR_AMD64_SNP_VMSA_REG_PROTECTION_ENABLED	BIT_ULL(16)
>> +#define MSR_AMD64_SNP_SMT_PROTECTION_ENABLED		BIT_ULL(17)
>> +/* Prevent hypervisor to enable undefined feature bits */
>> +#define MSR_AMD64_SNP_BIT13_RESERVED			BIT_ULL(13)
>> +#define MSR_AMD64_SNP_BIT15_RESERVED			BIT_ULL(15)
>> +#define MSR_AMD64_SNP_MASK_RESERVED			GENMASK_ULL(63, 18)
> 
> So I don't like this:
> 
> if you're going to enforce those bits, shouldn't that enforcement happen
> after *all* those above have been added to the kernel first?

Not all feature need guest kernel support so I did not use mask, so added all 
the known bit fields that are published in the APM.

> Because right now it will be dead code.
> 
> So what's the actual purpose of this patch?

Purpose of this patch is older guests kernel that have SNP enabled (5.19 onward), 
when a particular SNP feature is enabled by the hypervisor that needs enlightened guest, 
older kernel wont be able to support the feature. There is no mechanism that the hypervisor 
can find out what feature is supported by the SNP guest before hand.

For example PREVENT_HOST_IBS needs changes on hypervisor and no changes in the 
guest kernel. In this any guest kernel having SNP support should work.

While for SECURE_TSC, hypervisor and guest kernel changes are required. And older guest 
kernel will not work if hypervisor enables Secure TSC. When secure tsc feature is enabled
following define should be changed:

#define SNP_GUEST_SUPPORT_AVAILABLE (MSR_AMD64_SNP_SECURE_TSC_ENABLED)
 
>> +/*
>> + * Features that needs enlightened guest and cannot be supported with
>> + * unmodified SNP guest kernel. This is subset of SEV_FEATURES.
>> + */
>> +#define SNP_GUEST_SUPPORT_REQUIRED (MSR_AMD64_SNP_VTOM_ENABLED |		\
>> +				    MSR_AMD64_SNP_REFLECT_VC_ENABLED |		\
>> +				    MSR_AMD64_SNP_RESTRICTED_INJ_ENABLED |	\
>> +				    MSR_AMD64_SNP_ALT_INJ_ENABLED |		\
>> +				    MSR_AMD64_SNP_VMPL_SSS_ENABLED |		\
>> +				    MSR_AMD64_SNP_SECURE_TSC_ENABLED |		\
>> +				    MSR_AMD64_SNP_VMGEXIT_PARAM_ENABLED |	\
>> +				    MSR_AMD64_SNP_BIT13_RESERVED_ENABLED |	\
>> +				    MSR_AMD64_SNP_VMSA_REG_PROTECTION_ENABLED | \
>> +				    MSR_AMD64_SNP_BIT15_RESERVED_ENABLED |	\
>> +				    MSR_AMD64_SNP_MASK_RESERVED_ENABLED)
>> +/*
>> + * Subset of SNP_GUEST_SUPPORT_REQUIRED, advertising the features that are
>> + * supported in this enlightened guest kernel. As and when new features are
>> + * added in the guest kernel, corresponding bit for this feature needs to be
>> + * added as part of SNP_GUEST_SUPPORT_AVAILABLE.
>> + */
>> +#define SNP_GUEST_SUPPORT_AVAILABLE (0)
> 
> The reserved bits 13 and 15 trivially belong here already no?

As they are undefined feature bits, we do not know if it needs enlightened guest, can't add here.

> 
> And I don't like that AVAILABLE thing either. I think this all should be
> concentrated in this single function snp_guest_feature_supported() - it
> should be called
> 
> snp_guest_supports_all_required_features()

snp_guest_supports_all_required_features()
{
	/* Features guest is supporting, currently no SNP features supported */
	u64 supported_features = (0);

	/* Features that will not work without guest support */
	u64 required_features = (SNP_VTOM | REFLECT_VC | ...);

	return !(sev_status & (required_features & ~supported_features));
}

> or so, so that the name says what it does and there you can pick those
> apart and say yes or no at the end.
> 
> I'm also not sure you need each single bit defined separately but rather
> test a mask instead first.
> 
> Also, having "_ENABLED" at the end of each bit name is too much - the
> name is enough.

Will removed _ENABLED, kept it for consistency of previous defines MSR_AMD64_SEV_ENABLED, etc.

Thanks for the review

Regards
Nikunj