Re: [PATCH v2 24/25] KVM: x86: Introduce KVM_TDX_GET_CPUID

[Xiaoyao Li]

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.