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.
