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.
