Re: [PATCH v14 03/13] x86/sev: Add Secure TSC support for SNP guests

["Nikunj A. Dadhania" <nikunj@amd.com>]

On 11/1/2024 9:30 PM, Borislav Petkov wrote:
> 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.

Sure, I will update.

> 
>> 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?

Yes, that is correct. The BP makes the SNP guest requests and initializes 
both the values. These are later used by APs to initialize the VMSA fields
(TSC_SCALE and TSC_OFFSET).

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

Make sense, I will update the comment.

> 
>>  /* #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.

Sure will update.

> 
>> +	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.

Ok, will allocate dynamically.

>>> +	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...

Right, will add snp_msg_free().

> 
>> +	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.

Sure.

> 
>> +		 __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 //

Sure.

> 
>> +	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() ?

kmalloc() does not work at this stage. Moreover, mem_encrypt_setup_arch() does not have
CC_ATTR_GUEST_SEV_SNP check.

Regards
Nikunj