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://www.amd.com/system/files/TechDocs/40332_4.05.pdf
Fixes: cbd3d4f7c4e5 ("x86/sev: Check SEV-SNP features support")
CC: Borislav Petkov <[email protected]>
CC: Michael Roth <[email protected]>
CC: Tom Lendacky <[email protected]>
CC: <[email protected]>
Signed-off-by: Nikunj A Dadhania <[email protected]>
---
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|
++---------------+---------------+---------------+---------------+
+
+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
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.
+ */
+#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);
+
enforce_vmpl0();
}
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 37ff47552bcb..d3fe82c5d6b6 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -566,6 +566,26 @@
#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)
+/* SNP feature bits enabled by the hypervisor */
+#define MSR_AMD64_SNP_VTOM BIT_ULL(3)
+#define MSR_AMD64_SNP_REFLECT_VC BIT_ULL(4)
+#define MSR_AMD64_SNP_RESTRICTED_INJ BIT_ULL(5)
+#define MSR_AMD64_SNP_ALT_INJ BIT_ULL(6)
+#define MSR_AMD64_SNP_DEBUG_SWAP BIT_ULL(7)
+#define MSR_AMD64_SNP_PREVENT_HOST_IBS BIT_ULL(8)
+#define MSR_AMD64_SNP_BTB_ISOLATION BIT_ULL(9)
+#define MSR_AMD64_SNP_VMPL_SSS BIT_ULL(10)
+#define MSR_AMD64_SNP_SECURE_TSC BIT_ULL(11)
+#define MSR_AMD64_SNP_VMGEXIT_PARAM BIT_ULL(12)
+#define MSR_AMD64_SNP_IBS_VIRT BIT_ULL(14)
+#define MSR_AMD64_SNP_VMSA_REG_PROTECTION BIT_ULL(16)
+#define MSR_AMD64_SNP_SMT_PROTECTION BIT_ULL(17)
+
+/* SNP feature bits reserved for future use. */
+#define MSR_AMD64_SNP_RESERVED_BIT13 BIT_ULL(13)
+#define MSR_AMD64_SNP_RESERVED_BIT15 BIT_ULL(15)
+#define MSR_AMD64_SNP_RESERVED_MASK GENMASK_ULL(63, 18)
+
#define MSR_AMD64_VIRT_SPEC_CTRL 0xc001011f
/* AMD Collaborative Processor Performance Control MSRs */
diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
index b8357d6ecd47..db60cbb01b31 100644
--- a/arch/x86/include/asm/sev-common.h
+++ b/arch/x86/include/asm/sev-common.h
@@ -148,6 +148,7 @@ struct snp_psc_desc {
#define GHCB_SEV_ES_GEN_REQ 0
#define GHCB_SEV_ES_PROT_UNSUPPORTED 1
#define GHCB_SNP_UNSUPPORTED 2
+#define GHCB_SNP_FEAT_NOT_IMPLEMENTED 3
/* Linux-specific reason codes (used with reason set 1) */
#define SEV_TERM_SET_LINUX 1
--
2.32.0
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://www.amd.com/system/files/TechDocs/40332_4.05.pdf
>
> Fixes: cbd3d4f7c4e5 ("x86/sev: Check SEV-SNP features support")
> CC: Borislav Petkov <[email protected]>
> CC: Michael Roth <[email protected]>
> CC: Tom Lendacky <[email protected]>
> CC: <[email protected]>
> Signed-off-by: Nikunj A Dadhania <[email protected]>
>
> ---
>
> 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. 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}?"
> +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
> 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?
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.
> + */
> +#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)?
> +
> enforce_vmpl0();
> }
>
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 37ff47552bcb..d3fe82c5d6b6 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -566,6 +566,26 @@
> #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)
>
> +/* SNP feature bits enabled by the hypervisor */
> +#define MSR_AMD64_SNP_VTOM BIT_ULL(3)
> +#define MSR_AMD64_SNP_REFLECT_VC BIT_ULL(4)
> +#define MSR_AMD64_SNP_RESTRICTED_INJ BIT_ULL(5)
> +#define MSR_AMD64_SNP_ALT_INJ BIT_ULL(6)
> +#define MSR_AMD64_SNP_DEBUG_SWAP BIT_ULL(7)
> +#define MSR_AMD64_SNP_PREVENT_HOST_IBS BIT_ULL(8)
> +#define MSR_AMD64_SNP_BTB_ISOLATION BIT_ULL(9)
> +#define MSR_AMD64_SNP_VMPL_SSS BIT_ULL(10)
> +#define MSR_AMD64_SNP_SECURE_TSC BIT_ULL(11)
> +#define MSR_AMD64_SNP_VMGEXIT_PARAM BIT_ULL(12)
> +#define MSR_AMD64_SNP_IBS_VIRT BIT_ULL(14)
> +#define MSR_AMD64_SNP_VMSA_REG_PROTECTION BIT_ULL(16)
> +#define MSR_AMD64_SNP_SMT_PROTECTION BIT_ULL(17)
> +
> +/* SNP feature bits reserved for future use. */
> +#define MSR_AMD64_SNP_RESERVED_BIT13 BIT_ULL(13)
> +#define MSR_AMD64_SNP_RESERVED_BIT15 BIT_ULL(15)
> +#define MSR_AMD64_SNP_RESERVED_MASK GENMASK_ULL(63, 18)
> +
> #define MSR_AMD64_VIRT_SPEC_CTRL 0xc001011f
>
> /* AMD Collaborative Processor Performance Control MSRs */
> diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
> index b8357d6ecd47..db60cbb01b31 100644
> --- a/arch/x86/include/asm/sev-common.h
> +++ b/arch/x86/include/asm/sev-common.h
> @@ -148,6 +148,7 @@ struct snp_psc_desc {
> #define GHCB_SEV_ES_GEN_REQ 0
> #define GHCB_SEV_ES_PROT_UNSUPPORTED 1
> #define GHCB_SNP_UNSUPPORTED 2
> +#define GHCB_SNP_FEAT_NOT_IMPLEMENTED 3
>
> /* Linux-specific reason codes (used with reason set 1) */
> #define SEV_TERM_SET_LINUX 1
> --
> 2.32.0
>
>
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 <[email protected]>
>> CC: Michael Roth <[email protected]>
>> CC: Tom Lendacky <[email protected]>
>> CC: <[email protected]>
>> Signed-off-by: Nikunj A Dadhania <[email protected]>
>>
>> ---
>>
>> 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
On Mon, Jan 02, 2023 at 08:50:23PM +0530, Nikunj A. Dadhania wrote:
> 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.
I think you mean "does not have" here.
In any case, I think this whole handling of SEV features could go both ways:
* Cloud provider could say: we've enabled features X, Y and Z and if the guest
doesn't have support for them, then it would fail booting.
There would optimally be some text sowewhere in the cloud provider documentation
stating why those features are enabled and thus required to be supported by the
guest.
* Guest owner could require a minimal subset of features which must be present
in the HV in order to even boot on that HV.
Of course, I'm only speculating here. How it ends up really playing out in
reality we will have to see...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Mon, Jan 02, 2023 at 08:50:23PM +0530, Nikunj A. Dadhania wrote:
> >> + /*
> >> + * 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.
I think what David means is, can we have sev_es_terminate() say exactly which
feature wasn't implemented instead of users having to dig out which one exactly
wasn't by trying to find out what their SNP_FEATURES_IMPL_REQ and
SNP_FEATURES_PRESENT masks are.
Looking at the GHCB protocol, where GHCB_SNP_FEAT_NOT_IMPLEMENTED reason code
goes is GHCBData[23:16] which is not enough... And the VMSA has SEV_FEATURES but
that's guest-only.
I guess we need a way to communicate those masks in a more user-friendly way so
that it is exactly clear because of which missing feature(s) has the guest
terminated.
Hmm.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 03/01/23 01:12, Borislav Petkov wrote:
> On Mon, Jan 02, 2023 at 08:50:23PM +0530, Nikunj A. Dadhania wrote:
>> 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.
>
> I think you mean "does not have" here.
Yes, that is correct.
>
> In any case, I think this whole handling of SEV features could go both ways:
>
> * Cloud provider could say: we've enabled features X, Y and Z and if the guest
> doesn't have support for them, then it would fail booting.
>
> There would optimally be some text sowewhere in the cloud provider documentation
> stating why those features are enabled and thus required to be supported by the
> guest.
>
> * Guest owner could require a minimal subset of features which must be present
> in the HV in order to even boot on that HV.
>
> Of course, I'm only speculating here. How it ends up really playing out in
> reality we will have to see...
>
On 03/01/23 01:32, Borislav Petkov wrote:
> On Mon, Jan 02, 2023 at 08:50:23PM +0530, Nikunj A. Dadhania wrote:
>>>> + /*
>>>> + * 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.
>
> I think what David means is, can we have sev_es_terminate() say exactly which
> feature wasn't implemented instead of users having to dig out which one exactly
> wasn't by trying to find out what their SNP_FEATURES_IMPL_REQ and
> SNP_FEATURES_PRESENT masks are.
>
> Looking at the GHCB protocol, where GHCB_SNP_FEAT_NOT_IMPLEMENTED reason code
> goes is GHCBData[23:16] which is not enough... And the VMSA has SEV_FEATURES but
> that's guest-only.
Currently, GHCBData[24:63] is unused. If we intend to use the bit range(40bits), GHCB spec
will need to be updated. And probably would not be enough.
> I guess we need a way to communicate those masks in a more user-friendly way so
> that it is exactly clear because of which missing feature(s) has the guest
> terminated.
As the termination request is done using GHCB MSR protocol, exit codes cannot be used.
Regards,
Nikunj
On Tue, Jan 03, 2023 at 09:07:14AM +0530, Nikunj A. Dadhania wrote:
> Currently, GHCBData[24:63] is unused. If we intend to use the bit range(40bits), GHCB spec
> will need to be updated. And probably would not be enough.
My fear too...
> As the termination request is done using GHCB MSR protocol, exit codes cannot be used.
We need to figure out some other way of communicating to the guest owner because
of which feature the guest refused booting.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Mon, Jan 02, 2023 at 02:08:10PM +0530, 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
^
the
> 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://www.amd.com/system/files/TechDocs/40332_4.05.pdf
>
> Fixes: cbd3d4f7c4e5 ("x86/sev: Check SEV-SNP features support")
> CC: Borislav Petkov <[email protected]>
> CC: Michael Roth <[email protected]>
> CC: Tom Lendacky <[email protected]>
> CC: <[email protected]>
> Signed-off-by: Nikunj A Dadhania <[email protected]>
...
> 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):
No ":"
> +===========================
<---- newline here.
> +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|
> ++---------------+---------------+---------------+---------------+
sphinx is not happy about that table for some reason. I always find the error
messages cryptic though:
Documentation/x86/amd-memory-encryption.rst:110: WARNING: Block quote ends without a blank line; unexpected unindent.
Documentation/x86/amd-memory-encryption.rst:110: WARNING: Block quote ends without a blank line; unexpected unindent.
Documentation/x86/amd-memory-encryption.rst:122: WARNING: Block quote ends without a blank line; unexpected unindent.
Documentation/x86/amd-memory-encryption.rst:128: WARNING: Block quote ends without a blank line; unexpected unindent.
You can repro by doing "make htmldocs".
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, 3 Jan 2023, Borislav Petkov wrote:
> On Tue, Jan 03, 2023 at 09:07:14AM +0530, Nikunj A. Dadhania wrote:
> > Currently, GHCBData[24:63] is unused. If we intend to use the bit range(40bits), GHCB spec
> > will need to be updated. And probably would not be enough.
>
> My fear too...
>
> > As the termination request is done using GHCB MSR protocol, exit codes cannot be used.
>
> We need to figure out some other way of communicating to the guest owner because
> of which feature the guest refused booting.
>
Yeah, this was exactly the thought. If the guest terminates because
SNP_FEAT_NOT_IMPLEMENTED, it seems necessary to provide information on
which feature(s) the guest is lacking an implementation for that the
hypervisor has enabled. Otherwise this becomes a frustrating trial and
error for the customer.
On 03/01/23 19:10, Borislav Petkov wrote:
> On Mon, Jan 02, 2023 at 02:08:10PM +0530, 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
> ^
> the
Sure.
>
>> 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%7C6575db7c0d8f4f136d1f08daed902274%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638083500715058552%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=weTrXWfPxDBdu9OsFZ4FxlvlgbhhG%2F985%2Bii%2BM8vh6I%3D&reserved=0
>>
>> Fixes: cbd3d4f7c4e5 ("x86/sev: Check SEV-SNP features support")
>> CC: Borislav Petkov <[email protected]>
>> CC: Michael Roth <[email protected]>
>> CC: Tom Lendacky <[email protected]>
>> CC: <[email protected]>
>> Signed-off-by: Nikunj A Dadhania <[email protected]>
>
> ...
>
>> 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):
>
> No ":"
>
Done
>> +===========================
>
> <---- newline here.
Done
>
>> +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|
>> ++---------------+---------------+---------------+---------------+
>
> sphinx is not happy about that table for some reason. I always find the error
> messages cryptic though:
sphinx uses spaces before multi-line text as block quote. Also, found that after the table header it needs line with "=" and not "-".
> Documentation/x86/amd-memory-encryption.rst:110: WARNING: Block quote ends without a blank line; unexpected unindent.
> Documentation/x86/amd-memory-encryption.rst:110: WARNING: Block quote ends without a blank line; unexpected unindent.
> Documentation/x86/amd-memory-encryption.rst:122: WARNING: Block quote ends without a blank line; unexpected unindent.
> Documentation/x86/amd-memory-encryption.rst:128: WARNING: Block quote ends without a blank line; unexpected unindent.
>
> You can repro by doing "make htmldocs".
Fixed,
+-----------------+---------------+---------------+------------------+
| Feature Enabled | Guest needs | Guest has | Guest boot |
| by the HV | implementation| implementation| behaviour |
+=================+===============+===============+==================+
| 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 |
+-----------------+---------------+---------------+------------------+
Regards
Nikunj