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

[Edgecombe, Rick P]

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@xxxxxxxxx/

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.