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

[Francesco Lavra <francescolavra.fl@gmail.com>]

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.