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

Kai Huang
2021-02-04T00:12:15Z

On Wed, 2021-02-03 at 15:59 -0800, Sean Christopherson wrote:
> On Thu, Feb 04, 2021, Kai Huang wrote:
> > 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:
> > > > > > 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.
>
> It's very feasible. XOR the bit in a tight loop, build the enclave on a
> separate thread. Do that until EINIT succeeds. Compared to other timing
> attacks, I doubt it'd take all that long to get a successful result.

How does it work? The setting PROVISION bit needs to be set after KVM checks SECS's
attribute, and before KVM actually does ECREATE, right?

>
> Regardless, the difficulty in exploiting the bug is irrelevant, it's a glaring
> flaw that needs to be fixed.

Sure.