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

["Cohen, Haim" <haim.cohen@intel.com>]

On 6/8/2017 7:53 PM, Andy Lutomirski wrote:
>
>On Thu, Jun 8, 2017 at 4:47 PM, Huang, Kai <kai.huang@linux.intel.com> wrote:
>>
>>
>> On 6/9/2017 12:31 AM, Jarkko Sakkinen wrote:
>>>
>>> On Wed, Jun 07, 2017 at 08:52:42AM +1200, Huang, Kai wrote:
>>>>
>>>>
>>>>
>>>> On 5/18/2017 7:45 PM, Huang, Kai wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 5/17/2017 12:09 PM, Andy Lutomirski wrote:
>>>>>>
>>>>>> On Mon, May 15, 2017 at 5:48 PM, Huang, Kai
>>>>>> <kai.huang@linux.intel.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 5/12/2017 6:11 PM, Andy Lutomirski 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.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 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).
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 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).
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> I strongly disagree.  I can imagine two classes of sensible
>>>>>>>> policies for launch control:
>>>>>>>>
>>>>>>>> 1. Allow everything.  This seems quite sensible to me.
>>>>>>>>
>>>>>>>> 2. Allow some things, and make sure that VMs have at least as
>>>>>>>> restrictive a policy as host root has.  After all, what's the
>>>>>>>> point of restricting enclaves in the host if host code can
>>>>>>>> simply spawn a little VM to run otherwise-disallowed enclaves?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> What's the current SGX driver launch control policy? Yes allow
>>>>>>> everything works for KVM so lets skip this. Are we going to
>>>>>>> support allowing several LEs, or just allowing one single LE? I
>>>>>>> know Jarkko is doing in-kernel LE staff but I don't know details.
>>>>>>>
>>>>>>> I am trying to find a way that we can both not break host launch
>>>>>>> control policy, and be consistent to HW behavior (from guest's
>>>>>>> view).
>>>>>>> Currently we
>>>>>>> can create a KVM guest with runtime change to
>>>>>>> IA32_SGXLEPUBKEYHASHn either enabled or disabled. I introduced an
>>>>>>> Qemu parameter 'lewr' for this purpose.
>>>>>>> Actually I introduced below Qemu SGX parameters for creating guest:
>>>>>>>
>>>>>>>          -sgx epc=<size>,lehash='SHA-256 hash',lewr
>>>>>>>
>>>>>>> where 'epc' specifies guest's EPC size, lehash specifies
>>>>>>> (initial) value of
>>>>>>> guest's IA32_SGXLEPUBKEYHASHn, and 'lewr' specifies whether guest
>>>>>>> is allowed to change guest's IA32_SGXLEPUBKEYHASHn at runtime.
>>>>>>>
>>>>>>> If host only allows one single LE to run, KVM can add a restrict
>>>>>>> that only allows to create KVM guest with runtime change to
>>>>>>> IA32_SGXLEPUBKEYHASHn disabled, so that only host allowed
>>>>>>> (single) hash can be used by guest. From guest's view, it simply
>>>>>>> has IA32_FEATURE_CONTROL[bit17] cleared and has
>>>>>>> IA32_SGXLEPUBKEYHASHn with default value to be host allowed
>>>>>>> (single) hash.
>>>>>>>
>>>>>>> If host allows several LEs (not but everything), and if we create
>>>>>>> guest with 'lewr', then the behavior is not consistent with HW
>>>>>>> behavior, as from guest's hardware's point of view, we can
>>>>>>> actually run any LE but we have to tell guest that you are only
>>>>>>> allowed to change IA32_SGXLEPUBKEYHASHn to some specific values.
>>>>>>> One compromise solution is we don't allow to create guest with
>>>>>>> 'lewr' specified, and at the meantime, only allow to create guest
>>>>>>> with host approved hashes specified in 'lehash'. This will make
>>>>>>> guest's behavior consistent to HW behavior but only allows guest
>>>>>>> to run one LE (which is specified by 'lehash' when guest is
>>>>>>> created).
>>>>>>
>>>>>>
>>>>>> I'm not sure I entirely agree for a couple reasons.
>>>>>>
>>>>>> 1. I wouldn't be surprised if the kernel ends up implementing a
>>>>>> policy in which it checks all enclaves (not just LEs) for
>>>>>> acceptability.  In fact, if the kernel sticks with the "no LE at
>>>>>> all or just kernel-internal LE", then checking enclaves directly
>>>>>> against some
>>>>>> admin- or distro-provided signer list seems reasonable.  This type
>>>>>> of policy can't be forwarded to a guest by restricting allowed LE
>>>>>> signers.  But this is mostly speculation since AFAIK no one has
>>>>>> seriously proposed any particular policy support and the plan was
>>>>>> to not have this for the initial implementation.
>>>>>>
>>>>>> 2. While matching hardware behavior is nice in principle, there
>>>>>> doesn't seem to be useful hardware behavior to match here.  If the
>>>>>> host had a list of five allowed LE signers, how exactly would it
>>>>>> restrict the MSRs?  They're not written atomically, so you can't
>>>>>> directly tell what's being written.
>>>>>
>>>>>
>>>>> In this case I actually plan to just allow creating guest with
>>>>> guest's IA32_SGXLEPUBKEYHASHn disabled (without 'lewr' specified).
>>>>> If 'lewr' is specified, creating guest will fail. And we only allow
>>>>> creating guest with host allowed hash values (with
>>>>> 'lehash=hash-value'), and if 'hash-value' specified by 'lehash' is
>>>>> not allowed by host, we also fail to create guest.
>>>>>
>>>>> We can only allow creating guest with 'lewr' specified when host
>>>>> allows anything.
>>>>>
>>>>> But in this way, we are restricting guest OS's ability to run LE,
>>>>> as only one LE, that is specified by 'lehash' parameter, can be
>>>>> run. But I think this won't hurt much, as multiple guests still are
>>>>> able to run different LEs?
>>>>>
>>>>> Also, the only way to fail an MSR
>>>>>>
>>>>>> write is to send #GP, and Windows (and even Linux) may not expect
>>>>>> that.  Linux doesn't panic due to #GP on MSR writes these days,
>>>>>> but you still get a big fat warning.  I wouldn't be at all
>>>>>> surprised if Windows BSODs.
>>>>>
>>>>>
>>>>> We cannot allow writing some particular value to MSRs successfully,
>>>>> while injecting #GP when writing other values to the same MSRs. So
>>>>> #GP is not option.
>>>>>
>>>>> ENCLS[EINIT], on the other hand, returns an actual
>>>>>>
>>>>>> error code.  I'm not sure that a sensible error code exists
>>>>>> ("SGX_HYPERVISOR_SAID_NO?", perhaps),
>>>>>
>>>>>
>>>>> Looks no such error code exists. And we cannot return such error
>>>>> code to guest as such error code is only supposed to be valid when
>>>>> ENCLS is run in hypervisor.
>>>>>
>>>>> but SGX_INVALID_EINITTOKEN seems
>>>>>>
>>>>>> to mean, more or less, "the CPU thinks you're not authorized to do
>>>>>> this", so forcing that error code could be entirely reasonable.
>>>>>>
>>>>>> If the host policy is to allow a list of LE signers, you could
>>>>>> return SGX_INVALID_EINITTOKEN if the guest tries to EINIT an LE
>>>>>> that isn't in the list.
>>>>>
>>>>>
>>>>> But this would be inconsistent with HW behavior. If the hash value
>>>>> in guest's IA32_SGXLEPUBKEYHASHn is matched with the one passed by
>>>>> EINIT, EINIT is not supposed to return SGX_INVALID_EINITTOKEN.
>>>>>
>>>>> I think from VMM's perspective, emulating HW behavior to be
>>>>> consistent with real HW behavior is very important.
>>>>>
>>>>> Paolo, would you provide your comments?
>>>>
>>>>
>>>> Hi all,
>>>>
>>>> This has been quite for a while and I'd like to start discussion again.
>>>> Jarkko told me that currently he only supports one LE in SGX driver,
>>>> but I am not sure whether he is going to extend in the future or
>>>> not. I think this might also depend on requirements from customers.
>>>>
>>>> Andy,
>>>>
>>>> If we only support one LE in driver, then we can only support the
>>>> same LE for all KVM guests, according to your comments that host
>>>> kernel launch control policy should also apply to KVM guests? WOuld you
>comments more?
>>>>
>>>> Jarkko,
>>>>
>>>> Could you help to clarify the whole launch control policy in host
>>>> side so that we can have a better understanding together?
>>>>
>>>> Thanks,
>>>> -Kai
>>>
>>>
>>> So. I have pass through LE. It creates EINITTOKEN for anything.
>>> Couldn't VMM keep virtual values for MSRs and ask host side LE create
>>> token when it needs to?
>>
>>
>> Hi Jarkko,
>>
>> Thanks for replying. VMM doesn't need driver to generate EINITTOKEN.
>> The EINITTOKEN is from guest too, upon VMM traps EINIT.
>>
>> I think Andy's comments is, if host SGX driver only allows someone's
>> LE to run (ex, LE from Intel, Redhat, etc...), then SGX driver should
>> also govern LEs from KVM guests as well, by checking SIGSTRUCT from
>> KVM guest (the SIGSTRUCT is provided by KVM by trapping guest's EINIT).
>>
>> In my understanding, although you only allows one LE in kernel, but
>> you won't limit who's LE can be run (basically kernel can run LE
>> signed by anyone, but just one LE when kernel is running), so I don't
>> see there is any limitation to KVM guests here.
>>
>> But it may still be better if SGX driver can provide function like:
>>
>>     int sgx_validate_sigstruct(struct sigstruct *sig);
>>
>> for KVM to call, in case driver is changed (ex, to only allows LEs
>> from some particular ones to run), but this is not necessary now. KVM
>> changes can be done later when driver make the changes.
>>
>> Andy,
>>
>> Am I understanding correctly? Does this make sense to you?
>
>My understanding is that the kernel will not (at least initially) allow users to supply
>an LE.  The kernel will handle launches all by itself.  How it does so is an
>implementation detail, and it seems to me that it will cause compatibility issues if
>guests have to use the host's LE.
>
>sgx_validate_sigstruct(...) sounds like it could be a good approach to me.
>

I don't think the kernel needs to support more than one LE.
For my understanding the guest kernel may include different LE and will require different LE hash, either by configuring 'lehash' when launching the guest, or by the guest kernel setting the hash value.
I assume that the VMM should not block this MSR writes and allow the guest to set the hash values for its own LE (by caching the MSR writes and setting the values on EINIT).

So I don't see why the host kernel should provide EINITTOKEN for the guests.
Moreover, I guess that the approach of LE providing tokens to any enclave is only the initial implementation, and different distributions may decide to add some logic to the token generation, so it might be that the host LE or the guest LE will not provide token to some enclave, and I don't think we'll want to override this kernel logic.

>>
>> Thanks,
>> -Kai
>>
>>>
>>> /Jarkko
>>>
>>