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

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

On 10/21/2024 1:42 PM, Christophe JAILLET wrote:
> Le 21/10/2024 à 07:51, Nikunj A Dadhania a écrit :
> 
> ..
> 
>> +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.

That is correct, I will remove that.

> Is it needed, or maybe should it be tsc_req?

No, and tsc_req is zeroed by the caller and is not updated by AMD
security processor.

Regards
Nikunj