x86/sev: Add SEV-SNP guest feature negotiation support

  SEV_STATUS indicates features that hypervisor has enabled. Guest
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.

More details in AMD64 APM[1] Vol 2: 15.34.10 SEV_STATUS MSR

[1] https://www.amd.com/system/files/TechDocs/40332_4.05.pdf

Fixes: cbd3d4f7c4e5 ("x86/sev: Check SEV-SNP features support")
CC: Michael Roth <michael.roth@amd.com>
CC: Tom Lendacky <thomas.lendacky@amd.com>
CC: <stable@kernel.org>
Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
---
 arch/x86/boot/compressed/sev.c   | 18 +++++++++++++
 arch/x86/include/asm/msr-index.h | 46 +++++++++++++++++++++++++++++---
 2 files changed, 61 insertions(+), 3 deletions(-)
  

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 ..."

> 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.

> More details in AMD64 APM[1] Vol 2: 15.34.10 SEV_STATUS MSR
> 
> [1] https://www.amd.com/system/files/TechDocs/40332_4.05.pdf
> 
> Fixes: cbd3d4f7c4e5 ("x86/sev: Check SEV-SNP features support")
> CC: Michael Roth <michael.roth@amd.com>
> CC: Tom Lendacky <thomas.lendacky@amd.com>
> CC: <stable@kernel.org>
> Signed-off-by: Nikunj A Dadhania <nikunj@amd.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?

In file included from ./arch/x86/include/asm/msr.h:5,
                 from ./arch/x86/include/asm/processor.h:22,
                 from ./arch/x86/include/asm/cpufeature.h:5,
                 from ./arch/x86/include/asm/thread_info.h:53,
                 from ./include/linux/thread_info.h:60,
                 from ./arch/x86/include/asm/elf.h:8,
                 from ./include/linux/elf.h:6,
                 from arch/x86/boot/compressed/misc.h:24,
                 from arch/x86/boot/compressed/sev.c:13:
arch/x86/boot/compressed/sev.c: In function ‘snp_guest_feature_supported’:
./arch/x86/include/asm/msr-index.h:602:37: error: ‘MSR_AMD64_SNP_BIT13_RESERVED_ENABLED’ undeclared (first use in this function); did you mean ‘MSR_AMD64_SNP_BIT13_RESERVED’?
  602 |                                     MSR_AMD64_SNP_BIT13_RESERVED_ENABLED |      \
      |                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./arch/x86/include/asm/msr-index.h:602:37: note: in definition of macro ‘SNP_GUEST_SUPPORT_REQUIRED’
  602 |                                     MSR_AMD64_SNP_BIT13_RESERVED_ENABLED |      \
      |                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./arch/x86/include/asm/msr-index.h:602:37: note: each undeclared identifier is reported only once for each function it appears in
  602 |                                     MSR_AMD64_SNP_BIT13_RESERVED_ENABLED |      \
      |                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./arch/x86/include/asm/msr-index.h:602:37: note: in definition of macro ‘SNP_GUEST_SUPPORT_REQUIRED’
  602 |                                     MSR_AMD64_SNP_BIT13_RESERVED_ENABLED |      \
      |                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./arch/x86/include/asm/msr-index.h:604:37: error: ‘MSR_AMD64_SNP_BIT15_RESERVED_ENABLED’ undeclared (first use in this function); did you mean ‘MSR_AMD64_SNP_BIT15_RESERVED’?
  604 |                                     MSR_AMD64_SNP_BIT15_RESERVED_ENABLED |      \
      |                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./arch/x86/include/asm/msr-index.h:604:37: note: in definition of macro ‘SNP_GUEST_SUPPORT_REQUIRED’
  604 |                                     MSR_AMD64_SNP_BIT15_RESERVED_ENABLED |      \
      |                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./arch/x86/include/asm/msr-index.h:605:37: error: ‘MSR_AMD64_SNP_MASK_RESERVED_ENABLED’ undeclared (first use in this function); did you mean ‘MSR_AMD64_SNP_MASK_RESERVED’?
  605 |                                     MSR_AMD64_SNP_MASK_RESERVED_ENABLED)
      |                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./arch/x86/include/asm/msr-index.h:605:37: note: in definition of macro ‘SNP_GUEST_SUPPORT_REQUIRED’
  605 |                                     MSR_AMD64_SNP_MASK_RESERVED_ENABLED)
      |                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
make[2]: *** [scripts/Makefile.build:250: arch/x86/boot/compressed/sev.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [arch/x86/boot/Makefile:116: arch/x86/boot/compressed/vmlinux] Error 2
make: *** [arch/x86/Makefile:283: bzImage] Error 2

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.

> 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?

Because right now it will be dead code.

So what's the actual purpose of this patch?

> +/*
> + * 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?

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

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.
  

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&data=05%7C01%7Cnikunj.dadhania%40amd.com%7C44df93f0ef4349db849608dac8885136%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638042785223235199%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=RMoXNQZXTA2O%2F%2BJQHjmWCPMzkzITXt8F071UEYlOyIQ%3D&reserved=0
>>
>> Fixes: cbd3d4f7c4e5 ("x86/sev: Check SEV-SNP features support")
>> CC: Michael Roth <michael.roth@amd.com>
>> CC: Tom Lendacky <thomas.lendacky@amd.com>
>> CC: <stable@kernel.org>
>> Signed-off-by: Nikunj A Dadhania <nikunj@amd.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
  

On Thu, Nov 17, 2022 at 05:50:34PM +0530, Nikunj A. Dadhania wrote:
> 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:

This all is still veiled in mist to me. What are you trying to do here?

- Make sure older SNP guests boot on newer hypervisors?

- Newer guests boot on older hypervisors?

So, first, pls explain in detail what the goal here is.

I'm reading the above in multiple ways so you need to spell out first
what you wanna do.

PREVENT_HOST_IBS doesn't need any enablement. So why is it in the mask?

SECURE_TSC needs enablement on both. Why aren't you checking only this
one.

IOW, I would expect to check *only* for features which the guest needs
for the hypervisor to support before it boots. But not check everything
wholesale.

IOW, I see it this way: guest boots, sees what the hypervisor has
enabled as SEV_STATUS cannot be intercepted and acts accordingly.

Now, the question how *old* guests should act here is a whole different
story as it depends on whether this gets backported to old guests -
which doesn't make them old anymore as the checking will happen - or to
really old guests without the checking. There it doesn't matter.

And come to think of it, this whole deal is no different than having
feature bits in CPUID and the kernel implementing them.

If the kernel finds a feature bit set in CPUID, it enables the
corresponding code. If it doesn't know about it, then it doesn't do
anything.

Pretty much the same here: if a SNP guest finds a feature flag in
SEV_STATUS, then it enables the code corresponding to it. If it doesn't
find it but it needs it due to enablement, then it stops booting.

So let's define the problem first.

Thx.
  

On 17/11/22 18:23, Borislav Petkov wrote:
> On Thu, Nov 17, 2022 at 05:50:34PM +0530, Nikunj A. Dadhania wrote:
>> 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:
> 
> This all is still veiled in mist to me. What are you trying to do here?
> 
> - Make sure older SNP guests boot on newer hypervisors?

Yes and No.

Yes: For feature flags that do not need an enlightened guest, older guests should 
     boot.

No: For feature flags that need an enlightened guest, older guests should detect 
    and fail booting on any hypervisor that sets this feature flag. 

> - Newer guests boot on older hypervisors?

Yes, as the older hypervisor won't enable the new SNP feature flag.

> So, first, pls explain in detail what the goal here is.

The hypervisor can enable various new features flags(in SEV_FEATURES[1:63]) 
and start the guest. The hypervisor does not know beforehand that the guest 
kernel supports the feature that is being enabled.

While booting, SNP guests can discover the hypervisor-enabled features by looking 
at SEV_STATUS MSR. At this point, the SNP guest needs to decide either to 
continue boot or terminate depending on the feature support in the guest kernel.
Otherwise, if we let the guest continue booting with an unsupported feature, 
the guest can fail in non-obvious manner. 

* Primary goal here is to detect such unsupported features beforehand and 
  deterministically terminate the guest.
* Secondary goal is to use this mechanism to future-proof the current guests 
  for newer reserved features flags that may be defined later.

Here is the support matrix per feature that can explain better:

HypervisorEnabled: Feature bit is enabled in SEV_FEATURES
Required: Feature requires enlightened guest
Available: The guest is enlightened for that particular feature.
Boot: The expected behavior of the guest, whether it should boot or fail.

+-------------------+----------+-------------+----------+
| HypervisorEnabled | Required | Available   |   Boot   |
+-------------------+----------+-------------+----------+
| Y                 |  Y       |  N          | N (Fail) |
| Y                 |  Y       |  Y          | Y        |
| Y                 |  N       |  N          | Y        |
| N                 | Y/N      | Y/N         | Y        |
+-------------------+----------+-------------+----------+

> I'm reading the above in multiple ways so you need to spell out first
> what you wanna do.
> 
> PREVENT_HOST_IBS doesn't need any enablement. So why is it in the mask?

PREVENT_HOST_IBS will be defined but shouldn't be part of the 
"Required" mask.

> SECURE_TSC needs enablement on both. Why aren't you checking only this
> one.

SECURE_TSC should be part of "Required" mask and once secure tsc 
support is up-streamed it should be added to "Available" mask.

> IOW, I would expect to check *only* for features which the guest needs
> for the hypervisor to support before it boots. But not check everything
> wholesale.

Guests need to check for features enabled by the hypervisor that is not 
supported as well, so that we can terminate the guest right there.

> IOW, I see it this way: guest boots, sees what the hypervisor has
> enabled as SEV_STATUS cannot be intercepted and acts accordingly.
>
> Now, the question how *old* guests should act here is a whole different
> story as it depends on whether this gets backported to old guests -
> which doesn't make them old anymore as the checking will happen - 

Old guests with backport will deny booting with unsupported features. 

> or to
> really old guests without the checking. There it doesn't matter.

It does matter for old guests which dont have backport, the behavior will 
be undefined.

Hence fix needs to be back ported till 5.19, where SNP guest support was added.
 
> And come to think of it, this whole deal is no different than having
> feature bits in CPUID and the kernel implementing them.
> 
> If the kernel finds a feature bit set in CPUID, it enables the
> corresponding code. If it doesn't know about it, then it doesn't do
> anything.
> 
> Pretty much the same here: if a SNP guest finds a feature flag in
> SEV_STATUS, then it enables the code corresponding to it. 

This is the good case, where the feature flag is enabled and the guest 
kernel has support for the feature. This should boot fine.

Second Case which we are handling in this patch: SNP guest finds the 
feature flag enabled, but it does not have code corresponding to that 
feature. The guest boot should *fail*.

> If it doesn't
> find it but it needs it due to enablement, then it stops booting.

Above is the third case: where the SNP guest does not see the feature flag 
enabled and the  guest should boot fine irrespective of whether it is enlightened 
or not.

Regards,
Nikunj
  

On Fri, Nov 18, 2022 at 06:58:07PM +0530, Nikunj A. Dadhania wrote:
> No: For feature flags that need an enlightened guest, older guests
> should detect and fail booting on any hypervisor that sets this
> feature flag.

What would happen with such old guests nowadays? Wouldn't they explode
anyway?

And how is this supposed to work in practice?

I'm guessing this is supposed to address a case where guest owner goes
to a cloud provider, boots an old guest and it magically survives
booting and then it runs with some false sense of security.

But isn't that part of the whole attestation dance where the guest owner
checks for a minimum set of features it expects to be present?

Because if you do this and the cloud provider updates the hypervisor,
all the guest owner images might stop working all of a sudden because of
this check.

So what is the actual, real-life example where this helps? At all?

> The hypervisor can enable various new features flags(in
> SEV_FEATURES[1:63]) and start the guest. The hypervisor does not know
> beforehand that the guest kernel supports the feature that is being
> enabled.

This is not the right criterion: it should be more like: can a guest
still continue booting with a new feature it doesn't know about and
still provide the same security.

But see above - you need to think very practically here before even
considering such a thing.

> While booting, SNP guests can discover the hypervisor-enabled features
> by looking at SEV_STATUS MSR. At this point, the SNP guest needs to
> decide either to continue boot or terminate depending on the feature
> support in the guest kernel. Otherwise, if we let the guest continue
> booting with an unsupported feature, the guest can fail in non-obvious
> manner.

How can an old guest decide when it doesn't even have the intelligence
to do so?

What you're doing is, have old guests immediately stop booting when they
encounter a new feature - even if that new feature doesn't impair their
security.

> +-------------------+----------+-------------+----------+
> | HypervisorEnabled | Required | Available   |   Boot   |
> +-------------------+----------+-------------+----------+
> | Y                 |  Y       |  N          | N (Fail) |

This means that you need to know those features which would fail an old
guest *upfront*. Like right now. And I hardly doubt that.

> PREVENT_HOST_IBS will be defined but shouldn't be part of the 
> "Required" mask.

So it doesn't need to be mentioned at all.

> SECURE_TSC should be part of "Required" mask and once secure tsc 
> support is up-streamed it should be added to "Available" mask.

So when a guest owner gets a new guest which supports SECURE_TSC and
tries to boot it on an older HV - which is very much pretty everywhere
the case - cloud providers won't update their HV kernel whenever - that
new guest won't boot at all.

Which is a bad bad idea. I don't think you want that.

What you want, rather, is to say in the SECURE_TSC enablement code

	pr_warn("HV doesn't support secure TSC - your guest won't be 100% secure\n");

or so.

> Guests need to check for features enabled by the hypervisor that is not 
> supported as well, so that we can terminate the guest right there.

That needs to be done in the feature enablement code of each feature but
not wholesale like this.

Anyway, I think you get my point.

Thx.
  

Hi Boris,

I will work on sending a v2 patch that addresses all your questions/concerns.

On 21/11/22 21:44, Borislav Petkov wrote:
> On Fri, Nov 18, 2022 at 06:58:07PM +0530, Nikunj A. Dadhania wrote:
>> No: For feature flags that need an enlightened guest, older guests
>> should detect and fail booting on any hypervisor that sets this
>> feature flag.
> 
> What would happen with such old guests nowadays? Wouldn't they explode
> anyway?

Yes, and the patch is to prevent that and fail gracefully and so I had 
CC'ed stable.

> And how is this supposed to work in practice?
>
> I'm guessing this is supposed to address a case where guest owner goes
> to a cloud provider, boots an old guest and it magically survives
> booting and then it runs with some false sense of security.
> 
> But isn't that part of the whole attestation dance where the guest owner
> checks for a minimum set of features it expects to be present?
> 
> Because if you do this and the cloud provider updates the hypervisor,
> all the guest owner images might stop working all of a sudden because of
> this check.
> 
> So what is the actual, real-life example where this helps? At all?

A cloud provider having the hypervisor and qemu with Secure TSC support
tries to boot an old guest kernel (v5.19):

"-object sev-snp-guest,...,secure-tsc=on -kernel vmlinuz-5.19.0"

As per the current behavior, the guest kernel will hang.

This patch will make sure that if the hypervisor enables this feature, the 
guest kernel will know early that an unsupported SNP feature is enabled. The guest 
then sends a terminate message to the hypervisor, indicating that its not able
to fulfill hypervisor request to enable SecureTSC and cannot move ahead.

>> The hypervisor can enable various new features flags(in
>> SEV_FEATURES[1:63]) and start the guest. The hypervisor does not know
>> beforehand that the guest kernel supports the feature that is being
>> enabled.
> 
> This is not the right criterion: it should be more like: can a guest
> still continue booting with a new feature it doesn't know about and
> still provide the same security.

That is indeed the criterion. Hence, instead of allowing the guest to 
continue and have it fail randomly later(which would be a debugging nightmare), 
this is to detect the unsupported feature early and fail gracefully.
 
> But see above - you need to think very practically here before even
> considering such a thing.
> 
>> While booting, SNP guests can discover the hypervisor-enabled features
>> by looking at SEV_STATUS MSR. At this point, the SNP guest needs to
>> decide either to continue boot or terminate depending on the feature
>> support in the guest kernel. Otherwise, if we let the guest continue
>> booting with an unsupported feature, the guest can fail in non-obvious
>> manner.
> 
> How can an old guest decide when it doesn't even have the intelligence
> to do so?
> 
> What you're doing is, have old guests immediately stop booting when they
> encounter a new feature - even if that new feature doesn't impair their
> security.

A slight correction:
a) If the new feature was enabled by the HV/QEMU.
AND
b) The new feature requires guest enablement

" -object sev-snp-guest,...,secure-tsc=on -kernel vmlinuz-5.19.0 "

And it is not done wholesale.

> 
>> +-------------------+----------+-------------+----------+
>> | HypervisorEnabled | Required | Available   |   Boot   |
>> +-------------------+----------+-------------+----------+
>> | Y                 |  Y       |  N          | N (Fail) |
> 
> This means that you need to know those features which would fail an old
> guest *upfront*. Like right now. And I hardly doubt that.
> 
>> PREVENT_HOST_IBS will be defined but shouldn't be part of the 
>> "Required" mask.
> 
> So it doesn't need to be mentioned at all.

Yes, we could get rid of defines that do not need to be included in 
required mask.

>> SECURE_TSC should be part of "Required" mask and once secure tsc 
>> support is up-streamed it should be added to "Available" mask.
> 
> So when a guest owner gets a new guest which supports SECURE_TSC and
> tries to boot it on an older HV - which is very much pretty everywhere
> the case - cloud providers won't update their HV kernel whenever - that
> new guest won't boot at all.

I think you misunderstood, if the HV does not have Secure TSC and new guest 
has Secure TSC, still the guest should boot. As the hypervisor didnt request 
the feature to be enabled (below row in the table)

+-------------------+----------+-------------+----------+
| HypervisorEnabled | Required | Available   |   Boot   |
+-------------------+----------+-------------+----------+
| N                 | Y/N      | Y/N         | Y        |
+-------------------+----------+-------------+----------+

> 
> Which is a bad bad idea. I don't think you want that.
> 
> What you want, rather, is to say in the SECURE_TSC enablement code
> 
> 	pr_warn("HV doesn't support secure TSC - your guest won't be 100% secure\n");
> 
> or so.
> 
>> Guests need to check for features enabled by the hypervisor that is not 
>> supported as well, so that we can terminate the guest right there.
> 
> That needs to be done in the feature enablement code of each feature but
> not wholesale like this.
> 
> Anyway, I think you get my point.

Regards
Nikunj