Re: [RFC PATCH v3 23/27] KVM: VMX: Add SGX ENCLS[ECREATE] handler to enforce CPUID restrictions

Sean Christopherson
2021-02-03T17:09:12Z

On Wed, Feb 03, 2021, Kai Huang wrote:
> On Tue, 2 Feb 2021 17:36:12 -0800 Sean Christopherson wrote:
> > On Wed, Feb 03, 2021, Edgecombe, Rick P wrote:
> > > On Tue, 2021-01-26 at 22:31 +1300, Kai Huang wrote:
> > > > +       /*
> > > > +        * Verify alignment early.  This conveniently avoids having
> > > > to worry
> > > > +        * about page splits on userspace addresses.
> > > > +        */
> > > > +       if (!IS_ALIGNED(pageinfo.metadata, 64) ||
> > > > +           !IS_ALIGNED(pageinfo.contents, 4096)) {
> > > > +               kvm_inject_gp(vcpu, 0);
> > > > +               return 1;
> > > > +       }
> > > > +
> > > > +       /*
> > > > +        * Translate the SECINFO, SOURCE and SECS pointers from GVA
> > > > to GPA.
> > > > +        * Resume the guest on failure to inject a #PF.
> > > > +        */
> > > > +       if (sgx_gva_to_gpa(vcpu, pageinfo.metadata, false,
> > > > &metadata_gpa) ||
> > > > +           sgx_gva_to_gpa(vcpu, pageinfo.contents, false,
> > > > &contents_gpa) ||
> > > > +           sgx_gva_to_gpa(vcpu, secs_gva, true, &secs_gpa))
> > > > +               return 1;
> > > > +
> > >
> > > Do pageinfo.metadata and pageinfo.contents need cannonical checks here?
> >
> > Bugger, yes. So much boilerplate needed in this code :-/
> >
> > Maybe add yet another helper to do alignment+canonical checks, up where the
> > IS_ALIGNED() calls are?
>
> sgx_get_encls_gva() already does canonical check. Couldn't we just use it?

After rereading the SDM for the bajillionth time, yes, these should indeed use
sgx_get_encls_gva(). Originally I was thinking they were linear addresses, but
they are effective addresses that use DS, i.e. not using the helper to avoid the
DS.base adjustment for 32-bit mode was also wrong.

> For instance:
>
> if (sgx_get_encls_gva(vcpu, pageinfo.metadata, 64, 64 &metadata_gva) ||
> sgx_get_encls_gva(vcpu, pageinfo.contents, 4096, 4096,
> &contents_gva))
> return 1;