From: Xiaoyao Li <xiaoyao.li@intel.com>

Implement an IOCTL to allow userspace to read the CPUID bit values for a
configured TD.

The TDX module doesn't provide the ability to set all CPUID bits. Instead
some are configured indirectly, or have fixed values. But it does allow
for the final resulting CPUID bits to be read. This information will be
useful for userspace to understand the configuration of the TD, and set
KVM's copy via KVM_SET_CPUID2.

To prevent userspace from starting to use features that might not have KVM
support yet, filter the reported values by KVM's support CPUID bits.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
Co-developed-by: Tony Lindgren <tony.lindgren@linux.intel.com>
Signed-off-by: Tony Lindgren <tony.lindgren@linux.intel.com>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
uAPI breakout v2:
 - Improve error path for tdx_vcpu_get_cpuid() (Xu)
 - Drop unused cpuid in struct kvm_tdx (Xu)
 - Rip out cpuid bit filtering
 - Fixup SEAMCALL call sites due to function parameter changes to SEAMCALL
   wrappers (Kai)
 - Add mmu.h for kvm_gfn_direct_bits() (Binbin)
 - Drop unused nr_premapped (Tao)
 - Fix formatting for tdx_vcpu_get_cpuid_leaf() (Tony)
 - Use helpers for phys_addr_bits (Paolo)

uAPI breakout v1:
 - New patch
---
 arch/x86/include/uapi/asm/kvm.h |   1 +
 arch/x86/kvm/vmx/tdx.c          | 167 ++++++++++++++++++++++++++++++++
 arch/x86/kvm/vmx/tdx_arch.h     |   5 +
 arch/x86/kvm/vmx/tdx_errno.h    |   1 +
 4 files changed, 174 insertions(+)

diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
index 2cfec4b42b9d..36fa03376581 100644
--- a/arch/x86/include/uapi/asm/kvm.h
+++ b/arch/x86/include/uapi/asm/kvm.h
@@ -931,6 +931,7 @@ enum kvm_tdx_cmd_id {
 	KVM_TDX_CAPABILITIES = 0,
 	KVM_TDX_INIT_VM,
 	KVM_TDX_INIT_VCPU,
+	KVM_TDX_GET_CPUID,
 
 	KVM_TDX_CMD_NR_MAX,
 };
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 9008db6cf3b4..1feb3307fd70 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -2,6 +2,7 @@
 #include <linux/cpu.h>
 #include <asm/tdx.h>
 #include "capabilities.h"
+#include "mmu.h"
 #include "x86_ops.h"
 #include "tdx.h"
 
@@ -857,6 +858,94 @@ static int __tdx_td_init(struct kvm *kvm, struct td_params *td_params,
 	return ret;
 }
 
+static u64 tdx_td_metadata_field_read(struct kvm_tdx *tdx, u64 field_id,
+				      u64 *data)
+{
+	u64 err;
+
+	err = tdh_mng_rd(tdx->tdr_pa, field_id, data);
+
+	return err;
+}
+
+#define TDX_MD_UNREADABLE_LEAF_MASK	GENMASK(30, 7)
+#define TDX_MD_UNREADABLE_SUBLEAF_MASK	GENMASK(31, 7)
+
+static int tdx_read_cpuid(struct kvm_vcpu *vcpu, u32 leaf, u32 sub_leaf,
+			  bool sub_leaf_set, struct kvm_cpuid_entry2 *out)
+{
+	struct kvm_tdx *kvm_tdx = to_kvm_tdx(vcpu->kvm);
+	u64 field_id = TD_MD_FIELD_ID_CPUID_VALUES;
+	u64 ebx_eax, edx_ecx;
+	u64 err = 0;
+
+	if (sub_leaf & TDX_MD_UNREADABLE_LEAF_MASK ||
+	    sub_leaf_set & TDX_MD_UNREADABLE_SUBLEAF_MASK)
+		return -EINVAL;
+
+	/*
+	 * bit 23:17, REVSERVED: reserved, must be 0;
+	 * bit 16,    LEAF_31: leaf number bit 31;
+	 * bit 15:9,  LEAF_6_0: leaf number bits 6:0, leaf bits 30:7 are
+	 *                      implicitly 0;
+	 * bit 8,     SUBLEAF_NA: sub-leaf not applicable flag;
+	 * bit 7:1,   SUBLEAF_6_0: sub-leaf number bits 6:0. If SUBLEAF_NA is 1,
+	 *                         the SUBLEAF_6_0 is all-1.
+	 *                         sub-leaf bits 31:7 are implicitly 0;
+	 * bit 0,     ELEMENT_I: Element index within field;
+	 */
+	field_id |= ((leaf & 0x80000000) ? 1 : 0) << 16;
+	field_id |= (leaf & 0x7f) << 9;
+	if (sub_leaf_set)
+		field_id |= (sub_leaf & 0x7f) << 1;
+	else
+		field_id |= 0x1fe;
+
+	err = tdx_td_metadata_field_read(kvm_tdx, field_id, &ebx_eax);
+	if (err) //TODO check for specific errors
+		goto err_out;
+
+	out->eax = (u32) ebx_eax;
+	out->ebx = (u32) (ebx_eax >> 32);
+
+	field_id++;
+	err = tdx_td_metadata_field_read(kvm_tdx, field_id, &edx_ecx);
+	/*
+	 * It's weird that reading edx_ecx fails while reading ebx_eax
+	 * succeeded.
+	 */
+	if (WARN_ON_ONCE(err))
+		goto err_out;
+
+	out->ecx = (u32) edx_ecx;
+	out->edx = (u32) (edx_ecx >> 32);
+
+	out->function = leaf;
+	out->index = sub_leaf;
+	out->flags |= sub_leaf_set ? KVM_CPUID_FLAG_SIGNIFCANT_INDEX : 0;
+
+	/*
+	 * Work around missing support on old TDX modules, fetch
+	 * guest maxpa from gfn_direct_bits.
+	 */
+	if (leaf == 0x80000008) {
+		gpa_t gpa_bits = gfn_to_gpa(kvm_gfn_direct_bits(vcpu->kvm));
+		unsigned int g_maxpa = __ffs(gpa_bits) + 1;
+
+		out->eax = tdx_set_guest_phys_addr_bits(out->eax, g_maxpa);
+	}
+
+	return 0;
+
+err_out:
+	out->eax = 0;
+	out->ebx = 0;
+	out->ecx = 0;
+	out->edx = 0;
+
+	return -EIO;
+}
+
 static int tdx_td_init(struct kvm *kvm, struct kvm_tdx_cmd *cmd)
 {
 	struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
@@ -1055,6 +1144,81 @@ static int tdx_td_vcpu_init(struct kvm_vcpu *vcpu, u64 vcpu_rcx)
 	return ret;
 }
 
+/* Sometimes reads multipple subleafs. Return how many enties were written. */
+static int tdx_vcpu_get_cpuid_leaf(struct kvm_vcpu *vcpu, u32 leaf, int max_cnt,
+				   struct kvm_cpuid_entry2 *output_e)
+{
+	int i;
+
+	if (!max_cnt)
+		return 0;
+
+	/* First try without a subleaf */
+	if (!tdx_read_cpuid(vcpu, leaf, 0, false, output_e))
+		return 1;
+
+	/*
+	 * If the try without a subleaf failed, try reading subleafs until
+	 * failure. The TDX module only supports 6 bits of subleaf index.
+	 */
+	for (i = 0; i < 0b111111; i++) {
+		if (i > max_cnt)
+			goto out;
+
+		/* Keep reading subleafs until there is a failure. */
+		if (tdx_read_cpuid(vcpu, leaf, i, true, output_e))
+			return i;
+
+		output_e++;
+	}
+
+out:
+	return i;
+}
+
+static int tdx_vcpu_get_cpuid(struct kvm_vcpu *vcpu, struct kvm_tdx_cmd *cmd)
+{
+	struct kvm_cpuid2 __user *output, *td_cpuid;
+	struct kvm_cpuid_entry2 *output_e;
+	int r = 0, i = 0, leaf;
+
+	output = u64_to_user_ptr(cmd->data);
+	td_cpuid = kzalloc(sizeof(*td_cpuid) +
+			sizeof(output->entries[0]) * KVM_MAX_CPUID_ENTRIES,
+			GFP_KERNEL);
+	if (!td_cpuid)
+		return -ENOMEM;
+
+	for (leaf = 0; leaf <= 0x1f; leaf++) {
+		output_e = &td_cpuid->entries[i];
+		i += tdx_vcpu_get_cpuid_leaf(vcpu, leaf,
+					     KVM_MAX_CPUID_ENTRIES - i - 1,
+					     output_e);
+	}
+
+	for (leaf = 0x80000000; leaf <= 0x80000008; leaf++) {
+		output_e = &td_cpuid->entries[i];
+		i += tdx_vcpu_get_cpuid_leaf(vcpu, leaf,
+					     KVM_MAX_CPUID_ENTRIES - i - 1,
+					     output_e);
+	}
+
+	td_cpuid->nent = i;
+
+	if (copy_to_user(output, td_cpuid, sizeof(*output))) {
+		r = -EFAULT;
+		goto out;
+	}
+	if (copy_to_user(output->entries, td_cpuid->entries,
+			 td_cpuid->nent * sizeof(struct kvm_cpuid_entry2)))
+		r = -EFAULT;
+
+out:
+	kfree(td_cpuid);
+
+	return r;
+}
+
 static int tdx_vcpu_init(struct kvm_vcpu *vcpu, struct kvm_tdx_cmd *cmd)
 {
 	struct msr_data apic_base_msr;
@@ -1108,6 +1272,9 @@ int tdx_vcpu_ioctl(struct kvm_vcpu *vcpu, void __user *argp)
 	case KVM_TDX_INIT_VCPU:
 		ret = tdx_vcpu_init(vcpu, &cmd);
 		break;
+	case KVM_TDX_GET_CPUID:
+		ret = tdx_vcpu_get_cpuid(vcpu, &cmd);
+		break;
 	default:
 		ret = -EINVAL;
 		break;
diff --git a/arch/x86/kvm/vmx/tdx_arch.h b/arch/x86/kvm/vmx/tdx_arch.h
index 9d41699e66a2..d80ec118834e 100644
--- a/arch/x86/kvm/vmx/tdx_arch.h
+++ b/arch/x86/kvm/vmx/tdx_arch.h
@@ -157,4 +157,9 @@ struct td_params {
 
 #define MD_FIELD_ID_FEATURES0_TOPOLOGY_ENUM	BIT_ULL(20)
 
+/*
+ * TD scope metadata field ID.
+ */
+#define TD_MD_FIELD_ID_CPUID_VALUES		0x9410000300000000ULL
+
 #endif /* __KVM_X86_TDX_ARCH_H */
diff --git a/arch/x86/kvm/vmx/tdx_errno.h b/arch/x86/kvm/vmx/tdx_errno.h
index dc3fa2a58c2c..f9dbb3a065cc 100644
--- a/arch/x86/kvm/vmx/tdx_errno.h
+++ b/arch/x86/kvm/vmx/tdx_errno.h
@@ -23,6 +23,7 @@
 #define TDX_FLUSHVP_NOT_DONE			0x8000082400000000ULL
 #define TDX_EPT_WALK_FAILED			0xC0000B0000000000ULL
 #define TDX_EPT_ENTRY_STATE_INCORRECT		0xC0000B0D00000000ULL
+#define TDX_METADATA_FIELD_NOT_READABLE		0xC0000C0200000000ULL
 
 /*
  * TDX module operand ID, appears in 31:0 part of error code as
-- 
2.47.0

On 10/31/2024 3:00 AM, Rick Edgecombe wrote:
> From: Xiaoyao Li <xiaoyao.li@intel.com>
> 
> Implement an IOCTL to allow userspace to read the CPUID bit values for a
> configured TD.
> 
> The TDX module doesn't provide the ability to set all CPUID bits. Instead
> some are configured indirectly, or have fixed values. But it does allow
> for the final resulting CPUID bits to be read. This information will be
> useful for userspace to understand the configuration of the TD, and set
> KVM's copy via KVM_SET_CPUID2.
> 
> To prevent userspace from starting to use features that might not have KVM
> support yet, filter the reported values by KVM's support CPUID bits.

This sentence is not implemented, we need drop it.

> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> Co-developed-by: Tony Lindgren <tony.lindgren@linux.intel.com>
> Signed-off-by: Tony Lindgren <tony.lindgren@linux.intel.com>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> ---
> uAPI breakout v2:
>   - Improve error path for tdx_vcpu_get_cpuid() (Xu)
>   - Drop unused cpuid in struct kvm_tdx (Xu)
>   - Rip out cpuid bit filtering
>   - Fixup SEAMCALL call sites due to function parameter changes to SEAMCALL
>     wrappers (Kai)
>   - Add mmu.h for kvm_gfn_direct_bits() (Binbin)
>   - Drop unused nr_premapped (Tao)
>   - Fix formatting for tdx_vcpu_get_cpuid_leaf() (Tony)
>   - Use helpers for phys_addr_bits (Paolo)
> 
> uAPI breakout v1:
>   - New patch
> ---
>   arch/x86/include/uapi/asm/kvm.h |   1 +
>   arch/x86/kvm/vmx/tdx.c          | 167 ++++++++++++++++++++++++++++++++
>   arch/x86/kvm/vmx/tdx_arch.h     |   5 +
>   arch/x86/kvm/vmx/tdx_errno.h    |   1 +
>   4 files changed, 174 insertions(+)
> 
> diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
> index 2cfec4b42b9d..36fa03376581 100644
> --- a/arch/x86/include/uapi/asm/kvm.h
> +++ b/arch/x86/include/uapi/asm/kvm.h
> @@ -931,6 +931,7 @@ enum kvm_tdx_cmd_id {
>   	KVM_TDX_CAPABILITIES = 0,
>   	KVM_TDX_INIT_VM,
>   	KVM_TDX_INIT_VCPU,
> +	KVM_TDX_GET_CPUID,
>   
>   	KVM_TDX_CMD_NR_MAX,
>   };
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index 9008db6cf3b4..1feb3307fd70 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -2,6 +2,7 @@
>   #include <linux/cpu.h>
>   #include <asm/tdx.h>
>   #include "capabilities.h"
> +#include "mmu.h"
>   #include "x86_ops.h"
>   #include "tdx.h"
>   
> @@ -857,6 +858,94 @@ static int __tdx_td_init(struct kvm *kvm, struct td_params *td_params,
>   	return ret;
>   }
>   
> +static u64 tdx_td_metadata_field_read(struct kvm_tdx *tdx, u64 field_id,
> +				      u64 *data)
> +{
> +	u64 err;
> +
> +	err = tdh_mng_rd(tdx->tdr_pa, field_id, data);
> +
> +	return err;
> +}
> +
> +#define TDX_MD_UNREADABLE_LEAF_MASK	GENMASK(30, 7)
> +#define TDX_MD_UNREADABLE_SUBLEAF_MASK	GENMASK(31, 7)
> +
> +static int tdx_read_cpuid(struct kvm_vcpu *vcpu, u32 leaf, u32 sub_leaf,
> +			  bool sub_leaf_set, struct kvm_cpuid_entry2 *out)
> +{
> +	struct kvm_tdx *kvm_tdx = to_kvm_tdx(vcpu->kvm);
> +	u64 field_id = TD_MD_FIELD_ID_CPUID_VALUES;
> +	u64 ebx_eax, edx_ecx;
> +	u64 err = 0;
> +
> +	if (sub_leaf & TDX_MD_UNREADABLE_LEAF_MASK ||
> +	    sub_leaf_set & TDX_MD_UNREADABLE_SUBLEAF_MASK)
> +		return -EINVAL;
> +
> +	/*
> +	 * bit 23:17, REVSERVED: reserved, must be 0;
> +	 * bit 16,    LEAF_31: leaf number bit 31;
> +	 * bit 15:9,  LEAF_6_0: leaf number bits 6:0, leaf bits 30:7 are
> +	 *                      implicitly 0;
> +	 * bit 8,     SUBLEAF_NA: sub-leaf not applicable flag;
> +	 * bit 7:1,   SUBLEAF_6_0: sub-leaf number bits 6:0. If SUBLEAF_NA is 1,
> +	 *                         the SUBLEAF_6_0 is all-1.
> +	 *                         sub-leaf bits 31:7 are implicitly 0;
> +	 * bit 0,     ELEMENT_I: Element index within field;
> +	 */
> +	field_id |= ((leaf & 0x80000000) ? 1 : 0) << 16;
> +	field_id |= (leaf & 0x7f) << 9;
> +	if (sub_leaf_set)
> +		field_id |= (sub_leaf & 0x7f) << 1;
> +	else
> +		field_id |= 0x1fe;
> +
> +	err = tdx_td_metadata_field_read(kvm_tdx, field_id, &ebx_eax);
> +	if (err) //TODO check for specific errors
> +		goto err_out;
> +
> +	out->eax = (u32) ebx_eax;
> +	out->ebx = (u32) (ebx_eax >> 32);
> +
> +	field_id++;
> +	err = tdx_td_metadata_field_read(kvm_tdx, field_id, &edx_ecx);
> +	/*
> +	 * It's weird that reading edx_ecx fails while reading ebx_eax
> +	 * succeeded.
> +	 */
> +	if (WARN_ON_ONCE(err))
> +		goto err_out;
> +
> +	out->ecx = (u32) edx_ecx;
> +	out->edx = (u32) (edx_ecx >> 32);
> +
> +	out->function = leaf;
> +	out->index = sub_leaf;
> +	out->flags |= sub_leaf_set ? KVM_CPUID_FLAG_SIGNIFCANT_INDEX : 0;
> +
> +	/*
> +	 * Work around missing support on old TDX modules, fetch
> +	 * guest maxpa from gfn_direct_bits.
> +	 */
> +	if (leaf == 0x80000008) {
> +		gpa_t gpa_bits = gfn_to_gpa(kvm_gfn_direct_bits(vcpu->kvm));
> +		unsigned int g_maxpa = __ffs(gpa_bits) + 1;
> +
> +		out->eax = tdx_set_guest_phys_addr_bits(out->eax, g_maxpa);
> +	}
> +
> +	return 0;
> +
> +err_out:
> +	out->eax = 0;
> +	out->ebx = 0;
> +	out->ecx = 0;
> +	out->edx = 0;
> +
> +	return -EIO;
> +}
> +
>   static int tdx_td_init(struct kvm *kvm, struct kvm_tdx_cmd *cmd)
>   {
>   	struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
> @@ -1055,6 +1144,81 @@ static int tdx_td_vcpu_init(struct kvm_vcpu *vcpu, u64 vcpu_rcx)
>   	return ret;
>   }
>   
> +/* Sometimes reads multipple subleafs. Return how many enties were written. */
> +static int tdx_vcpu_get_cpuid_leaf(struct kvm_vcpu *vcpu, u32 leaf, int max_cnt,
> +				   struct kvm_cpuid_entry2 *output_e)
> +{
> +	int i;
> +
> +	if (!max_cnt)
> +		return 0;
> +
> +	/* First try without a subleaf */
> +	if (!tdx_read_cpuid(vcpu, leaf, 0, false, output_e))
> +		return 1;
> +
> +	/*
> +	 * If the try without a subleaf failed, try reading subleafs until
> +	 * failure. The TDX module only supports 6 bits of subleaf index.
> +	 */
> +	for (i = 0; i < 0b111111; i++) {
> +		if (i > max_cnt)
> +			goto out;
> +
> +		/* Keep reading subleafs until there is a failure. */
> +		if (tdx_read_cpuid(vcpu, leaf, i, true, output_e))
> +			return i;
> +
> +		output_e++;
> +	}
> +
> +out:
> +	return i;
> +}
> +
> +static int tdx_vcpu_get_cpuid(struct kvm_vcpu *vcpu, struct kvm_tdx_cmd *cmd)
> +{
> +	struct kvm_cpuid2 __user *output, *td_cpuid;
> +	struct kvm_cpuid_entry2 *output_e;
> +	int r = 0, i = 0, leaf;
> +
> +	output = u64_to_user_ptr(cmd->data);
> +	td_cpuid = kzalloc(sizeof(*td_cpuid) +
> +			sizeof(output->entries[0]) * KVM_MAX_CPUID_ENTRIES,
> +			GFP_KERNEL);
> +	if (!td_cpuid)
> +		return -ENOMEM;
> +
> +	for (leaf = 0; leaf <= 0x1f; leaf++) {

0x1f needs clarification here.

If it's going to use the maximum leaf KVM can support, it should be 0x24 
to align with __do_cpuid_func().

alternatively, it can use the EAX value of leaf 0 returned by TDX 
module. That is the value TDX module presents to the TD guest.

> +		output_e = &td_cpuid->entries[i];
> +		i += tdx_vcpu_get_cpuid_leaf(vcpu, leaf,
> +					     KVM_MAX_CPUID_ENTRIES - i - 1,
> +					     output_e);
> +	}
> +
> +	for (leaf = 0x80000000; leaf <= 0x80000008; leaf++) {
> +		output_e = &td_cpuid->entries[i];
> +		i += tdx_vcpu_get_cpuid_leaf(vcpu, leaf,
> +					     KVM_MAX_CPUID_ENTRIES - i - 1,
> +					     output_e);

Though what gets passed in for max_cnt is

   KVM_MAX_CPUID_ENTRIES - i - 1

tdx_vcpu_get_cpuid_leaf() can return "max_cnt+1", i.e., 
KVM_MAX_CPUID_ENTRIES - i.

Then, it makes next round i to be KVM_MAX_CPUID_ENTRIES, and

   output_e = &td_cpuid->entries[i];

will overflow the buffer and access illegal memory.

Similar issue inside tdx_vcpu_get_cpuid_leaf() as I replied in [*]

[*] 
https://lore.kernel.org/all/7574968a-f0e2-49d5-b740-2454a0f70bb6@intel.com/

> +	}
> +
> +	td_cpuid->nent = i;
> +
> +	if (copy_to_user(output, td_cpuid, sizeof(*output))) {
> +		r = -EFAULT;
> +		goto out;
> +	}
> +	if (copy_to_user(output->entries, td_cpuid->entries,
> +			 td_cpuid->nent * sizeof(struct kvm_cpuid_entry2)))
> +		r = -EFAULT;
> +
> +out:
> +	kfree(td_cpuid);
> +
> +	return r;
> +}
> +
>   static int tdx_vcpu_init(struct kvm_vcpu *vcpu, struct kvm_tdx_cmd *cmd)
>   {
>   	struct msr_data apic_base_msr;
> @@ -1108,6 +1272,9 @@ int tdx_vcpu_ioctl(struct kvm_vcpu *vcpu, void __user *argp)
>   	case KVM_TDX_INIT_VCPU:
>   		ret = tdx_vcpu_init(vcpu, &cmd);
>   		break;
> +	case KVM_TDX_GET_CPUID:
> +		ret = tdx_vcpu_get_cpuid(vcpu, &cmd);
> +		break;
>   	default:
>   		ret = -EINVAL;
>   		break;
> diff --git a/arch/x86/kvm/vmx/tdx_arch.h b/arch/x86/kvm/vmx/tdx_arch.h
> index 9d41699e66a2..d80ec118834e 100644
> --- a/arch/x86/kvm/vmx/tdx_arch.h
> +++ b/arch/x86/kvm/vmx/tdx_arch.h
> @@ -157,4 +157,9 @@ struct td_params {
>   
>   #define MD_FIELD_ID_FEATURES0_TOPOLOGY_ENUM	BIT_ULL(20)
>   
> +/*
> + * TD scope metadata field ID.
> + */
> +#define TD_MD_FIELD_ID_CPUID_VALUES		0x9410000300000000ULL
> +
>   #endif /* __KVM_X86_TDX_ARCH_H */
> diff --git a/arch/x86/kvm/vmx/tdx_errno.h b/arch/x86/kvm/vmx/tdx_errno.h
> index dc3fa2a58c2c..f9dbb3a065cc 100644
> --- a/arch/x86/kvm/vmx/tdx_errno.h
> +++ b/arch/x86/kvm/vmx/tdx_errno.h
> @@ -23,6 +23,7 @@
>   #define TDX_FLUSHVP_NOT_DONE			0x8000082400000000ULL
>   #define TDX_EPT_WALK_FAILED			0xC0000B0000000000ULL
>   #define TDX_EPT_ENTRY_STATE_INCORRECT		0xC0000B0D00000000ULL
> +#define TDX_METADATA_FIELD_NOT_READABLE		0xC0000C0200000000ULL
>   
>   /*
>    * TDX module operand ID, appears in 31:0 part of error code as

On Fri, 2025-01-10 at 12:47 +0800, Xiaoyao Li wrote:
> > +		output_e = &td_cpuid->entries[i];
> > +		i += tdx_vcpu_get_cpuid_leaf(vcpu, leaf,
> > +					     KVM_MAX_CPUID_ENTRIES - i - 1,
> > +					     output_e);
> > +	}
> > +
> > +	for (leaf = 0x80000000; leaf <= 0x80000008; leaf++) {
> > +		output_e = &td_cpuid->entries[i];
> > +		i += tdx_vcpu_get_cpuid_leaf(vcpu, leaf,
> > +					     KVM_MAX_CPUID_ENTRIES - i - 1,
> > +					     output_e);
> 
> Though what gets passed in for max_cnt is
> 
>    KVM_MAX_CPUID_ENTRIES - i - 1
> 
> tdx_vcpu_get_cpuid_leaf() can return "max_cnt+1", i.e., 
> KVM_MAX_CPUID_ENTRIES - i.
> 
> Then, it makes next round i to be KVM_MAX_CPUID_ENTRIES, and
> 
>    output_e = &td_cpuid->entries[i];
> 
> will overflow the buffer and access illegal memory.
> 
> Similar issue inside tdx_vcpu_get_cpuid_leaf() as I replied in [*]
> 
> [*] 
> https://lore.kernel.org/all/7574968a-f0e2-49d5-b740-2454a0f70bb6@intel.com/

Per Francesco's comment in the other thread, I'm not sure there is an off-by-one
bug here. But in any case the code is too sensitive to issues like that.

In line with Francesco's other comment to move the subleaf checking into
tdx_read_cpuid(), I just changed it to pass around the real index and check
KVM_MAX_CPUID_ENTRIES in tdx_read_cpuid() too. It seems less elegant but easier
to read.

On Fri, 2025-01-10 at 12:47 +0800, Xiaoyao Li wrote:
> 0x1f needs clarification here.
> 
> If it's going to use the maximum leaf KVM can support, it should be 0x24 
> to align with __do_cpuid_func().
> 
> alternatively, it can use the EAX value of leaf 0 returned by TDX 
> module. That is the value TDX module presents to the TD guest.
> 
> > +		output_e = &td_cpuid->entries[i];
> > +		i += tdx_vcpu_get_cpuid_leaf(vcpu, leaf,
> > +					     KVM_MAX_CPUID_ENTRIES - i - 1,
> > +					     output_e);
> > +	}
> > +
> > +	for (leaf = 0x80000000; leaf <= 0x80000008; leaf++) {
> > +		output_e = &td_cpuid->entries[i];
> > +		i += tdx_vcpu_get_cpuid_leaf(vcpu, leaf,
> > +					     KVM_MAX_CPUID_ENTRIES - i - 1,
> > +					     output_e);

Since we are not filtering by KVM supported features anymore, maybe just use the
max leaf for the host CPU, like:

@@ -2790,14 +2791,14 @@ static int tdx_vcpu_get_cpuid(struct kvm_vcpu *vcpu,
struct kvm_tdx_cmd *cmd)
        if (!td_cpuid)
                return -ENOMEM;
 
-       for (leaf = 0; leaf <= 0x1f; leaf++) {
+       for (leaf = 0; leaf <= boot_cpu_data.cpuid_level; leaf++) {
                output_e = &td_cpuid->entries[i];
                i += tdx_vcpu_get_cpuid_leaf(vcpu, leaf,
                                             KVM_MAX_CPUID_ENTRIES - i - 1,
                                             output_e);
        }
 
-       for (leaf = 0x80000000; leaf <= 0x80000008; leaf++) {
+       for (leaf = 0x80000000; leaf <= boot_cpu_data.extended_cpuid_level;
leaf++) {
                output_e = &td_cpuid->entries[i];
                i += tdx_vcpu_get_cpuid_leaf(vcpu, leaf,
                                             KVM_MAX_CPUID_ENTRIES - i - 1,

On 1/22/2025 4:24 AM, Edgecombe, Rick P wrote:
> On Fri, 2025-01-10 at 12:47 +0800, Xiaoyao Li wrote:
>> 0x1f needs clarification here.
>>
>> If it's going to use the maximum leaf KVM can support, it should be 0x24
>> to align with __do_cpuid_func().
>>
>> alternatively, it can use the EAX value of leaf 0 returned by TDX
>> module. That is the value TDX module presents to the TD guest.
>>
>>> +		output_e = &td_cpuid->entries[i];
>>> +		i += tdx_vcpu_get_cpuid_leaf(vcpu, leaf,
>>> +					     KVM_MAX_CPUID_ENTRIES - i - 1,
>>> +					     output_e);
>>> +	}
>>> +
>>> +	for (leaf = 0x80000000; leaf <= 0x80000008; leaf++) {
>>> +		output_e = &td_cpuid->entries[i];
>>> +		i += tdx_vcpu_get_cpuid_leaf(vcpu, leaf,
>>> +					     KVM_MAX_CPUID_ENTRIES - i - 1,
>>> +					     output_e);
> 
> Since we are not filtering by KVM supported features anymore, maybe just use the
> max leaf for the host CPU, like:

host value is not matched with the value returned by TDX module.
I.e., On my SPR machine, the boot_cpu_data.cpuid_level is 0x20, while 
TDX module returns 0x23. It at least fails to report the leaf 0x21 to 
userspace, which is a always valid leaf for TD guest.

> @@ -2790,14 +2791,14 @@ static int tdx_vcpu_get_cpuid(struct kvm_vcpu *vcpu,
> struct kvm_tdx_cmd *cmd)
>          if (!td_cpuid)
>                  return -ENOMEM;
>   
> -       for (leaf = 0; leaf <= 0x1f; leaf++) {
> +       for (leaf = 0; leaf <= boot_cpu_data.cpuid_level; leaf++) {
>                  output_e = &td_cpuid->entries[i];
>                  i += tdx_vcpu_get_cpuid_leaf(vcpu, leaf,
>                                               KVM_MAX_CPUID_ENTRIES - i - 1,
>                                               output_e);
>          }
>   
> -       for (leaf = 0x80000000; leaf <= 0x80000008; leaf++) {
> +       for (leaf = 0x80000000; leaf <= boot_cpu_data.extended_cpuid_level;
> leaf++) {
>                  output_e = &td_cpuid->entries[i];
>                  i += tdx_vcpu_get_cpuid_leaf(vcpu, leaf,
>                                               KVM_MAX_CPUID_ENTRIES - i - 1,
> 

On Wed, 2025-01-22 at 15:43 +0800, Xiaoyao Li wrote:
> > Since we are not filtering by KVM supported features anymore, maybe just use
> > the
> > max leaf for the host CPU, like:
> 
> host value is not matched with the value returned by TDX module.
> I.e., On my SPR machine, the boot_cpu_data.cpuid_level is 0x20, while 
> TDX module returns 0x23. It at least fails to report the leaf 0x21 to 
> userspace, which is a always valid leaf for TD guest.

Good point, we can use the cpuid level read from the TD.

On 2024-10-30 at 19:00, Rick Edgecombe wrote:
> @@ -1055,6 +1144,81 @@ static int tdx_td_vcpu_init(struct kvm_vcpu
> *vcpu, u64 vcpu_rcx)
>  	return ret;
>  }
>  
> +/* Sometimes reads multipple subleafs. Return how many enties were
> written. */
> +static int tdx_vcpu_get_cpuid_leaf(struct kvm_vcpu *vcpu, u32 leaf,
> int max_cnt,
> +				   struct kvm_cpuid_entry2
> *output_e)
> +{
> +	int i;
> +
> +	if (!max_cnt)
> +		return 0;
> +
> +	/* First try without a subleaf */
> +	if (!tdx_read_cpuid(vcpu, leaf, 0, false, output_e))
> +		return 1;
> +
> +	/*
> +	 * If the try without a subleaf failed, try reading subleafs
> until
> +	 * failure. The TDX module only supports 6 bits of subleaf
> index.

It actually supports 7 bits, i.e. bits 6:0, so the limit below should
be 0b1111111.

> +	 */
> +	for (i = 0; i < 0b111111; i++) {
> +		if (i > max_cnt)
> +			goto out;

This will make this function return (max_cnt + 1) instead of max_cnt.
I think the code would be simpler if max_cnt was initialized to
min(max_cnt, 0x80) (because 0x7f is a supported subleaf index, as far
as I can tell), and the for() condition was changed to `i < max_cnt`.

> +		/* Keep reading subleafs until there is a failure.
> */
> +		if (tdx_read_cpuid(vcpu, leaf, i, true, output_e))
> +			return i;
> +
> +		output_e++;
> +	}
> +
> +out:
> +	return i;
> +}
> +
> +static int tdx_vcpu_get_cpuid(struct kvm_vcpu *vcpu, struct
> kvm_tdx_cmd *cmd)
> +{
> +	struct kvm_cpuid2 __user *output, *td_cpuid;
> +	struct kvm_cpuid_entry2 *output_e;
> +	int r = 0, i = 0, leaf;
> +
> +	output = u64_to_user_ptr(cmd->data);
> +	td_cpuid = kzalloc(sizeof(*td_cpuid) +
> +			sizeof(output->entries[0]) *
> KVM_MAX_CPUID_ENTRIES,
> +			GFP_KERNEL);
> +	if (!td_cpuid)
> +		return -ENOMEM;
> +
> +	for (leaf = 0; leaf <= 0x1f; leaf++) {
> +		output_e = &td_cpuid->entries[i];
> +		i += tdx_vcpu_get_cpuid_leaf(vcpu, leaf,
> +					     KVM_MAX_CPUID_ENTRIES -
> i - 1,

This should be KVM_MAX_CPUID_ENTRIES - i.

> +					     output_e);
> +	}
> +
> +	for (leaf = 0x80000000; leaf <= 0x80000008; leaf++) {
> +		output_e = &td_cpuid->entries[i];
> +		i += tdx_vcpu_get_cpuid_leaf(vcpu, leaf,
> +					     KVM_MAX_CPUID_ENTRIES -
> i - 1,

Same here.

On 1/9/2025 7:07 PM, Francesco Lavra wrote:
> On 2024-10-30 at 19:00, Rick Edgecombe wrote:
>> @@ -1055,6 +1144,81 @@ static int tdx_td_vcpu_init(struct kvm_vcpu
>> *vcpu, u64 vcpu_rcx)
>>   	return ret;
>>   }
>>   
>> +/* Sometimes reads multipple subleafs. Return how many enties were
>> written. */
>> +static int tdx_vcpu_get_cpuid_leaf(struct kvm_vcpu *vcpu, u32 leaf,
>> int max_cnt,
>> +				   struct kvm_cpuid_entry2
>> *output_e)
>> +{
>> +	int i;
>> +
>> +	if (!max_cnt)
>> +		return 0;
>> +
>> +	/* First try without a subleaf */
>> +	if (!tdx_read_cpuid(vcpu, leaf, 0, false, output_e))
>> +		return 1;
>> +
>> +	/*
>> +	 * If the try without a subleaf failed, try reading subleafs
>> until
>> +	 * failure. The TDX module only supports 6 bits of subleaf
>> index.
> 
> It actually supports 7 bits, i.e. bits 6:0, so the limit below should
> be 0b1111111.

Nice catch!

>> +	 */
>> +	for (i = 0; i < 0b111111; i++) {
>> +		if (i > max_cnt)
>> +			goto out;
> 
> This will make this function return (max_cnt + 1) instead of max_cnt.
> I think the code would be simpler if max_cnt was initialized to
> min(max_cnt, 0x80) (because 0x7f is a supported subleaf index, as far
> as I can tell), and the for() condition was changed to `i < max_cnt`.

Looks better.

>> +		/* Keep reading subleafs until there is a failure.
>> */
>> +		if (tdx_read_cpuid(vcpu, leaf, i, true, output_e))
>> +			return i;
>> +
>> +		output_e++;

here the output_e++ can overflow the buffer.

>> +	}
>> +
>> +out:
>> +	return i;
>> +}
>> +
>> +static int tdx_vcpu_get_cpuid(struct kvm_vcpu *vcpu, struct
>> kvm_tdx_cmd *cmd)
>> +{
>> +	struct kvm_cpuid2 __user *output, *td_cpuid;
>> +	struct kvm_cpuid_entry2 *output_e;
>> +	int r = 0, i = 0, leaf;
>> +
>> +	output = u64_to_user_ptr(cmd->data);
>> +	td_cpuid = kzalloc(sizeof(*td_cpuid) +
>> +			sizeof(output->entries[0]) *
>> KVM_MAX_CPUID_ENTRIES,
>> +			GFP_KERNEL);
>> +	if (!td_cpuid)
>> +		return -ENOMEM;
>> +
>> +	for (leaf = 0; leaf <= 0x1f; leaf++) {
>> +		output_e = &td_cpuid->entries[i];
>> +		i += tdx_vcpu_get_cpuid_leaf(vcpu, leaf,
>> +					     KVM_MAX_CPUID_ENTRIES -
>> i - 1,
> 
> This should be KVM_MAX_CPUID_ENTRIES - i.

Nice catch!

>> +					     output_e);
>> +	}
>> +
>> +	for (leaf = 0x80000000; leaf <= 0x80000008; leaf++) {
>> +		output_e = &td_cpuid->entries[i];
>> +		i += tdx_vcpu_get_cpuid_leaf(vcpu, leaf,
>> +					     KVM_MAX_CPUID_ENTRIES -
>> i - 1,
> 
> Same here.
>

On Fri, 2025-01-10 at 12:29 +0800, Xiaoyao Li wrote:
> On 1/9/2025 7:07 PM, Francesco Lavra wrote:
> > On 2024-10-30 at 19:00, Rick Edgecombe wrote:
> > > @@ -1055,6 +1144,81 @@ static int tdx_td_vcpu_init(struct
> > > kvm_vcpu
> > > *vcpu, u64 vcpu_rcx)
> > >         return ret;
> > >   }
> > >   
> > > +/* Sometimes reads multipple subleafs. Return how many enties
> > > were
> > > written. */
> > > +static int tdx_vcpu_get_cpuid_leaf(struct kvm_vcpu *vcpu, u32
> > > leaf,
> > > int max_cnt,
> > > +                                  struct kvm_cpuid_entry2
> > > *output_e)
> > > +{
> > > +       int i;
> > > +
> > > +       if (!max_cnt)
> > > +               return 0;
> > > +
> > > +       /* First try without a subleaf */
> > > +       if (!tdx_read_cpuid(vcpu, leaf, 0, false, output_e))
> > > +               return 1;
> > > +
> > > +       /*
> > > +        * If the try without a subleaf failed, try reading
> > > subleafs
> > > until
> > > +        * failure. The TDX module only supports 6 bits of
> > > subleaf
> > > index.
> > 
> > It actually supports 7 bits, i.e. bits 6:0, so the limit below
> > should
> > be 0b1111111.
> 
> Nice catch!
> 
> > > +        */
> > > +       for (i = 0; i < 0b111111; i++) {
> > > +               if (i > max_cnt)
> > > +                       goto out;
> > 
> > This will make this function return (max_cnt + 1) instead of
> > max_cnt.
> > I think the code would be simpler if max_cnt was initialized to
> > min(max_cnt, 0x80) (because 0x7f is a supported subleaf index, as
> > far
> > as I can tell), and the for() condition was changed to `i <
> > max_cnt`.
> 
> Looks better.

You could even simplify this function further by removing the 7-bit
limit altogether and relying on tdx_read_cpuid() returning failure when
the subleaf index is not supported (due to the
TDX_MD_UNREADABLE_SUBLEAF_MASK check).
> 
> > > +               /* Keep reading subleafs until there is a
> > > failure.
> > > */
> > > +               if (tdx_read_cpuid(vcpu, leaf, i, true,
> > > output_e))
> > > +                       return i;
> > > +
> > > +               output_e++;
> 
> here the output_e++ can overflow the buffer.

Not if the for() loop terminates when i reaches max_cnt.

On 10/31/2024 3:00 AM, Rick Edgecombe wrote:
[...]
> +
> +#define TDX_MD_UNREADABLE_LEAF_MASK	GENMASK(30, 7)
> +#define TDX_MD_UNREADABLE_SUBLEAF_MASK	GENMASK(31, 7)
> +
> +static int tdx_read_cpuid(struct kvm_vcpu *vcpu, u32 leaf, u32 sub_leaf,
> +			  bool sub_leaf_set, struct kvm_cpuid_entry2 *out)
> +{
> +	struct kvm_tdx *kvm_tdx = to_kvm_tdx(vcpu->kvm);
> +	u64 field_id = TD_MD_FIELD_ID_CPUID_VALUES;
> +	u64 ebx_eax, edx_ecx;
> +	u64 err = 0;
> +
> +	if (sub_leaf & TDX_MD_UNREADABLE_LEAF_MASK ||
> +	    sub_leaf_set & TDX_MD_UNREADABLE_SUBLEAF_MASK)
> +		return -EINVAL;
It looks weird.
Should be the following?

+	if (leaf & TDX_MD_UNREADABLE_LEAF_MASK ||
+	    sub_leaf & TDX_MD_UNREADABLE_SUBLEAF_MASK)
+		return -EINVAL;


> +
> +	/*
> +	 * bit 23:17, REVSERVED: reserved, must be 0;
> +	 * bit 16,    LEAF_31: leaf number bit 31;
> +	 * bit 15:9,  LEAF_6_0: leaf number bits 6:0, leaf bits 30:7 are
> +	 *                      implicitly 0;
> +	 * bit 8,     SUBLEAF_NA: sub-leaf not applicable flag;
> +	 * bit 7:1,   SUBLEAF_6_0: sub-leaf number bits 6:0. If SUBLEAF_NA is 1,
> +	 *                         the SUBLEAF_6_0 is all-1.
> +	 *                         sub-leaf bits 31:7 are implicitly 0;
> +	 * bit 0,     ELEMENT_I: Element index within field;
> +	 */
> +	field_id |= ((leaf & 0x80000000) ? 1 : 0) << 16;
> +	field_id |= (leaf & 0x7f) << 9;
> +	if (sub_leaf_set)
> +		field_id |= (sub_leaf & 0x7f) << 1;
> +	else
> +		field_id |= 0x1fe;
> +
> +	err = tdx_td_metadata_field_read(kvm_tdx, field_id, &ebx_eax);
> +	if (err) //TODO check for specific errors
> +		goto err_out;
> +
> +	out->eax = (u32) ebx_eax;
> +	out->ebx = (u32) (ebx_eax >> 32);
> +
> +	field_id++;
> +	err = tdx_td_metadata_field_read(kvm_tdx, field_id, &edx_ecx);
> +	/*
> +	 * It's weird that reading edx_ecx fails while reading ebx_eax
> +	 * succeeded.
> +	 */
> +	if (WARN_ON_ONCE(err))
> +		goto err_out;
> +
> +	out->ecx = (u32) edx_ecx;
> +	out->edx = (u32) (edx_ecx >> 32);
> +
> +	out->function = leaf;
> +	out->index = sub_leaf;
> +	out->flags |= sub_leaf_set ? KVM_CPUID_FLAG_SIGNIFCANT_INDEX : 0;
> +
> +	/*
> +	 * Work around missing support on old TDX modules, fetch
> +	 * guest maxpa from gfn_direct_bits.
> +	 */
> +	if (leaf == 0x80000008) {
> +		gpa_t gpa_bits = gfn_to_gpa(kvm_gfn_direct_bits(vcpu->kvm));
> +		unsigned int g_maxpa = __ffs(gpa_bits) + 1;
> +
> +		out->eax = tdx_set_guest_phys_addr_bits(out->eax, g_maxpa);
> +	}
> +
> +	return 0;
> +
> +err_out:
> +	out->eax = 0;
> +	out->ebx = 0;
> +	out->ecx = 0;
> +	out->edx = 0;
> +
> +	return -EIO;
> +}
> +
>
[...]

On Fri, 2024-11-01 at 14:39 +0800, Binbin Wu wrote:
> > +static int tdx_read_cpuid(struct kvm_vcpu *vcpu, u32 leaf, u32 sub_leaf,
> > +			  bool sub_leaf_set, struct kvm_cpuid_entry2 *out)
> > +{
> > +	struct kvm_tdx *kvm_tdx = to_kvm_tdx(vcpu->kvm);
> > +	u64 field_id = TD_MD_FIELD_ID_CPUID_VALUES;
> > +	u64 ebx_eax, edx_ecx;
> > +	u64 err = 0;
> > +
> > +	if (sub_leaf & TDX_MD_UNREADABLE_LEAF_MASK ||
> > +	    sub_leaf_set & TDX_MD_UNREADABLE_SUBLEAF_MASK)
> > +		return -EINVAL;
> It looks weird.
> Should be the following?
> 
> +	if (leaf & TDX_MD_UNREADABLE_LEAF_MASK ||
> +	    sub_leaf & TDX_MD_UNREADABLE_SUBLEAF_MASK)
> +		return -EINVAL;
> 

Yes, nice catch.