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

[Andy Lutomirski <luto@kernel.org>]

[resending due to some kind of kernel.org glitch -- sorry if anyone
gets duplicates]

On Thu, May 11, 2017 at 5:32 PM, Huang, Kai <kai.huang@linux.intel.com> wrote:
> My current patch is based on this assumption. For KVM guest, naturally, we
> will write the cached value to real MSRs when vcpu is scheduled in. For
> host, SGX driver should write its own value to MSRs when it performs EINIT
> for LE.

This seems unnecessarily slow (perhaps *extremely* slow) to me.  I
would propose a totally different solution:

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.

FWIW, I think that KVM will, in the long run, want to trap EINIT for
other reasons: someone is going to want to implement policy for what
enclaves are allowed that applies to guests as well as the host.
Also, some day Intel may fix its architectural design flaw [1] by
allowing EINIT to personalize the enclave's keying, and, if it's done
by a new argument to EINIT instead of an MSR, KVM will have to trap
EINIT to handle it.

>
> One argument against this approach is KVM guest should never have impact on
> host side, meaning host should not be aware of such MSR change

As a somewhat generic comment, I don't like this approach to KVM
development.  KVM mucks with lots of important architectural control
registers, and, in all too many cases, it tries to do so independently
of the other arch/x86 code.  This ends up causing all kinds of grief.

Can't KVM and the real x86 arch code cooperate for real?  The host and
the KVM code are in arch/x86 in the same source tree.

>
> 2. KVM should restore MSRs after changing for guest.

No, this is IMO silly.  Don't restore it on each exit, in the user
return hook, or anywhere else.  Just make sure the host knows it was
changed.

> Another thing is, not quite related to selecting which approach above, and
> either we choose approach 1 or approach 2, KVM still suffers the performance
> loss of writing (and/or reading) to IA32_SGXLEPUBKEYHASHn MSRs, either when
> vcpu scheduled in or during each VMEXIT/VMENTRY. Given the fact that the
> IA32_SGXLEPUBKEYHASHn will only be used by EINIT, We can actually do some
> optimization by trapping EINIT from guest and only update MSRs in EINIT
> VMEXIT.

Yep.

> But trapping ENCLS requires either 1) KVM to run ENCLS on hebalf of guest,
> in which case we have to reconstruct and remap guest's ENCLS parameters and
> skip the ENCLS for guest; 2) using MTF to let guest to run ENCLS again,
> while still trapping ENCLS.

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.)

If necessary for some reason, trap EINIT when the SGXLEPUBKEYKASH is
wrong and then clear the exit flag once the MSRs are in sync.  You'll
need to be careful to avoid races in which the host's value leaks into
the guest.  I think you'll find that this is more complicated, less
flexible, and less performant than just handling ENCLS[EINIT] directly
in the host.

[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.