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
