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

Christophe JAILLET
2024-10-21T08:46:06Z

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