Date: Thu, 2 May 2024 10:29:02 -0500
From: Tom Lendacky <thomas.lendacky@....com>
To: Borislav Petkov <bp@...en8.de>
Cc: linux-kernel@...r.kernel.org, x86@...nel.org, linux-coco@...ts.linux.dev,
 svsm-devel@...onut-svsm.dev, Thomas Gleixner <tglx@...utronix.de>,
 Ingo Molnar <mingo@...hat.com>, Dave Hansen <dave.hansen@...ux.intel.com>,
 "H. Peter Anvin" <hpa@...or.com>, Andy Lutomirski <luto@...nel.org>,
 Peter Zijlstra <peterz@...radead.org>,
 Dan Williams <dan.j.williams@...el.com>, Michael Roth
 <michael.roth@....com>, Ashish Kalra <ashish.kalra@....com>
Subject: Re: [PATCH v4 04/15] x86/sev: Check for the presence of an SVSM in
 the SNP Secrets page

On 5/2/24 04:35, Borislav Petkov wrote:
> On Wed, Apr 24, 2024 at 10:58:00AM -0500, Tom Lendacky wrote:
>> During early boot phases, check for the presence of an SVSM when running
>> as an SEV-SNP guest.
>>
>> An SVSM is present if not running at VMPL0 and the 64-bit value at offset
>> 0x148 into the secrets page is non-zero. If an SVSM is present, save the
>> SVSM Calling Area address (CAA), located at offset 0x150 into the secrets
>> page, and set the VMPL level of the guest, which should be non-zero, to
>> indicate the presence of an SVSM.
>>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@....com>
>> ---
>>   .../arch/x86/amd-memory-encryption.rst        | 22 ++++++
>>   arch/x86/boot/compressed/sev.c                |  8 +++
>>   arch/x86/include/asm/sev-common.h             |  4 ++
>>   arch/x86/include/asm/sev.h                    | 25 ++++++-
>>   arch/x86/kernel/sev-shared.c                  | 70 +++++++++++++++++++
>>   arch/x86/kernel/sev.c                         |  7 ++
>>   6 files changed, 135 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/arch/x86/amd-memory-encryption.rst b/Documentation/arch/x86/amd-memory-encryption.rst
>> index 414bc7402ae7..32737718d4a2 100644
>> --- a/Documentation/arch/x86/amd-memory-encryption.rst
>> +++ b/Documentation/arch/x86/amd-memory-encryption.rst
>> @@ -130,4 +130,26 @@ SNP feature support.
>>   
>>   More details in AMD64 APM[1] Vol 2: 15.34.10 SEV_STATUS MSR
>>   
>> +Secure VM Service Module (SVSM)
>> +===============================
>> +
>> +SNP provides a feature called Virtual Machine Privilege Levels (VMPL). The most
>> +privileged VMPL is 0 with numerically higher numbers having lesser privileges.
>> +More details in AMD64 APM[1] Vol 2: 15.35.7 Virtual Machine Privilege Levels.
>> +
>> +The VMPL feature provides the ability to run software services at a more
>> +privileged level than the guest OS is running at. This provides a secure
> 
> Too many "provides".
> 
>> +environment for services within the guest's SNP environment, while protecting
>> +the service from hypervisor interference. An example of a secure service
>> +would be a virtual TPM (vTPM). Additionally, certain operations require the
>> +guest to be running at VMPL0 in order for them to be performed. For example,
>> +the PVALIDATE instruction is required to be executed at VMPL0.
>> +
>> +When a guest is not running at VMPL0, it needs to communicate with the software
>> +running at VMPL0 to perform privileged operations or to interact with secure
>> +services. This software running at VMPL0 is known as a Secure VM Service Module
>> +(SVSM). Discovery of an SVSM and the API used to communicate with it is
>> +documented in Secure VM Service Module for SEV-SNP Guests[2].
> 
> This paragraph needs to go second, not third.
> 
> Somehow that text is missing "restraint" and is all over the place.
> Lemme try to restructure it:
> 
> "SNP provides a feature called Virtual Machine Privilege Levels (VMPL) which
> defines four privilege levels at which guest software can run. The most
> privileged level is 0 and numerically higher numbers have lesser privileges.
> More details in the AMD64 APM[1] Vol 2, section "15.35.7 Virtual Machine
> Privilege Levels", docID: 24593.
> 
> When using that feature, different services can run at different protection
> levels, apart from the guest OS but still within the secure SNP environment.
> They can provide services to the guest, like a vTPM, for example.
> 
> When a guest is not running at VMPL0, it needs to communicate with the software
> running at VMPL0 to perform privileged operations or to interact with secure
> services. An example fur such a privileged operation is PVALIDATE which is
> *required* to be executed at VMPL0.
> 
> In this scenario, the software running at VMPL0 is usually called a Secure VM
> Service Module (SVSM). Discovery of an SVSM and the API used to communicate
> with it is documented in "Secure VM Service Module for SEV-SNP Guests", docID:
> 58019."
> 
> How's that?

Works for me.

> 
>> +
>>   [1] https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/programmer-references/24593.pdf
>> +[2] https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/58019.pdf
> 
> Yeah, about those links - they get stale pretty quickly. I think it suffices to
> explain what the document is and what it is called so that one can find it by
> searching the web. See what I did above.
> 
>> diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
>> index 0457a9d7e515..cb771b380a6b 100644
>> --- a/arch/x86/boot/compressed/sev.c
>> +++ b/arch/x86/boot/compressed/sev.c
>> @@ -12,6 +12,7 @@
>>    */
>>   #include "misc.h"
>>   
>> +#include <linux/mm.h>
> 
> Please do not include a kernel-proper header into the decompresssor.
> Those things are solved by exposing the shared *minimal* functionality
> into

Right, should've known that.

> 
> arch/x86/include/asm/shared/
> 
> There are examples there.
> 
> By the looks of it:
> 
> In file included from arch/x86/boot/compressed/sev.c:130:
> arch/x86/boot/compressed/../../kernel/sev-shared.c: In function ‘setup_svsm_ca’:
> arch/x86/boot/compressed/../../kernel/sev-shared.c:1332:14: warning: implicit declaration of function ‘PAGE_ALIGNED’; did you mean ‘IS_ALIGNED’? [-Wimplicit-function-declaration]
>   1332 |         if (!PAGE_ALIGNED(caa))
>        |              ^~~~~~~~~~~~
>        |              IS_ALIGNED
> 
> it'll need PAGE_ALIGNED and IS_ALIGNED into an arch/x86/include/asm/shared/mm.h
> header.

PAGE_ALIGNED and IS_ALIGNED are from two separate header files (mm.h and 
align.h) which seems like a lot of extra changes for just one check.

Any objection to either adding this define to sev-shared.c on the "else" 
patch of the "#ifndef __BOOT_COMPRESSED" check:

#define PAGE_ALIGNED(x) IS_ALIGNED((x), PAGE_SIZE)

or just changing the above check to:

	if (!IS_ALIGNED(caa, PAGE_SIZE))

> 
>>   #include <asm/bootparam.h>
>>   #include <asm/pgtable_types.h>
>>   #include <asm/sev.h>
> 
> ..
> 
>> +static void __head setup_svsm_ca(const struct cc_blob_sev_info *cc_info)
>> +{
>> +	struct snp_secrets_page *secrets_page;
>> +	u64 caa;
>> +
>> +	BUILD_BUG_ON(sizeof(*secrets_page) != PAGE_SIZE);
>> +
>> +	/*
>> +	 * RMPADJUST modifies RMP permissions of a lesser-privileged (numerically
>> +	 * higher) privilege level. Here, clear the VMPL1 permission mask of the
>> +	 * GHCB page. If the guest is not running at VMPL0, this will fail.
>> +	 *
>> +	 * If the guest is running at VMPL0, it will succeed. Even if that operation
>> +	 * modifies permission bits, it is still ok to do so currently because Linux
>> +	 * SNP guests running at VMPL0 only run at VMPL0, so VMPL1 or higher
>> +	 * permission mask changes are a don't-care.
>> +	 *
>> +	 * Use __pa() since this routine is running identity mapped when called,
>> +	 * both by the decompressor code and the early kernel code.
>> +	 */
> 
> Let's not replicate that comment. Diff ontop:
> 
> diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
> index cb771b380a6b..cde1890c8843 100644
> --- a/arch/x86/boot/compressed/sev.c
> +++ b/arch/x86/boot/compressed/sev.c
> @@ -576,18 +576,7 @@ 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);
>   
> -		/*
> -		 * Enforce running at VMPL0.
> -		 *
> -		 * RMPADJUST modifies RMP permissions of a lesser-privileged (numerically
> -		 * higher) privilege level. Here, clear the VMPL1 permission mask of the
> -		 * GHCB page. If the guest is not running at VMPL0, this will fail.
> -		 *
> -		 * If the guest is running at VMPL0, it will succeed. Even if that operation
> -		 * modifies permission bits, it is still ok to do so currently because Linux
> -		 * SNP guests running at VMPL0 only run at VMPL0, so VMPL1 or higher
> -		 * permission mask changes are a don't-care.
> -		 */
> +		/* Enforce running at VMPL0 - see comment above rmpadjust(). */

Not sure I agree. I'd prefer to keep the comment here because it is 
specific to this rmpadjust() call. See below.

>   		if (rmpadjust((unsigned long)&boot_ghcb_page, RMP_PG_SIZE_4K, 1))
>   			sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_NOT_VMPL0);
>   	}
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index 350db22e66be..b168403c07be 100644
> --- a/arch/x86/include/asm/sev.h
> +++ b/arch/x86/include/asm/sev.h
> @@ -204,6 +204,17 @@ static __always_inline void sev_es_nmi_complete(void)
>   extern int __init sev_es_efi_map_ghcbs(pgd_t *pgd);
>   extern void sev_enable(struct boot_params *bp);
>   
> +/*
> + * RMPADJUST modifies RMP permissions of a lesser-privileged
> + * (numerically higher) privilege level. If @attrs==0, it will attempt
> + * to clear the VMPL1 permission mask of @vaddr. If the guest is not
> + * running at VMPL0, this will fail.
> + *
> + * If the guest is running at VMPL0, it will succeed. Even if that operation
> + * modifies permission bits, it is still ok to do so currently because Linux
> + * SNP guests running at VMPL0 only run at VMPL0, so VMPL1 or higher
> + * permission mask changes are a don't-care.

If you want to put a comment here, then it needs to be more generic. The 
attrs value would be 1 if VMPL0 was attempting to clear VMPL1 
permissions. Also, you could be running at VMPL2 and successfully clear 
or set VMPL3 permissions. So this comment doesn't really flow with a 
generic RMPADJUST function.

/*
  * RMPAJDUST modifies the RMP permissions of a lesser-privileged
  * (numerically higher) VMPL. The @attrs option contains the VMPL
  * level to be modified for @vaddr. The operation will succeed only
  * if the guest is running at a higher-privileged (numerically lower)
  * VMPL.
  */

> + */
>   static inline int rmpadjust(unsigned long vaddr, bool rmp_psize, unsigned long attrs)
>   {
>   	int rc;
> diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
> index 46ea4e5e118a..9ca54bcf0e99 100644
> --- a/arch/x86/kernel/sev-shared.c
> +++ b/arch/x86/kernel/sev-shared.c
> @@ -1297,17 +1297,9 @@ static void __head setup_svsm_ca(const struct cc_blob_sev_info *cc_info)
>   	BUILD_BUG_ON(sizeof(*secrets_page) != PAGE_SIZE);
>   
>   	/*
> -	 * RMPADJUST modifies RMP permissions of a lesser-privileged (numerically
> -	 * higher) privilege level. Here, clear the VMPL1 permission mask of the
> -	 * GHCB page. If the guest is not running at VMPL0, this will fail.
> -	 *
> -	 * If the guest is running at VMPL0, it will succeed. Even if that operation
> -	 * modifies permission bits, it is still ok to do so currently because Linux
> -	 * SNP guests running at VMPL0 only run at VMPL0, so VMPL1 or higher
> -	 * permission mask changes are a don't-care.
> -	 *
> -	 * Use __pa() since this routine is running identity mapped when called,
> -	 * both by the decompressor code and the early kernel code.
> +	 * See comment above rmpadjust() for details. Use __pa() since
> +	 * this routine is running identity mapped when called both by
> +	 * the decompressor code and the early kernel code.
>   	 */
>   	if (!rmpadjust((unsigned long)__pa(&boot_ghcb_page), RMP_PG_SIZE_4K, 1))
>   		return;
> 
>> +	if (!rmpadjust((unsigned long)__pa(&boot_ghcb_page), RMP_PG_SIZE_4K, 1))
>> +		return;
>> +
>> +	/*
>> +	 * Not running at VMPL0, ensure everything has been properly supplied
>> +	 * for running under an SVSM.
>> +	 */
>> +	if (!cc_info || !cc_info->secrets_phys || cc_info->secrets_len != PAGE_SIZE)
>> +		sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_SECRETS_PAGE);
>> +
>> +	secrets_page = (struct snp_secrets_page *)cc_info->secrets_phys;
>> +	if (!secrets_page->svsm_size)
>> +		sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_NO_SVSM);
>> +
>> +	if (!secrets_page->svsm_guest_vmpl)
>> +		sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_SVSM_VMPL0);
> 
> 0x15C	1 byte	SVSM_GUEST_VMPL		Indicates the VMPL at which the guest is executing.
> 
> Do I understand it correctly that this contains the VMPL of the guest and  the
> SVSM is running below it?

Right, the SVSM is supposed to place the VMPL level that it starts the 
guest at in this location.

> 
> IOW, SVSM should be at VMPL0 and the guest should be a at a level determined by
> that value and it cannot be 0.

Right. Not sure about the "cannot", more like "must not." The 
specification states that the guest should run at a VMPL other than 0. 
If an SVSM starts the guest at VMPL0, then the SVSM would not be 
protected from guest.

Thanks,
Tom

> 
> Just making sure I'm reading it right.
> 
> Thx.
> 

Powered by blists - more mailing lists