linux-kernel - Re: [PATCH v2] KVM: x86: nSVM/nVMX: Fix handling triple fault on RSM instruction

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240125005710.GA8443@yjiang5-mobl.amr.corp.intel.com>
Date: Wed, 24 Jan 2024 16:57:50 -0800
From: Yunhong Jiang <yunhong.jiang@...ux.intel.com>
To: Michal Wilczynski <michal.wilczynski@...el.com>
Cc: seanjc@...gle.com, pbonzini@...hat.com, mlevitsk@...hat.com,
	tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
	dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com,
	kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
	dedekind1@...il.com, yuan.yao@...el.com,
	Zheyu Ma <zheyuma97@...il.com>
Subject: Re: [PATCH v2] KVM: x86: nSVM/nVMX: Fix handling triple fault on RSM
 instruction

On Tue, Jan 23, 2024 at 02:15:55AM +0200, Michal Wilczynski wrote:
> Syzkaller found a warning triggered in nested_vmx_vmexit().
> vmx->nested.nested_run_pending is non-zero, even though we're in
> nested_vmx_vmexit(). Generally, trying  to cancel a pending entry is
> considered a bug. However in this particular scenario, the kernel
> behavior seems correct.
> 
> Syzkaller scenario:
> 1) Set up VCPU's
> 2) Run some code with KVM_RUN in L2 as a nested guest
> 3) Return from KVM_RUN
> 4) Inject KVM_SMI into the VCPU
> 5) Change the EFER register with KVM_SET_SREGS to value 0x2501
> 6) Run some code on the VCPU using KVM_RUN
> 7) Observe following behavior:
> 
> kvm_smm_transition: vcpu 0: entering SMM, smbase 0x30000
> kvm_entry: vcpu 0, rip 0x8000
> kvm_entry: vcpu 0, rip 0x8000
> kvm_entry: vcpu 0, rip 0x8002
> kvm_smm_transition: vcpu 0: leaving SMM, smbase 0x30000
> kvm_nested_vmenter: rip: 0x0000000000008002 vmcs: 0x0000000000007000
>                     nested_rip: 0x0000000000000000 int_ctl: 0x00000000
> 		    event_inj: 0x00000000 nested_ept=n guest
> 		    cr3: 0x0000000000002000
> kvm_nested_vmexit_inject: reason: TRIPLE_FAULT ext_inf1: 0x0000000000000000
>                           ext_inf2: 0x0000000000000000 ext_int: 0x00000000
> 			  ext_int_err: 0x00000000
> 
> What happened here is an SMI was injected immediately and the handler was
> called at address 0x8000; all is good. Later, an RSM instruction is
> executed in an emulator to return to the nested VM. em_rsm() is called,
> which leads to emulator_leave_smm(). A part of this function calls VMX/SVM
> callback, in this case vmx_leave_smm(). It attempts to set up a pending
> reentry to guest VM by calling nested_vmx_enter_non_root_mode() and sets
> vmx->nested.nested_run_pending to one. Unfortunately, later in
> emulator_leave_smm(), rsm_load_state_64() fails to write invalid EFER to
> the MSR. This results in em_rsm() calling triple_fault callback. At this
> point it's clear that the KVM should call the vmexit, but
> vmx->nested.nested_run_pending is left set to 1.
> 
> Similar flow goes for SVM, as the bug also reproduces on AMD platforms.
> 
> To address this issue, reset the nested_run_pending flag in the
> triple_fault handler. However, it's crucial to note that
> nested_pending_run cannot be cleared in all cases. It should only be
> cleared for the specific instruction requiring hardware VM-Enter to
> complete the emulation, such as RSM. Previously, there were instances
> where KVM prematurely synthesized a triple fault on nested VM-Enter. In
> these cases, it is not appropriate to zero the nested_pending_run.
> 
> To resolve this, introduce a new emulator flag indicating the need for
> HW VM-Enter to complete emulating RSM. Based on this flag, a decision can
> be made in vendor-specific triple fault handlers about whether
> nested_pending_run needs to be cleared.
Would it be ok to move the followed emulator_leave_smm() code into
vmx_leave_smm, before setting nested_run_pending bit? It avoids changing
the generic emulator code.

#ifdef CONFIG_X86_64
        if (guest_cpuid_has(vcpu, X86_FEATURE_LM))
                return rsm_load_state_64(ctxt, &smram.smram64);
        else
#endif
                return rsm_load_state_32(ctxt, &smram.smram32);

--jyh