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

[Francesco Lavra <francescolavra.fl@gmail.com>]

On 2024-12-03 at 9:00, Nikunj A Dadhania wrote:
> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
> index a61898c7f114..39683101b526 100644
> --- a/arch/x86/coco/sev/core.c
> +++ b/arch/x86/coco/sev/core.c
> @@ -96,6 +96,14 @@ static u64 sev_hv_features __ro_after_init;
>  /* Secrets page physical address from the CC blob */
>  static u64 secrets_pa __ro_after_init;
>  
> +/*
> + * For Secure TSC guests, the BP fetches TSC_INFO using SNP guest
> messaging and
> + * initializes snp_tsc_scale and snp_tsc_offset. These values are
> replicated
> + * across the APs VMSA fields (TSC_SCALE and TSC_OFFSET).
> + */
> +static u64 snp_tsc_scale __ro_after_init;
> +static u64 snp_tsc_offset __ro_after_init;
> +
>  /* #VC handler runtime per-CPU data */
>  struct sev_es_runtime_data {
>  	struct ghcb ghcb_page;
> @@ -1277,6 +1285,12 @@ static int wakeup_cpu_via_vmgexit(u32 apic_id,
> unsigned long start_ip)
>  	vmsa->vmpl		= snp_vmpl;
>  	vmsa->sev_features	= sev_status >> 2;
>  
> +	/* Populate AP's TSC scale/offset to get accurate TSC
> values. */
> +	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) {
> @@ -3127,3 +3141,105 @@ int snp_send_guest_request(struct
> snp_msg_desc *mdesc, struct snp_guest_req *req
>  }
>  EXPORT_SYMBOL_GPL(snp_send_guest_request);
>  
> +static int __init snp_get_tsc_info(void)
> +{
> +	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;
> +	unsigned char *buf;
> +	int rc = -ENOMEM;
> +
> +	tsc_req = kzalloc(sizeof(*tsc_req), GFP_KERNEL);
> +	if (!tsc_req)
> +		return rc;
> +
> +	tsc_resp = kzalloc(sizeof(*tsc_resp), GFP_KERNEL);
> +	if (!tsc_resp)
> +		goto e_free_tsc_req;
> +
> +	req = kzalloc(sizeof(*req), GFP_KERNEL);
> +	if (!req)
> +		goto e_free_tsc_resp;
> +
> +	rio = kzalloc(sizeof(*rio), GFP_KERNEL);
> +	if (!rio)
> +		goto e_free_req;
> +
> +	/*
> +	 * The intermediate response buffer is used while decrypting
> the
> +	 * response payload. Make sure that it has enough space to
> cover
> +	 * the authtag.
> +	 */
> +	buf = kzalloc(SNP_TSC_INFO_RESP_SZ + AUTHTAG_LEN,
> GFP_KERNEL);
> +	if (!buf)
> +		goto e_free_rio;
> +
> +	mdesc = snp_msg_alloc();
> +	if (IS_ERR_OR_NULL(mdesc))
> +		goto e_free_buf;
> +
> +	rc = snp_msg_init(mdesc, snp_vmpl);
> +	if (rc)
> +		goto e_free_mdesc;
> +
> +	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 e_request;
> +
> +	memcpy(tsc_resp, buf, sizeof(*tsc_resp));
> +	pr_debug("%s: response status 0x%x scale 0x%llx offset
> 0x%llx factor 0x%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
> 0x%x\n", tsc_resp->status);
> +		rc = -EIO;
> +	}
> +
> +e_request:
> +	/* The response buffer contains sensitive data, explicitly
> clear it. */
> +	memzero_explicit(buf, sizeof(buf));
> +	memzero_explicit(tsc_resp, sizeof(*tsc_resp));

buf is an unsigned char *, so by using sizeof(buf) you are not zeroing
the entire buffer.
Also, I see no point in having a separate tsc_resp buffer just to copy
the response from buf to tsc_resp, if you just use a single buffer with
size (SNP_TSC_INFO_RESP_SZ + AUTHTAG_LEN) and parse the response from
that buffer you will avoid the double buffer allocation, the memory
copying, and the double zeroing.