Re: [PATCH 7/7] KVM: TDX: Add TSX_CTRL msr into uret_msrs list

[Adrian Hunter]

On 3/12/24 19:34, Edgecombe, Rick P wrote:
> On Mon, 2024-12-02 at 16:34 -0800, Sean Christopherson wrote:
>>> Small point - the last conversation[0] we had on this was to let *userspace*
>>> ensure consistency between KVM's CPUID (i.e. KVM_SET_CPUID2) and the TDX
>>> Module's view.
>>
>> I'm all for that, right up until KVM needs to protect itself again userspace
>> and
>> flawed TDX architecture.  A relevant comment I made in that thread:
>>
>>  : If the upgrade breaks a setup because it confuses _KVM_, then I'll care
>>
>> As it applies here, letting vCPU CPUID and actual guest functionality diverge
>> for
>> features that KVM cares about _will_ cause problems.
> 
> Right, just wanted to make sure we don't need to re-open the major design.
> 
>>
>> This will be less ugly to handle once kvm_vcpu_arch.cpu_caps is a thing.  KVM
>> can simply force set/clear bits to match the actual guest functionality that's
>> hardcoded by the TDX Module or defined by TDPARAMS.
>>
>>> So the configuration goes:
>>> 1. Userspace configures per-VM CPU features
>>> 2. Userspace gets TDX Module's final per-vCPU version of CPUID configuration
>>> via
>>> KVM API
>>> 3. Userspace calls KVM_SET_CPUID2 with the merge of TDX Module's version,
>>> and
>>> userspace's desired values for KVM "owned" CPUID leads (pv features, etc)
>>>
>>> But KVM's knowledge of CPUID bits still remains per-vcpu for TDX in any
>>> case.
>>>
>>>>
>>>>  - Don't hardcode fixed/required CPUID values in KVM, use available
>>>> metadata
>>>>    from TDX Module to reject "bad" guest CPUID (or let the TDX module
>>>> reject?).
>>>>    I.e. don't let a guest silently run with a CPUID that diverges from
>>>> what
>>>>    userspace provided.
>>>
>>> The latest QEMU patches have this fixed bit data hardcoded in QEMU. Then the
>>> long term solution is to make the TDX module return this data. Xiaoyao will
>>> post
>>> a proposal on how the TDX module should expose this soon.
>>
>> Punting the "merge" to userspace is fine, but KVM still needs to ensure it
>> doesn't
>> have holes where userspace can attack the kernel by lying about what features
>> the
>> guest has access to.  And that means forcing bits in kvm_vcpu_arch.cpu_caps;
>> anything else is just asking for problems.
> 
> Ok, then for now let's just address them on a case-by-case basis for logic that
> protects KVM. I'll add to look at using kvm_vcpu_arch.cpu_caps to our future-
> things todo list.
> 
> I think Adrian is going post a proposal for how to handle this case better.

Perhaps just do without TSX support to start with e.g. drop
this "KVM: TDX: Add TSX_CTRL msr into uret_msrs list" patch
and instead add the following:


From: Adrian Hunter <adrian.hunter@xxxxxxxxx>
Date: Tue, 3 Dec 2024 08:20:03 +0200
Subject: [PATCH] KVM: TDX: Disable support for TSX and WAITPKG

Support for restoring IA32_TSX_CTRL MSR and IA32_UMWAIT_CONTROL MSR is not
yet implemented, so disable support for TSX and WAITPKG for now.  Clear the
associated CPUID bits returned by KVM_TDX_CAPABILITIES, and return an error
if those bits are set in KVM_TDX_INIT_VM.

Signed-off-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>
---
 arch/x86/kvm/vmx/tdx.c | 43 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 69bb3136076d..947f78dc3429 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -105,6 +105,44 @@ static u32 tdx_set_guest_phys_addr_bits(const u32 eax, int addr_bits)
 	return (eax & ~GENMASK(23, 16)) | (addr_bits & 0xff) << 16;
 }
 
+#define TDX_FEATURE_TSX (__feature_bit(X86_FEATURE_HLE) | __feature_bit(X86_FEATURE_RTM))
+
+static bool has_tsx(const struct kvm_cpuid_entry2 *entry)
+{
+	return entry->function == 7 && entry->index == 0 &&
+	       (entry->ebx & TDX_FEATURE_TSX);
+}
+
+static void clear_tsx(struct kvm_cpuid_entry2 *entry)
+{
+	entry->ebx &= ~TDX_FEATURE_TSX;
+}
+
+static bool has_waitpkg(const struct kvm_cpuid_entry2 *entry)
+{
+	return entry->function == 7 && entry->index == 0 &&
+	       (entry->ecx & __feature_bit(X86_FEATURE_WAITPKG));
+}
+
+static void clear_waitpkg(struct kvm_cpuid_entry2 *entry)
+{
+	entry->ecx &= ~__feature_bit(X86_FEATURE_WAITPKG);
+}
+
+static void tdx_clear_unsupported_cpuid(struct kvm_cpuid_entry2 *entry)
+{
+	if (has_tsx(entry))
+		clear_tsx(entry);
+
+	if (has_waitpkg(entry))
+		clear_waitpkg(entry);
+}
+
+static bool tdx_unsupported_cpuid(const struct kvm_cpuid_entry2 *entry)
+{
+	return has_tsx(entry) || has_waitpkg(entry);
+}
+
 #define KVM_TDX_CPUID_NO_SUBLEAF	((__u32)-1)
 
 static void td_init_cpuid_entry2(struct kvm_cpuid_entry2 *entry, unsigned char idx)
@@ -124,6 +162,8 @@ static void td_init_cpuid_entry2(struct kvm_cpuid_entry2 *entry, unsigned char i
 	/* Work around missing support on old TDX modules */
 	if (entry->function == 0x80000008)
 		entry->eax = tdx_set_guest_phys_addr_bits(entry->eax, 0xff);
+
+	tdx_clear_unsupported_cpuid(entry);
 }
 
 static int init_kvm_tdx_caps(const struct tdx_sys_info_td_conf *td_conf,
@@ -1235,6 +1275,9 @@ static int setup_tdparams_cpuids(struct kvm_cpuid2 *cpuid,
 		if (!entry)
 			continue;
 
+		if (tdx_unsupported_cpuid(entry))
+			return -EINVAL;
+
 		copy_cnt++;
 
 		value = &td_params->cpuid_values[i];
-- 
2.43.0