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

From: Nikunj A. Dadhania
Date: Mon Jan 02 2023 - 10:21:20 EST



On 02/01/23 16:53, David Rientjes wrote:
> On Mon, 2 Jan 2023, Nikunj A Dadhania wrote:
>
>> The hypervisor can enable various new features (SEV_FEATURES[1:63])
>> and start the SNP guest. Some of these features need guest side
>> implementation. If any of these features are enabled without guest
>> side implementation, the behavior of the SNP guest will be undefined.
>> The SNP guest boot may fail in a non-obvious way making it difficult
>> to debug.
>>
>> Instead of allowing the guest to continue and have it fail randomly
>> later, detect this early and fail gracefully.
>>
>> SEV_STATUS MSR indicates features which hypervisor has enabled. While
>> booting, SNP guests should ascertain that all the enabled features
>> have guest side implementation. In case any feature is not implemented
>> in the guest, the guest terminates booting with SNP feature
>> unsupported exit code.
>>
>> 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%7Cd4d286d55435487f7d4f08daecb3d3e6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638082554474351848%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=TLBCkg20WCaF77YrOzGC%2Bn9V3JFgVl2vs1XMPmSWHcM%3D&reserved=0
>>
>> Fixes: cbd3d4f7c4e5 ("x86/sev: Check SEV-SNP features support")
>> CC: Borislav Petkov <bp@xxxxxxxxx>
>> CC: Michael Roth <michael.roth@xxxxxxx>
>> CC: Tom Lendacky <thomas.lendacky@xxxxxxx>
>> CC: <stable@xxxxxxxxxx>
>> Signed-off-by: Nikunj A Dadhania <nikunj@xxxxxxx>
>>
>> ---
>>
>> Changes:
>> v2:
>> * Updated Documentation/x86/amd-memory-encryption.rst
>> * Address review feedback from Boris/Tom
>>
>> v1:
>> * Dropped _ENABLED from the feature bits
>> * Use approprate macro/function names and move closer to the function where
>> it is used.
>> * More details added to the commit message and comments
>> * Fixed compilation issue
>> ---
>> Documentation/x86/amd-memory-encryption.rst | 35 +++++++++++++++++++
>> arch/x86/boot/compressed/sev.c | 37 +++++++++++++++++++++
>> arch/x86/include/asm/msr-index.h | 20 +++++++++++
>> arch/x86/include/asm/sev-common.h | 1 +
>> 4 files changed, 93 insertions(+)
>>
>> diff --git a/Documentation/x86/amd-memory-encryption.rst b/Documentation/x86/amd-memory-encryption.rst
>> index a1940ebe7be5..b8b6b87be995 100644
>> --- a/Documentation/x86/amd-memory-encryption.rst
>> +++ b/Documentation/x86/amd-memory-encryption.rst
>> @@ -95,3 +95,38 @@ by supplying mem_encrypt=on on the kernel command line. However, if BIOS does
>> not enable SME, then Linux will not be able to activate memory encryption, even
>> if configured to do so by default or the mem_encrypt=on command line parameter
>> is specified.
>> +
>> +Secure Nested Paging (SNP):
>> +===========================
>> +SEV-SNP introduces new features (SEV_FEATURES[1:63]) which can be enabled
>> +by the hypervisor for security enhancements. Some of these features need
>> +guest side implementation to function correctly. The below table lists the
>> +expected guest behavior with various possible scenarios of guest/hypervisor
>> +SNP feature support.
>> +
>> ++---------------+---------------+---------------+---------------+
>> +|Feature Enabled| Guest needs | Guest has | Guest boot |
>> +| by HV |implementation |implementation | behavior |
>> ++---------------+---------------+---------------+---------------+
>> +| No | No | No | Boot |
>> +| | | | |
>> ++---------------+---------------+---------------+---------------+
>> +| No | Yes | No | Boot |
>> +| | | | |
>> ++---------------+---------------+---------------+---------------+
>> +| No | Yes | Yes | Boot |
>> +| | | | |
>> ++---------------+---------------+---------------+---------------+
>> +| Yes | No | No | Boot with |
>> +| | | |feature enabled|
>> ++---------------+---------------+---------------+---------------+
>> +| Yes | Yes | No | Graceful Boot |
>> +| | | | Failure |
>> ++---------------+---------------+---------------+---------------+
>> +| Yes | Yes | Yes | Boot with |
>> +| | | |feature enabled|
>> ++---------------+---------------+---------------+---------------+
>> +
>
> Couple things:
>
> - I'd assume that the documentation would refer the reader to the
> SNP_FEATURES_IMPL_REQ mask that defines whether the guest is required
> to have a specific feature or not.>
> - Don't hate me, but I feel wanting more from this, such as a rationale
> for why certain features are required in the guest.

The guest can boot without any of these features provided the hypervisor has
not enabled any of the SNP_FEATURES_IMPL_REQ features. But in case the hypervisor
does enable them, the guest needs to react in a predictable manner.

These are optional security features provided for SNP guests which the hypervisor
can enable.

> When a guest fails to boot and the reasoning provided by a cloud provider
> is "your guest doesn't support ${feature}", the natural question to ask
> will be "why do I need ${feature}?"

I think the "why" part depends on the user. Whether or not the user needs a
certain feature enabled for the confidential guest.

If the cloud provider(hypervisor) enables the feature on user request, the guest
terminates with GHCB_SNP_FEAT_NOT_IMPLEMENTED when guest kernel does have corresponding
code/implementation.


>
>> +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%7Cd4d286d55435487f7d4f08daecb3d3e6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638082554474351848%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=TLBCkg20WCaF77YrOzGC%2Bn9V3JFgVl2vs1XMPmSWHcM%3D&reserved=0
>> diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
>> index c93930d5ccbd..43793f46cfc1 100644
>> --- a/arch/x86/boot/compressed/sev.c
>> +++ b/arch/x86/boot/compressed/sev.c
>> @@ -270,6 +270,36 @@ static void enforce_vmpl0(void)
>> sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_NOT_VMPL0);
>> }
>>
>> +/*
>> + * SNP_FEATURES_IMPL_REQ is the mask of SNP features that will need
>> + * guest side implementation for proper functioning of the guest. If any
>> + * of these features are enabled in the hypervisor but are lacking guest
>> + * side implementation, the behavior of the guest will be undefined. The
>> + * guest could fail in non-obvious way making it difficult to debug.
>> + *
>> + * As the behavior of reserved feature bits is unknown to be on the
>> + * safe side add them to the required features mask.
>
> If one or more of the reserved feature bits turns out to not be required
> for the SNP guest to function correctly, is this the correct choice?

Yes, I think so. As they are undefined at the moment, it is safe to assume
that a guest implementation is required.

>
> IIUC, legacy guests would fail to boot correctly (and unnecessarily,
> because of this change) if they do not have an implentation for a
> non-required feature. Absent this change, however, they would proceed
> just fine.

True, but the other way around guest behaviour may be undefined.

>
>> + */
>> +#define SNP_FEATURES_IMPL_REQ (MSR_AMD64_SNP_VTOM | \
>> + MSR_AMD64_SNP_REFLECT_VC | \
>> + MSR_AMD64_SNP_RESTRICTED_INJ | \
>> + MSR_AMD64_SNP_ALT_INJ | \
>> + MSR_AMD64_SNP_DEBUG_SWAP | \
>> + MSR_AMD64_SNP_VMPL_SSS | \
>> + MSR_AMD64_SNP_SECURE_TSC | \
>> + MSR_AMD64_SNP_VMGEXIT_PARAM | \
>> + MSR_AMD64_SNP_VMSA_REG_PROTECTION | \
>> + MSR_AMD64_SNP_RESERVED_BIT13 | \
>> + MSR_AMD64_SNP_RESERVED_BIT15 | \
>> + MSR_AMD64_SNP_RESERVED_MASK)
>> +
>> +/*
>> + * SNP_FEATURES_PRESENT is the mask of SNP features that are implemented
>> + * by the guest kernel. As and when a new feature is implemented in the
>> + * guest kernel, a corresponding bit should be added to the mask.
>> + */
>> +#define SNP_FEATURES_PRESENT (0)
>> +
>> void sev_enable(struct boot_params *bp)
>> {
>> unsigned int eax, ebx, ecx, edx;
>> @@ -335,6 +365,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 any feature
>> + * lacking guest side implementation.
>> + */
>> + if (sev_status & SNP_FEATURES_IMPL_REQ & ~SNP_FEATURES_PRESENT)
>> + sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_FEAT_NOT_IMPLEMENTED);
>
> We can't help out by specifying which feature(s)?

The purpose of SNP_FEATURES_PRESENT is just that, at present no features that need guest
implementation is part of the kernel. For e.g. I will be posting patches with SecureTSC
enabled, that will make the following change.

diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
index 43793f46cfc1..cd0948c6db50 100644
--- a/arch/x86/boot/compressed/sev.c
+++ b/arch/x86/boot/compressed/sev.c
@@ -298,7 +298,7 @@ static void enforce_vmpl0(void)
* by the guest kernel. As and when a new feature is implemented in the
* guest kernel, a corresponding bit should be added to the mask.
*/
-#define SNP_FEATURES_PRESENT (0)
+#define SNP_FEATURES_PRESENT (MSR_AMD64_SNP_SECURE_TSC)

void sev_enable(struct boot_params *bp)
{


Regards,
Nikunj