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

[Kai Huang <kai.huang@intel.com>]

On Wed, 2021-02-03 at 11:36 -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:
> > > +       /* Exit to userspace if copying from a host userspace address
> > > fails. */
> > > +       if (sgx_read_hva(vcpu, m_hva, &miscselect,
> > > sizeof(miscselect)) ||
> > > +           sgx_read_hva(vcpu, a_hva, &attributes,
> > > sizeof(attributes)) ||
> > > +           sgx_read_hva(vcpu, x_hva, &xfrm, sizeof(xfrm)) ||
> > > +           sgx_read_hva(vcpu, s_hva, &size, sizeof(size)))
> > > +               return 0;
> > > +
> > > +       /* Enforce restriction of access to the PROVISIONKEY. */
> > > +       if (!vcpu->kvm->arch.sgx_provisioning_allowed &&
> > > +           (attributes & SGX_ATTR_PROVISIONKEY)) {
> > > +               if (sgx_12_1->eax & SGX_ATTR_PROVISIONKEY)
> > > +                       pr_warn_once("KVM: SGX PROVISIONKEY
> > > advertised but not allowed\n");
> > > +               kvm_inject_gp(vcpu, 0);
> > > +               return 1;
> > > +       }
> > > +
> > > +       /* Enforce CPUID restrictions on MISCSELECT, ATTRIBUTES and
> > > XFRM. */
> > > +       if ((u32)miscselect & ~sgx_12_0->ebx ||
> > > +           (u32)attributes & ~sgx_12_1->eax ||
> > > +           (u32)(attributes >> 32) & ~sgx_12_1->ebx ||
> > > +           (u32)xfrm & ~sgx_12_1->ecx ||
> > > +           (u32)(xfrm >> 32) & ~sgx_12_1->edx) {
> > > +               kvm_inject_gp(vcpu, 0);
> > > +               return 1;
> > > +       }
> > 
> > Don't you need to deep copy the pageinfo.contents struct as well?
> > Otherwise the guest could change these after they were checked.
> > 
> > But it seems it is checked by the HW and something is caught that would
> > inject a GP anyway? Can you elaborate on the importance of these
> > checks?
> 
> Argh, yes.  These checks are to allow migration between systems with different
> SGX capabilities, and more importantly to prevent userspace from doing an end
> around on the restricted access to PROVISIONKEY.
> 
> IIRC, earlier versions did do a deep copy, but then I got clever.  Anyways, yeah,
> sadly the entire pageinfo.contents page will need to be copied.

I don't fully understand the problem. Are you worried about contents being updated by
other vcpus during the trap? 

And I don't see how copy can avoid this problem. Even you do copy, the content can
still be modified afterwards, correct? So what's the point of copying? Looks a better
solution is to kick all vcpus and put them into block state while KVM is doing ENCLS
for guest.