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

[Christophe JAILLET <christophe.jaillet@wanadoo.fr>]

Le 21/10/2024 à 07:51, Nikunj A Dadhania a écrit :
> 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). 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.
> 
> Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
> Tested-by: Peter Gonda <pgonda@google.com>
> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---

..

> +static int __init snp_get_tsc_info(void)
> +{
> +	static u8 buf[SNP_TSC_INFO_RESP_SZ + AUTHTAG_LEN];
> +	struct snp_guest_request_ioctl rio;
> +	struct snp_tsc_info_resp tsc_resp;
> +	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.
> +	 */
> +	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;
> +
> +	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",
> +		 __func__, tsc_resp.status, tsc_resp.tsc_scale, tsc_resp.tsc_offset,
> +		 tsc_resp.tsc_factor);
> +
> +	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);
> +		rc = -EIO;
> +	}
> +
> +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));

req does not seem to hold sensitive data.
Is it needed, or maybe should it be tsc_req?

> +
> +	return rc;
> +}

...

CJ