RE: [intel-sgx-kernel-dev] [PATCH 08/10] kvm: vmx: add guest's IA32_SGXLEPUBKEYHASHn runtime switch support

["Christopherson, Sean J" <sean.j.christopherson@intel.com>]

Andy Lutomirski <luto@kernel.org> wrote:
> On Thu, May 11, 2017 at 9:56 PM, Huang, Kai <kai.huang@linux.intel.com> wrote:
> > I am not sure whether the cost of writing to 4 MSRs would be *extremely*
> > slow, as when vcpu is schedule in, KVM is already doing vmcs_load, writing
> > to several MSRs, etc.
> 
> I'm speculating that these MSRs may be rather unoptimized and hence
> unusualy slow.
> 

Good speculation :)  We've been told to expect that writing the hash MSRs
will be at least 2.5x slower than normal MSRs.

> >
> >>
> >> Have a percpu variable that stores the current SGXLEPUBKEYHASH along
> >> with whatever lock is needed (probably just a mutex).  Users of EINIT
> >> will take the mutex, compare the percpu variable to the desired value,
> >> and, if it's different, do WRMSR and update the percpu variable.
> >>
> >> KVM will implement writes to SGXLEPUBKEYHASH by updating its in-memory
> >> state but *not* changing the MSRs.  KVM will trap and emulate EINIT to
> >> support the same handling as the host.  There is no action required at
> >> all on KVM guest entry and exit.
> >
> >
> > This is doable, but SGX driver needs to do those things and expose
> > interfaces for KVM to use. In terms of the percpu data, it is nice to have,
> > but I am not sure whether it is mandatory, as IMO EINIT is not even in
> > performance critical path. We can simply read old value from MSRs out and
> > compare whether the old equals to the new.
> 
> I think the SGX driver should probably live in arch/x86, and the
> interface could be a simple percpu variable that is exported (from the
> main kernel image, not from a module).
> 

Agreed, this would make life easier for future SGX code that can't be
self-contained in the driver, e.g. EPC cgroup.  Future architectural
enhancements might also require tighter integration with the kernel.


> >>
> >> I would advocate for the former approach.  (But you can't remap the
> >> parameters due to TOCTOU issues, locking, etc.  Just copy them.  I
> >> don't see why this is any more complicated than emulating any other
> >> instruction that accesses memory.)
> >
> >
> > No you cannot just copy. Because all address in guest's ENCLS parameters are
> > guest's virtual address, we cannot use them to execute ENCLS in KVM. If any
> > guest virtual addresses is used in ENCLS parameters, for example,
> > PAGEINFO.SECS, PAGEINFO.SECINFO/PCMD, etc, you have to remap them to KVM's
> > virtual address.
> >
> > Btw, what is TOCTOU issue? would you also elaborate locking issue?
> 
> I was partially mis-remembering how this worked.  It looks like
> SIGSTRUCT and EINITTOKEN could be copied but SECS would have to be
> mapped.  If KVM applied some policy to the launchable enclaves, it
> would want to make sure that it only looks at fields that are copied
> to make sure that the enclave that gets launched is the one it
> verified.  The locking issue I'm imagining is that the SECS (or
> whatever else might be mapped) doesn't disappear and get reused for
> something else while it's mapped in the host.  Presumably KVM has an
> existing mechanism for this, but maybe SECS is special because it's
> not quite normal memory IIRC.
> 

Mapping the SECS in the host should not be an issue, AFAIK there aren't
any restrictions on the VA passed to EINIT as long as it resolves to a
SECS page in the EPCM, e.g. the SGX driver maps the SECS for EINIT with
an arbitrary VA.

I don't think emulating EINIT introduces any TOCTOU race conditions that
wouldn't already exist.  Evicting the SECS or modifying the page tables
on a different thread while executing EINIT is either a guest kernel bug
or bizarre behavior that the guest can already handle.  Similarly, KVM
would need special handling for evicting a guest's SECS, regardless of
EINIT emulation.

> >> [1] Guests that steal sealed data from each other or from the host can
> >> manipulate that data without compromising the hypervisor by simply
> >> loading the same enclave that its rightful owner would use.  If you're
> >> trying to use SGX to protect your crypto credentials so that, if
> >> stolen, they can't be used outside the guest, I would consider this to
> >> be a major flaw.  It breaks the security model in a multi-tenant cloud
> >> situation.  I've complained about it before.
> >>
> >
> > Looks potentially only guest's IA32_SGXLEPUBKEYHASHn may be leaked? In this
> > case even it is leaked looks we cannot dig anything out just the hash value?
> 
> Not sure what you mean.  Are you asking about the lack of guest
> personalization?
> 
> Concretely, imagine I write an enclave that seals my TLS client
> certificate's private key and offers an API to sign TLS certificate
> requests with it.  This way, if my system is compromised, an attacker
> can use the certificate only so long as they have access to my
> machine.  If I kick them out or if they merely get the ability to read
> the sealed data but not to execute code, the private key should still
> be safe.  But, if this system is a VM guest, the attacker could run
> the exact same enclave on another guest on the same physical CPU and
> sign using my key.  Whoops!

I know this issue has been raised internally as well, but I don't know
the status of the situation.  I'll follow up and provide any information
I can.