On Wed, 2021-02-03 at 15:36 -0800, Sean Christopherson wrote:
> 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.
Oh ok. Agreed.
However, such attack would require precise timing. Not sure whether it is feasible in
practice.
>
> > 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.
Good point. Agreed.
