linux-kernel - Re: [PATCH 14/25] KVM: TDX: initialize VM with TDX specific parameters

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <7b256d52843698785e7d13d43c4e7c46e6fa77b5.camel@intel.com>
Date: Wed, 2 Oct 2024 23:39:53 +0000
From: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To: "kvm@...r.kernel.org" <kvm@...r.kernel.org>, "pbonzini@...hat.com"
	<pbonzini@...hat.com>, "seanjc@...gle.com" <seanjc@...gle.com>
CC: "Li, Xiaoyao" <xiaoyao.li@...el.com>, "Yamahata, Isaku"
	<isaku.yamahata@...el.com>, "tony.lindgren@...ux.intel.com"
	<tony.lindgren@...ux.intel.com>, "Huang, Kai" <kai.huang@...el.com>,
	"isaku.yamahata@...il.com" <isaku.yamahata@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 14/25] KVM: TDX: initialize VM with TDX specific
 parameters

Xiaoyao,

On Mon, 2024-08-12 at 15:48 -0700, Rick Edgecombe wrote:
> +static int setup_tdparams_cpuids(struct kvm_cpuid2 *cpuid,
> +				 struct td_params *td_params)
> +{
> +	const struct tdx_sysinfo_td_conf *td_conf = &tdx_sysinfo->td_conf;
> +	const struct kvm_tdx_cpuid_config *c;
> +	const struct kvm_cpuid_entry2 *entry;
> +	struct tdx_cpuid_value *value;
> +	int i;
> +
> +	/*
> +	 * td_params.cpuid_values: The number and the order of cpuid_value must
> +	 * be same to the one of struct tdsysinfo.{num_cpuid_config, cpuid_configs}
> +	 * It's assumed that td_params was zeroed.
> +	 */
> +	for (i = 0; i < td_conf->num_cpuid_config; i++) {
> +		c = &kvm_tdx_caps->cpuid_configs[i];
> +		entry = kvm_find_cpuid_entry2(cpuid->entries, cpuid->nent,
> +					      c->leaf, c->sub_leaf);
> +		if (!entry)
> +			continue;
> +
> +		/*
> +		 * Check the user input value doesn't set any non-configurable
> +		 * bits reported by kvm_tdx_caps.
> +		 */
> +		if ((entry->eax & c->eax) != entry->eax ||
> +		    (entry->ebx & c->ebx) != entry->ebx ||
> +		    (entry->ecx & c->ecx) != entry->ecx ||
> +		    (entry->edx & c->edx) != entry->edx)
> +			return -EINVAL;
> +
> +		value = &td_params->cpuid_values[i];
> +		value->eax = entry->eax;
> +		value->ebx = entry->ebx;
> +		value->ecx = entry->ecx;
> +		value->edx = entry->edx;
> +	}
> +
> +	return 0;
> +}

We agreed to let the TDX module reject CPUID bits that are not supported instead
of having KVM do it. While removing conditional above I found that we actually
still need some filtering.

The problem is that the filtering here only filters bits for leafs that are in
kvm_tdx_caps, the other leafs are just ignored. But we can't pass those other
leafs to the TDX module for it to do verification on because the index they are
supposed to go in is determined by kvm_tdx_caps->cpuid_configs, so there is no
place to pass them.

So KVM still needs to make sure no leafs are provided that are not in
kvm_tdx_caps, otherwise it will accept bits from userspace and ignore them. It
turns out this is already happening because QEMU is not filtering the CPUID
leafs that it passes. After I changed KVM to reject the other leafs, I needed
the following QEMU change to not pass leafs not in tdx caps:

diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
index 29ff7d2f7e..990960ec27 100644
--- a/target/i386/kvm/tdx.c
+++ b/target/i386/kvm/tdx.c
@@ -648,22 +648,29 @@ static struct kvm_tdx_cpuid_config
*tdx_find_cpuid_config(uint32_t leaf, uint32_
 
 static void tdx_filter_cpuid(struct kvm_cpuid2 *cpuids)
 {
-    int i;
-    struct kvm_cpuid_entry2 *e;
+    int i, dest_cnt = 0;
+    struct kvm_cpuid_entry2 *src, *dest;
     struct kvm_tdx_cpuid_config *config;
 
     for (i = 0; i < cpuids->nent; i++) {
-        e = cpuids->entries + i;
-        config = tdx_find_cpuid_config(e->function, e->index);
+        src = cpuids->entries + i;
+        config = tdx_find_cpuid_config(src->function, src->index);
         if (!config) {
             continue;
         }
+        dest = cpuids->entries + dest_cnt;
+
+        dest->function = src->function;
+        dest->index = src->index;
+        dest->flags = src->flags;
+        dest->eax = src->eax & config->eax;
+        dest->ebx = src->ebx & config->ebx;
+        dest->ecx = src->ecx & config->ecx;
+        dest->edx = src->edx & config->edx;
 
-        e->eax &= config->eax;
-        e->ebx &= config->ebx;
-        e->ecx &= config->ecx;
-        e->edx &= config->edx;
+        dest_cnt++;
     }
+    cpuids->nent = dest_cnt;
 }
 
 int tdx_pre_create_vcpu(CPUState *cpu, Error **errp)