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

["Huang, Kai" <kai.huang@linux.intel.com>]

Hi Andy,

Thanks for your helpful comments! See my reply below, and some questions 
as well.

On 5/12/2017 3:28 PM, Andy Lutomirski wrote:
> [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:

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.

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

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

I am not very convinced why "what enclaves are allowed" in host would 
apply to guest. Can you elaborate? I mean in general virtualization just 
focus emulating hardware behavior. If a native machine is able to run 
any LE, the virtual machine should be able to as well (of course, with 
guest's IA32_FEATURE_CONTROL[bit 17] set).

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

Looks this flaw is not the same issue as above (host enclave policy 
applies to guest)?

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

Currently on host SGX driver, which is pretty much self-contained, 
implements all SGX related staff.

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

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?

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

Sorry I don't quite follow this part. Why would host's value leaks into 
guest? I suppose the *value* means host's IA32_SGXLEPUBKEYHASHn? guest's 
MSR read/write is always trapped and emulated by KVM.

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

Thanks,
-Kai