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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Feb 04, 2021, Kai Huang wrote:
> 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?

The goal isn't correctness, it's to prevent a TOCTOU bug.  E.g. the guest could
do ECREATE w/ SECS.SGX_ATTR_PROVISIONKEY=0, and simultaneously set
SGX_ATTR_PROVISIONKEY to bypass the above check.

> Looks a better solution is to kick all vcpus and put them into block state
> while KVM is doing ENCLS for guest.

No.  (a) it won't work, as the memory is writable from host userspace.  (b) that
does not scale, at all.



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux