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: <CABgObfYaUHXyRmsmg8UjRomnpQ0Jnaog9-L2gMjsjkqChjDYUQ@mail.gmail.com>
Date: Thu, 8 Feb 2024 11:31:03 +0100
From: Paolo Bonzini <pbonzini@...hat.com>
To: Yunhong Jiang <yunhong.jiang@...ux.intel.com>
Cc: Michal Wilczynski <michal.wilczynski@...el.com>, seanjc@...gle.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 Thu, Jan 25, 2024 at 1:59 AM Yunhong Jiang
<yunhong.jiang@...ux.intel.com> wrote:
> 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);

I agree with Michal that vendor code should not be in charge of
calling rsm_load_state_*.

However, I don't understand the problem or the fix.

The possible causes are two, and in neither case the fix is to clear
nested_run_pending:

1) if the problem is that setting nested_run_pending was premature,
the correct fix in my opinion is to split the leave_smm vendor
callback in "prepare" and "commit" parts; not to clear it later.  See
the attached, untested, patch.

2) otherwise, if the problem is that we have not gone through the
vmenter yet, then KVM needs to do that and _then_ inject the triple
fault. The fix is to merge the .triple_fault and .check_nested_events
callbacks, with something like the second attached patch - which
probably has so many problems that I haven't even tried to compile it.

The first patch should be equivalent to yours, and I guess it is
okay-ish with a few more comments that explain what's going on.

Paolo

View attachment "nested.patch" of type "text/x-patch" (8336 bytes)

View attachment "nested2.patch" of type "text/x-patch" (2691 bytes)