linux-kernel - Re: [PATCH v2 24/25] KVM: x86: Introduce KVM_TDX_GET

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <173ba3b7c890f1ec523853b3a12859eb309cd563.camel@gmail.com>
Date: Fri, 10 Jan 2025 11:34:24 +0100
From: Francesco Lavra <francescolavra.fl@...il.com>
To: Xiaoyao Li <xiaoyao.li@...el.com>, rick.p.edgecombe@...el.com
Cc: isaku.yamahata@...il.com, kai.huang@...el.com, kvm@...r.kernel.org, 
 linux-kernel@...r.kernel.org, pbonzini@...hat.com,
 reinette.chatre@...el.com,  seanjc@...gle.com,
 tony.lindgren@...ux.intel.com, yan.y.zhao@...el.com
Subject: Re: [PATCH v2 24/25] KVM: x86: Introduce KVM_TDX_GET_CPUID

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.