> On 25 Mar 2019, at 19:30, Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> wrote:
>
> Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> writes:
>
>>
>> In other words: work in progress, stay tuned.
>
> It seems I was able to identify the culprit,
>
> commit 5bea5123cbf08f990a1aee8f08c643a272e06a0f
> Author: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> Date: Tue Sep 18 15:19:17 2018 +0200
>
> KVM: VMX: check nested state and CR4.VMXE against SMM
>
> this also matches Jon's observation that the regression happened in
> 4.19.
>
> In case I'm not mistaken, the issue is: while trying to forbid setting
> VMXE from SMM we forgot about RSM which in case VMXE was set before
> going into SMM is supposed to restore it. My first RFC would then be to
> move the check out of vmx_set_cr4() to handle_set_cr4():
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 02cf8a551bd1..1f2724c5e99d 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -2901,17 +2901,13 @@ int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
> SECONDARY_EXEC_DESC);
> }
>
> - if (cr4 & X86_CR4_VMXE) {
> - /*
> - * To use VMXON (and later other VMX instructions), a guest
> - * must first be able to turn on cr4.VMXE (see handle_vmon()).
> - * So basically the check on whether to allow nested VMX
> - * is here. We operate under the default treatment of SMM,
> - * so VMX cannot be enabled under SMM.
> - */
> - if (!nested_vmx_allowed(vcpu) || is_smm(vcpu))
> - return 1;
> - }
> + /*
> + * To use VMXON (and later other VMX instructions), a guest must first
> + * be able to turn on cr4.VMXE (see handle_vmon()). So basically the
> + * check on whether to allow nested VMX is here.
> + */
> + if (cr4 & X86_CR4_VMXE && !nested_vmx_allowed(vcpu))
Please put () around “cr4 & X86_CR4_VMXE”.
> + return 1;
>
> if (to_vmx(vcpu)->nested.vmxon && !nested_cr4_valid(vcpu, cr4))
> return 1;
> @@ -4627,6 +4623,10 @@ static int handle_set_cr0(struct kvm_vcpu *vcpu, unsigned long val)
>
> static int handle_set_cr4(struct kvm_vcpu *vcpu, unsigned long val)
> {
> + /* VMXE can't be enabled from SMM */
> + if (val & X86_CR4_VMXE && is_smm(vcpu))
> + return 1;
> +
> if (is_guest_mode(vcpu)) {
> struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
> unsigned long orig_val = val;
>
>
> Paolo, as the author of the original commit, what do you think? Would
> this protection be enough or do you envision any other scenarios?
If I understand correctly, RSM is handled by the fact it raises #UD which KVM intercepts which invokes the x86 emulator. Reaching em_rsm().
First, note that em_rsm() calls vmx_pre_leave_smm() which handles similar situation when calling nested_vmx_enter_non_root_mode() while temporarily clearing HF_SMM_MASK from hflags.
Second, note that em_rsm() calls rsm_load_state_64() -> rsm_enter_protected_mode() which is responsible for setting all CRs and performs various hacks regarding it.
I would consider to modify rsm_enter_protected_mode() to set CR4 while temporarily clearing HF_SMM_MASK from hflags. Similarly to as done in vmx_pre_leave_smm().
In order to continue clearly maintaining all checks regarding setting CR4 only in kvm_set_cr4() and vmx_set_cr4().
-Liran
>
> --
> Vitaly
