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.
