Message-ID: <20241101160019.GKZyT7E6DrVhCijDAH@fat_crate.local>
Date: Fri, 1 Nov 2024 17:00:19 +0100
From: Borislav Petkov <bp@...en8.de>
To: Nikunj A Dadhania <nikunj@....com>
Cc: linux-kernel@...r.kernel.org, thomas.lendacky@....com, x86@...nel.org,
	kvm@...r.kernel.org, mingo@...hat.com, tglx@...utronix.de,
	dave.hansen@...ux.intel.com, pgonda@...gle.com, seanjc@...gle.com,
	pbonzini@...hat.com
Subject: Re: [PATCH v14 03/13] x86/sev: Add Secure TSC support for SNP guests

On Mon, Oct 28, 2024 at 11:04:21AM +0530, Nikunj A Dadhania wrote:
> Add support for Secure TSC in SNP-enabled guests. Secure TSC allows guests
> to securely use RDTSC/RDTSCP instructions, ensuring that the parameters
> used cannot be altered by the hypervisor once the guest is launched.
> 
> Secure TSC-enabled guests need to query TSC information from the AMD
> Security Processor. This communication channel is encrypted between the AMD
> Security Processor and the guest, with the hypervisor acting merely as a
> conduit to deliver the guest messages to the AMD Security Processor. Each
> message is protected with AEAD (AES-256 GCM).

Zap all that text below or shorten it to the bits only which explain why
something is done the way it is.

> Use a minimal AES GCM library
> to encrypt and decrypt SNP guest messages for communication with the PSP.
> 
> Use mem_encrypt_init() to fetch SNP TSC information from the AMD Security
> Processor and initialize snp_tsc_scale and snp_tsc_offset. During secondary
> CPU initialization, set the VMSA fields GUEST_TSC_SCALE (offset 2F0h) and
> GUEST_TSC_OFFSET (offset 2F8h) with snp_tsc_scale and snp_tsc_offset,
> respectively.
> 
> Add confidential compute platform attribute CC_ATTR_GUEST_SNP_SECURE_TSC
> that can be used by the guest to query whether the Secure TSC feature is
> active.
> 
> Since handle_guest_request() is common routine used by both the SEV guest
> driver and Secure TSC code, move it to the SEV header file.

...

> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
> index c96b742789c5..88cae62382c2 100644
> --- a/arch/x86/coco/sev/core.c
> +++ b/arch/x86/coco/sev/core.c
> @@ -98,6 +98,10 @@ static u64 secrets_pa __ro_after_init;
>  
>  static struct snp_msg_desc *snp_mdesc;
>  
> +/* Secure TSC values read using TSC_INFO SNP Guest request */
> +static u64 snp_tsc_scale __ro_after_init;
> +static u64 snp_tsc_offset __ro_after_init;

I don't understand the point of this: this is supposed to be per VMSA so
everytime you create a guest, that guest is supposed to query the PSP. What
are those for?

Or are those the guest's TSC values which you're supposed to replicate across
the APs?

If so, put that info in the comment above it - it is much more important than
what you have there now.

>  /* #VC handler runtime per-CPU data */
>  struct sev_es_runtime_data {
>  	struct ghcb ghcb_page;
> @@ -1148,6 +1152,12 @@ static int wakeup_cpu_via_vmgexit(u32 apic_id, unsigned long start_ip)
>  	vmsa->vmpl		= snp_vmpl;
>  	vmsa->sev_features	= sev_status >> 2;
>  
> +	/* Set Secure TSC parameters */

That's obvious. Why are you setting them, is more important.

> +	if (cc_platform_has(CC_ATTR_GUEST_SNP_SECURE_TSC)) {
> +		vmsa->tsc_scale = snp_tsc_scale;
> +		vmsa->tsc_offset = snp_tsc_offset;
> +	}
> +
>  	/* Switch the page over to a VMSA page now that it is initialized */
>  	ret = snp_set_vmsa(vmsa, caa, apic_id, true);
>  	if (ret) {
> @@ -2942,3 +2952,83 @@ int snp_send_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_req *req
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(snp_send_guest_request);
> +
> +static int __init snp_get_tsc_info(void)
> +{
> +	static u8 buf[SNP_TSC_INFO_RESP_SZ + AUTHTAG_LEN];

You're allocating stuff below dynamically. Why is this buffer allocated on the
stack?

> +	struct snp_guest_request_ioctl rio;
> +	struct snp_tsc_info_resp tsc_resp;

Ditto.

> +	struct snp_tsc_info_req *tsc_req;
> +	struct snp_msg_desc *mdesc;
> +	struct snp_guest_req req;
> +	int rc;
> +
> +	/*
> +	 * The intermediate response buffer is used while decrypting the
> +	 * response payload. Make sure that it has enough space to cover the
> +	 * authtag.
> +	 */

Yes, this is how you do comments - you comment stuff which is non-obvious.

> +	BUILD_BUG_ON(sizeof(buf) < (sizeof(tsc_resp) + AUTHTAG_LEN));
> +
> +	mdesc = snp_msg_alloc();
> +	if (IS_ERR_OR_NULL(mdesc))
> +		return -ENOMEM;
> +
> +	rc = snp_msg_init(mdesc, snp_vmpl);
> +	if (rc)
> +		return rc;
> +
> +	tsc_req = kzalloc(sizeof(struct snp_tsc_info_req), GFP_KERNEL);
> +	if (!tsc_req)
> +		return -ENOMEM;

You return here and you leak mdesc. Where are those mdesc things even freed?
I see snp_msg_alloc() but not a "free" counterpart...

> +	memset(&req, 0, sizeof(req));
> +	memset(&rio, 0, sizeof(rio));
> +	memset(buf, 0, sizeof(buf));
> +
> +	req.msg_version = MSG_HDR_VER;
> +	req.msg_type = SNP_MSG_TSC_INFO_REQ;
> +	req.vmpck_id = snp_vmpl;
> +	req.req_buf = tsc_req;
> +	req.req_sz = sizeof(*tsc_req);
> +	req.resp_buf = buf;
> +	req.resp_sz = sizeof(tsc_resp) + AUTHTAG_LEN;
> +	req.exit_code = SVM_VMGEXIT_GUEST_REQUEST;
> +
> +	rc = snp_send_guest_request(mdesc, &req, &rio);
> +	if (rc)
> +		goto err_req;
> +
> +	memcpy(&tsc_resp, buf, sizeof(tsc_resp));
> +	pr_debug("%s: response status %x scale %llx offset %llx factor %x\n",

Prefix all hex values with "0x" so that it is unambiguous.

> +		 __func__, tsc_resp.status, tsc_resp.tsc_scale, tsc_resp.tsc_offset,
> +		 tsc_resp.tsc_factor);
> +

	if (!tsc_resp.status)

> +	if (tsc_resp.status == 0) {
> +		snp_tsc_scale = tsc_resp.tsc_scale;
> +		snp_tsc_offset = tsc_resp.tsc_offset;
> +	} else {
> +		pr_err("Failed to get TSC info, response status %x\n", tsc_resp.status);

								Ox

> +		rc = -EIO;
> +	}
> +
> +err_req:
> +	/* The response buffer contains the sensitive data, explicitly clear it. */

s/the //

> +	memzero_explicit(buf, sizeof(buf));
> +	memzero_explicit(&tsc_resp, sizeof(tsc_resp));
> +
> +	return rc;
> +}
> +
> +void __init snp_secure_tsc_prepare(void)
> +{
> +	if (!cc_platform_has(CC_ATTR_GUEST_SNP_SECURE_TSC))
> +		return;
> +
> +	if (snp_get_tsc_info()) {
> +		pr_alert("Unable to retrieve Secure TSC info from ASP\n");
> +		sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_SECURE_TSC);
> +	}
> +
> +	pr_debug("SecureTSC enabled");
> +}
> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
> index 0a120d85d7bb..996ca27f0b72 100644
> --- a/arch/x86/mm/mem_encrypt.c
> +++ b/arch/x86/mm/mem_encrypt.c
> @@ -94,6 +94,10 @@ void __init mem_encrypt_init(void)
>  	/* Call into SWIOTLB to update the SWIOTLB DMA buffers */
>  	swiotlb_update_mem_attributes();
>  
> +	/* Initialize SNP Secure TSC */

Useless comment.

> +	if (cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
> +		snp_secure_tsc_prepare();

Why don't you call this one in the same if-condition in
mem_encrypt_setup_arch() ?

> +
>  	print_mem_encrypt_feature_info();
>  }
>  
> -- 
> 2.34.1
> 

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Powered by blists - more mailing lists