Re: [PATCH] KVM: nVMX: Replace kvm_set_cr0 with vmx_set_cr0 in load_vmcs12_host_state

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 1938 bytes --]

On 2013-02-24 19:56, Avi Kivity wrote:
> On Sun, Feb 24, 2013 at 12:49 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> On 2013-02-24 11:11, Avi Kivity wrote:
>>> On Sun, Feb 24, 2013 at 11:40 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>>>>>> We have the same problem in KVM_SET_SREGS.
>>>>>>
>>>>>> I don't see the problem. kvm_arch_vcpu_ioctl_set_sregs open-codes the
>>>>>> state update, not applying any transition checks.
>>>>>
>>>>> That's the problem.  We have this open coding in three different
>>>>> places (KVM_SET_SREGS, nvmx, nsvm).
>>>>>
>>>>> It's not as if vmx_set_cr0() is defined as "kvm_set_cr0() without the
>>>>> transition checks".
>>>>
>>>> ...and without mmu updates. The latter is done via or after the closing
>>>> cr3 update. Interestingly, nsvm does not perform kvm_set_cr3 on vmexit
>>>> when in npt mode. Seems things aren't that regular.
>>>
>>> We do want the mmu updates.  Of course they can't be attached to
>>> kvm_set_cr0_without_the_checks() since there is cross-register
>>> dependencies.
>>>
>>> Option 1 is kvm_set_multiple_cr().  This does the checks and updates,
>>> but only after all registers are updated.
>>> Option 2 is kvm_begin_cr_transaction()/kvm_commit_cr_transaction().
>>> Prettier and more flexible, but a more complicated to implement.
>>>
>>
>> The only thing that these three use case truly have in common is the
>> closing kvm_mmu_reset_context. Maybe nvmx and nsvm can share a bit more.
>> But let's get nVMX right first, then think about sharing.
> 
> They all need consistency checks, otherwise userspace or the guest and
> inject inconsistent values and perhaps exploit the host.

To my understanding, the hardware does this for us: If we try to enter
the guest (L1, L2) with invalid CRx bits set or cleared, we get an
error, at least on Intel. But I bet AMD does so as well - and, if not,
it would make this test specific again.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]