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.
