Date:   Mon, 3 Apr 2023 16:41:37 -0500
From:   Tom Lendacky <thomas.lendacky@....com>
To:     Nikunj A Dadhania <nikunj@....com>, linux-kernel@...r.kernel.org,
        x86@...nel.org
Cc:     bp@...en8.de, dionnaglaze@...gle.com, pgonda@...gle.com,
        seanjc@...gle.com, pbonzini@...hat.com, michael.roth@....com,
        ketanch@...k.ac.in
Subject: Re: [PATCH v2 08/11] x86/sev: Add Secure TSC support for SNP guests

On 3/26/23 09:46, Nikunj A Dadhania wrote:
> Add support for Secure TSC in SNP enabled guests. Secure TSC
> allows guest to securely use RDTSC/RDTSCP instructions as the
> parameters being used cannot be changed by hypervisor once the
> guest is launched.
> 
> During the boot-up of the secondary cpus, SecureTSC enabled
> guests need to query TSC info from Security processor (PSP).

s/Security processor (PSP)/AMD Secure Processor/

> This communication channel is encrypted between the security

here as well.

> processor and the guest, hypervisor is just the conduit to

s/hypervisor/the hypervisor/

> deliver the guest messages to the security processor. Each
> message is protected with an AEAD (AES-256 GCM). Use minimal
> GCM library to encrypt/decrypt SNP Guest messages to communicate
> with the PSP.
> 
> Moreover, the hypervisor should not be intercepting RDTSC/RDTSCP
> when Secure TSC is enabled. A #VC exception will be generated if
> the RDTSC/RDTSCP instructions are being intercepted. If this should
> occur and Secure TSC is enabled, terminate guest execution.

This seems like a separate patch.

> 
> Signed-off-by: Nikunj A Dadhania <nikunj@....com>
> ---
>   arch/x86/include/asm/sev-guest.h | 18 +++++++
>   arch/x86/include/asm/sev.h       |  2 +
>   arch/x86/include/asm/svm.h       |  6 ++-
>   arch/x86/kernel/sev-shared.c     |  7 +++
>   arch/x86/kernel/sev.c            | 92 +++++++++++++++++++++++++++++---
>   arch/x86/mm/mem_encrypt_amd.c    |  6 +++
>   include/linux/cc_platform.h      |  8 +++
>   7 files changed, 131 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/include/asm/sev-guest.h b/arch/x86/include/asm/sev-guest.h
> index 834cdae302ad..d5ed041ce06b 100644
> --- a/arch/x86/include/asm/sev-guest.h
> +++ b/arch/x86/include/asm/sev-guest.h
> @@ -37,6 +37,8 @@ enum msg_type {
>   	SNP_MSG_ABSORB_RSP,
>   	SNP_MSG_VMRK_REQ,
>   	SNP_MSG_VMRK_RSP,
> +	SNP_MSG_TSC_INFO_REQ = 17,
> +	SNP_MSG_TSC_INFO_RSP,
>   
>   	SNP_MSG_TYPE_MAX
>   };
> @@ -75,6 +77,22 @@ struct snp_guest_req {
>   	u8 msg_type;
>   };
>   
> +struct snp_tsc_info_req {
> +#define SNP_TSC_INFO_REQ_SZ 128
> +	/* Must be zero filled */
> +	u8 rsvd[SNP_TSC_INFO_REQ_SZ];
> +} __packed;
> +
> +struct snp_tsc_info_resp {
> +	/* Status of TSC_INFO message */
> +	u32 status;
> +	u32 rsvd1;
> +	u64 tsc_scale;
> +	u64 tsc_offset;
> +	u64 tsc_factor;

This should be a u32 ...

> +	u8 rsvd2[96];

Which then makes this 100.

> +} __packed;
> +
>   int snp_send_guest_request(struct snp_guest_dev *dev, struct snp_guest_req *req);
>   bool snp_assign_vmpck(struct snp_guest_dev *dev, int vmpck_id);
>   
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index 36868e21c3e0..d05cbab5e9e0 100644
> --- a/arch/x86/include/asm/sev.h
> +++ b/arch/x86/include/asm/sev.h
> @@ -218,6 +218,7 @@ void snp_set_memory_private(unsigned long vaddr, unsigned int npages);
>   void snp_set_wakeup_secondary_cpu(void);
>   bool snp_init(struct boot_params *bp);
>   void __init __noreturn snp_abort(void);
> +bool __init snp_secure_tsc_prepare(void);
>   int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsigned long *fw_err);
>   #else
>   static inline void sev_es_ist_enter(struct pt_regs *regs) { }
> @@ -238,6 +239,7 @@ static inline void snp_set_memory_private(unsigned long vaddr, unsigned int npag
>   static inline void snp_set_wakeup_secondary_cpu(void) { }
>   static inline bool snp_init(struct boot_params *bp) { return false; }
>   static inline void snp_abort(void) { }
> +static inline bool __init snp_secure_tsc_prepare(void) { return false; }
>   static inline int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input,
>   					  unsigned long *fw_err)
>   {
> diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
> index 770dcf75eaa9..c781f8e0aae4 100644
> --- a/arch/x86/include/asm/svm.h
> +++ b/arch/x86/include/asm/svm.h
> @@ -404,7 +404,9 @@ struct sev_es_save_area {
>   	u8 reserved_0x298[80];
>   	u32 pkru;
>   	u32 tsc_aux;
> -	u8 reserved_0x2f0[24];
> +	u64 tsc_scale;
> +	u64 tsc_offset;
> +	u8 reserved_0x300[8];
>   	u64 rcx;
>   	u64 rdx;
>   	u64 rbx;
> @@ -536,7 +538,7 @@ static inline void __unused_size_checks(void)
>   	BUILD_BUG_RESERVED_OFFSET(sev_es_save_area, 0x1c0);
>   	BUILD_BUG_RESERVED_OFFSET(sev_es_save_area, 0x248);
>   	BUILD_BUG_RESERVED_OFFSET(sev_es_save_area, 0x298);
> -	BUILD_BUG_RESERVED_OFFSET(sev_es_save_area, 0x2f0);
> +	BUILD_BUG_RESERVED_OFFSET(sev_es_save_area, 0x300);
>   	BUILD_BUG_RESERVED_OFFSET(sev_es_save_area, 0x320);
>   	BUILD_BUG_RESERVED_OFFSET(sev_es_save_area, 0x380);
>   	BUILD_BUG_RESERVED_OFFSET(sev_es_save_area, 0x3f0);
> diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
> index 3a5b0c9c4fcc..1c22025b298f 100644
> --- a/arch/x86/kernel/sev-shared.c
> +++ b/arch/x86/kernel/sev-shared.c
> @@ -912,6 +912,13 @@ static enum es_result vc_handle_rdtsc(struct ghcb *ghcb,
>   	bool rdtscp = (exit_code == SVM_EXIT_RDTSCP);
>   	enum es_result ret;
>   
> +	/*
> +	 * RDTSC and RDTSCP should not be intercepted when Secure TSC is
> +	 * enabled. Terminate the SNP guest when the interception is enabled.
> +	 */
> +	if (sev_status & MSR_AMD64_SNP_SECURE_TSC)
> +		return ES_VMM_ERROR;
> +
>   	ret = sev_es_ghcb_hv_call(ghcb, ctxt, exit_code, 0, 0);
>   	if (ret != ES_OK)
>   		return ret;
> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
> index 3750e545d688..280aaa1e6aad 100644
> --- a/arch/x86/kernel/sev.c
> +++ b/arch/x86/kernel/sev.c
> @@ -72,6 +72,10 @@ static struct ghcb *boot_ghcb __section(".data");
>   /* Bitmap of SEV features supported by the hypervisor */
>   static u64 sev_hv_features __ro_after_init;
>   
> +/* Secure TSC values read using TSC_INFO SNP Guest request */
> +static u64 guest_tsc_scale __ro_after_init;
> +static u64 guest_tsc_offset __ro_after_init;
> +
>   /* #VC handler runtime per-CPU data */
>   struct sev_es_runtime_data {
>   	struct ghcb ghcb_page;
> @@ -1107,7 +1111,7 @@ static void *alloc_shared_pages(size_t sz)
>   	return page_address(page);
>   }
>   
> -static int snp_setup_psp_messaging(struct sev_guest_platform_data *pdata)
> +static int __init snp_setup_psp_messaging(struct sev_guest_platform_data *pdata)
>   {
>   	u64 gpa;
>   	int ret;
> @@ -1406,6 +1410,80 @@ bool snp_assign_vmpck(struct snp_guest_dev *dev, int vmpck_id)
>   }
>   EXPORT_SYMBOL_GPL(snp_assign_vmpck);
>   
> +static int __init snp_get_tsc_info(void)
> +{
> +	u8 buf[SNP_TSC_INFO_REQ_SZ + AUTHTAG_LEN];
> +	struct snp_tsc_info_resp tsc_resp = {0};
> +	struct snp_tsc_info_req tsc_req;
> +	struct snp_guest_req req;
> +	struct snp_guest_dev dev;
> +	int rc, resp_len;
> +
> +	/*
> +	 * The intermediate response buffer is used while decrypting the
> +	 * response payload. Make sure that it has enough space to cover the
> +	 * authtag.
> +	 */
> +	resp_len = sizeof(tsc_resp) + AUTHTAG_LEN;
> +	if (sizeof(buf) < resp_len)
> +		return -EINVAL;
> +
> +	/* Zero the tsc_info_req */
> +	memzero_explicit(&tsc_req, sizeof(tsc_req));
> +	memzero_explicit(&req, sizeof(req));
> +
> +	dev.pdata = platform_data;
> +	if (!snp_assign_vmpck(&dev, 0))
> +		return -EINVAL;
> +
> +	req.msg_version = MSG_HDR_VER;
> +	req.msg_type = SNP_MSG_TSC_INFO_REQ;
> +	req.req_buf = &tsc_req;
> +	req.req_sz = sizeof(tsc_req);
> +	req.resp_buf = buf;
> +	req.resp_sz = resp_len;
> +	req.fw_err = NULL;
> +	req.exit_code = SVM_VMGEXIT_GUEST_REQUEST;
> +	rc = snp_send_guest_request(&dev, &req);
> +	if (rc)
> +		goto err_req;
> +
> +	memcpy(&tsc_resp, buf, sizeof(tsc_resp));
> +	pr_debug("%s: Valid response status %x scale %llx offset %llx factor %llx\n",
> +		 __func__, tsc_resp.status, tsc_resp.tsc_scale, tsc_resp.tsc_offset,
> +		 tsc_resp.tsc_factor);
> +
> +	guest_tsc_scale = tsc_resp.tsc_scale;
> +	guest_tsc_offset = tsc_resp.tsc_offset;
> +
> +err_req:
> +	/* The response buffer contains the sensitive data, explicitly clear it. */
> +	memzero_explicit(buf, sizeof(buf));
> +	memzero_explicit(&tsc_resp, sizeof(tsc_resp));
> +	memzero_explicit(&req, sizeof(req));
> +
> +	return rc;
> +}
> +
> +bool __init snp_secure_tsc_prepare(void)
> +{
> +	platform_data = kzalloc(sizeof(*platform_data), GFP_KERNEL);
> +	if (!platform_data)
> +		return false;
> +
> +	/* Initialize the PSP channel to send snp messages */
> +	if (snp_setup_psp_messaging(platform_data))
> +		sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
> +
> +	if (cc_platform_has(CC_ATTR_GUEST_SECURE_TSC)) {

Should this be checked before allocating memory and calling 
snp_setup_psp_messaging()?

Also, I notice here you use the cc_platform_has() function but in previous 
patches you check sev_status directly. And you don't implement support for 
CC_ATTR_GUEST_SECURE_TSC until the last patch instead of now.

You can't get here until SNP_FEATURES_PRESENT is updated.

> +		if (snp_get_tsc_info())
> +			sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
> +
> +		pr_info("SecureTSC enabled\n");
> +	}

Blank line.

Thanks,
Tom

> +	return true;
> +}
> +
>   static int wakeup_cpu_via_vmgexit(int apic_id, unsigned long start_ip)
>   {
>   	struct sev_es_save_area *cur_vmsa, *vmsa;
> @@ -1506,6 +1584,12 @@ static int wakeup_cpu_via_vmgexit(int apic_id, unsigned long start_ip)
>   	vmsa->vmpl		= 0;
>   	vmsa->sev_features	= sev_status >> 2;
>   
> +	/* Setting Secure TSC parameters */
> +	if (cc_platform_has(CC_ATTR_GUEST_SECURE_TSC)) {
> +		vmsa->tsc_scale = guest_tsc_scale;
> +		vmsa->tsc_offset = guest_tsc_offset;
> +	}
> +
>   	/* Switch the page over to a VMSA page now that it is initialized */
>   	ret = snp_set_vmsa(vmsa, true);
>   	if (ret) {
> @@ -2698,11 +2782,7 @@ static int __init snp_init_platform_device(void)
>   	if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
>   		return -ENODEV;
>   
> -	platform_data = kzalloc(sizeof(*platform_data), GFP_KERNEL);
> -	if (!platform_data)
> -		return -ENOMEM;
> -
> -	if (snp_setup_psp_messaging(platform_data))
> +	if (!platform_data->ctx)
>   		return -ENODEV;
>   
>   	if (platform_device_add_data(&sev_guest_device, platform_data, sizeof(*platform_data)))
> diff --git a/arch/x86/mm/mem_encrypt_amd.c b/arch/x86/mm/mem_encrypt_amd.c
> index 9c4d8dbcb129..7d2388e52b8f 100644
> --- a/arch/x86/mm/mem_encrypt_amd.c
> +++ b/arch/x86/mm/mem_encrypt_amd.c
> @@ -215,6 +215,11 @@ void __init sme_map_bootdata(char *real_mode_data)
>   	__sme_early_map_unmap_mem(__va(cmdline_paddr), COMMAND_LINE_SIZE, true);
>   }
>   
> +void __init amd_enc_init(void)
> +{
> +	snp_secure_tsc_prepare();
> +}
> +
>   void __init sev_setup_arch(void)
>   {
>   	phys_addr_t total_mem = memblock_phys_mem_size();
> @@ -501,6 +506,7 @@ void __init sme_early_init(void)
>   	x86_platform.guest.enc_status_change_finish  = amd_enc_status_change_finish;
>   	x86_platform.guest.enc_tlb_flush_required    = amd_enc_tlb_flush_required;
>   	x86_platform.guest.enc_cache_flush_required  = amd_enc_cache_flush_required;
> +	x86_platform.guest.enc_init		     = amd_enc_init;
>   }
>   
>   void __init mem_encrypt_free_decrypted_mem(void)
> diff --git a/include/linux/cc_platform.h b/include/linux/cc_platform.h
> index cb0d6cd1c12f..e081ca4d5da2 100644
> --- a/include/linux/cc_platform.h
> +++ b/include/linux/cc_platform.h
> @@ -90,6 +90,14 @@ enum cc_attr {
>   	 * Examples include TDX Guest.
>   	 */
>   	CC_ATTR_HOTPLUG_DISABLED,
> +
> +	/**
> +	 * @CC_ATTR_GUEST_SECURE_TSC: Secure TSC is active.
> +	 *
> +	 * The platform/OS is running as a guest/virtual machine and actively
> +	 * using AMD SEV-SNP Secure TSC feature.
> +	 */
> +	CC_ATTR_GUEST_SECURE_TSC,
>   };
>   
>   #ifdef CONFIG_ARCH_HAS_CC_PLATFORM

Powered by blists - more mailing lists