| 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
| ||
|
Message-ID: <df8d8451-60d5-e126-6528-8318b689fcc0@amd.com>
Date: Wed, 11 Jan 2023 09:41:34 -0600
From: Tom Lendacky <thomas.lendacky@....com>
To: Nikunj A Dadhania <nikunj@....com>, linux-kernel@...r.kernel.org,
x86@...nel.org, kvm@...r.kernel.org, bp@...en8.de
Cc: mingo@...hat.com, tglx@...utronix.de, dave.hansen@...ux.intel.com,
seanjc@...gle.com, pbonzini@...hat.com, michael.roth@....com,
David Rientjes <rientjes@...gle.com>, stable@...nel.org
Subject: Re: [PATCH v4] x86/sev: Add SEV-SNP guest feature negotiation support
On 1/11/23 00:45, 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 the 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 GHCB
> protocol Non-Automatic Exit(NAE) termination request event[1]. Populate
> SW_EXITINFO2 with mask of unsupported features that the hypervisor
> can easily report to the user.
>
> More details in AMD64 APM[2] Vol 2: 15.34.10 SEV_STATUS MSR
>
> [1] https://developer.amd.com/wp-content/resources/56421.pdf
> 4.1.13 Termination Request
>
> [2] https://www.amd.com/system/files/TechDocs/40332_4.05.pdf
>
> Fixes: cbd3d4f7c4e5 ("x86/sev: Check SEV-SNP features support")
> CC: Borislav Petkov <bp@...en8.de>
> CC: David Rientjes <rientjes@...gle.com>
> CC: Michael Roth <michael.roth@....com>
> CC: Tom Lendacky <thomas.lendacky@....com>
> CC: <stable@...nel.org>
> Signed-off-by: Nikunj A Dadhania <nikunj@....com>
>
> ---
>
> Changes:
> v3:
> * Use GHCB protocol NAE termination event SEV-SNP feature(s)
> not supported along with SW_EXITINFO2 containing mask of the
> unsupported features. Need handling of this event on the HV.
> * Add the SNP features check initialize_identity_maps() when the
> boot GHCB page can be initialized and used.
> * Fixed sphinx warnings in documentation
>
> 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 | 36 +++++++++++++
> arch/x86/boot/compressed/head_64.S | 10 ++++
> arch/x86/boot/compressed/misc.h | 2 +
> arch/x86/boot/compressed/sev.c | 59 +++++++++++++++++++++
> arch/x86/include/asm/msr-index.h | 20 +++++++
> arch/x86/include/asm/sev-common.h | 1 +
> arch/x86/include/uapi/asm/svm.h | 10 ++++
> 7 files changed, 138 insertions(+)
>
> diff --git a/Documentation/x86/amd-memory-encryption.rst b/Documentation/x86/amd-memory-encryption.rst
> index a1940ebe7be5..b3adc39d7735 100644
> --- a/Documentation/x86/amd-memory-encryption.rst
> +++ b/Documentation/x86/amd-memory-encryption.rst
> @@ -95,3 +95,39 @@ 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 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 |
> ++-----------------+---------------+---------------+------------------+
> +
> +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/head_64.S b/arch/x86/boot/compressed/head_64.S
> index a75712991df3..22037443e958 100644
> --- a/arch/x86/boot/compressed/head_64.S
> +++ b/arch/x86/boot/compressed/head_64.S
> @@ -557,6 +557,16 @@ SYM_FUNC_START_LOCAL_NOALIGN(.Lrelocated)
> /* Pass boot_params to initialize_identity_maps() */
> movq (%rsp), %rdi
> call initialize_identity_maps
> +
> +#ifdef CONFIG_AMD_MEM_ENCRYPT
> + /*
> + * Now that the required page table and mappings are done, early boot ghcb
> + * page can be setup and used. Check for SNP guest/HV feature compatibility
> + * and terminate the guest providing exit information in boot ghcb.
> + */
How about a more concise comment...
/*
* Now that the required page table mappings are established and a
* GHCB can be used, check for SNP guest/HV feature compatibility.
*/
> + call snp_check_features
> +#endif
> +
> popq %rsi
>
> /*
> diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
> index 62208ec04ca4..0bc3639be1f8 100644
> --- a/arch/x86/boot/compressed/misc.h
> +++ b/arch/x86/boot/compressed/misc.h
> @@ -126,6 +126,7 @@ static inline void console_init(void)
>
> #ifdef CONFIG_AMD_MEM_ENCRYPT
> void sev_enable(struct boot_params *bp);
> +void snp_check_features(void);
> void sev_es_shutdown_ghcb(void);
> extern bool sev_es_check_ghcb_fault(unsigned long address);
> void snp_set_page_private(unsigned long paddr);
> @@ -143,6 +144,7 @@ static inline void sev_enable(struct boot_params *bp)
> if (bp)
> bp->cc_blob_address = 0;
> }
> +static void snp_check_features(void) { }
Unneeded since you're wrapping the call in a #ifdef check.
> static inline void sev_es_shutdown_ghcb(void) { }
> static inline bool sev_es_check_ghcb_fault(unsigned long address)
> {
> diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
> index c93930d5ccbd..a26a5d6949c3 100644
> --- a/arch/x86/boot/compressed/sev.c
> +++ b/arch/x86/boot/compressed/sev.c
> @@ -270,6 +270,65 @@ 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)
Should these be indented one extra space to line up with MSR_AMD64_SNP_VTOM?
> +
> +/*
> + * 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 snp_check_features(void)
> +{
> + u64 unsupported_features;
> +
> + if (!(sev_status & MSR_AMD64_SEV_SNP_ENABLED))
> + return;
> +
> + /*
> + * Terminate the boot if hypervisor has enabled any feature
> + * lacking guest side implementation.
> + */
> + unsupported_features = sev_status & SNP_FEATURES_IMPL_REQ & ~SNP_FEATURES_PRESENT;
> + if (unsupported_features) {
> + u64 exit_info_1 = SVM_VMGEXIT_TERM_REASON(SVM_VMGEXIT_TERM_REASON_SET,
This should be SEV_TERM_SET_GEN (or see below).
> + SVM_VMGEXIT_TERM_SNP_FEAT_UNSUPPORTED);
> +
> + if (!boot_ghcb && !early_setup_ghcb())
> + sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_FEAT_NOT_IMPLEMENTED);
> +
You need to call vc_ghcb_invalidate() before doing any ghcb_set*() calls.
> + ghcb_set_sw_exit_code(boot_ghcb, SVM_VMGEXIT_TERM_REQUEST);
> + ghcb_set_sw_exit_info_1(boot_ghcb, exit_info_1);
> + ghcb_set_sw_exit_info_2(boot_ghcb, unsupported_features);
Add a blank line here.
> + sev_es_wr_ghcb_msr(__pa(boot_ghcb));
> + VMGEXIT();
Add a blank line here.
> + while (true)
> + asm volatile("hlt\n" : : : "memory");
> + }
> +}
> +
> void sev_enable(struct boot_params *bp)
> {
> unsigned int eax, ebx, ecx, edx;
> 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
No, you can't create a new value to the SEV_TERM_SET_GEN without modifying
the GHCB spec. So please use GHCB_SNP_UNSUPPORTED if using the
SEV_TERM_SET_GEN set or else add a new value to be used with the
SEV_TERM_SET_LINUX set.
>
> /* Linux-specific reason codes (used with reason set 1) */
> #define SEV_TERM_SET_LINUX 1
> diff --git a/arch/x86/include/uapi/asm/svm.h b/arch/x86/include/uapi/asm/svm.h
> index f69c168391aa..5bd81adfb114 100644
> --- a/arch/x86/include/uapi/asm/svm.h
> +++ b/arch/x86/include/uapi/asm/svm.h
> @@ -116,6 +116,16 @@
> #define SVM_VMGEXIT_AP_CREATE 1
> #define SVM_VMGEXIT_AP_DESTROY 2
> #define SVM_VMGEXIT_HV_FEATURES 0x8000fffd
> +#define SVM_VMGEXIT_TERM_REQUEST 0x8000fffe
> +#define SVM_VMGEXIT_TERM_REASON_SET 0
> +#define SVM_VMGEXIT_TERM_GENERAL 0
> +#define SVM_VMGEXIT_TERM_SEVES 1
> +#define SVM_VMGEXIT_TERM_SNP_FEAT_UNSUPPORTED 2
This NAE event uses the same reason code set information as the MSR
protocol, so the above 4 definitions are not needed or the definitions in
sev-common.h should be redefined to use these defines, e.g.:
#define SEV_TERM_SET_GEN SVM_VMGEXIT_TERM_REASON_SET
#define GHCB_SEV_ES_GEN_REQ SVM_VMGEXIT_TERM_GENERAL
...
Thanks,
Tom
> +#define SVM_VMGEXIT_TERM_REASON(reason_set, reason_code) \
> + /* SW_EXITINFO1[3:0] */ \
> + (((((u64)reason_set) & 0xf)) | \
> + /* SW_EXITINFO1[11:4] */ \
> + ((((u64)reason_code) & 0xff) << 4))
> #define SVM_VMGEXIT_UNSUPPORTED_EVENT 0x8000ffff
>
> /* Exit code reserved for hypervisor/software use */
Powered by blists - more mailing lists