linux-kernel - Re: [PATCH 0/3] KVM: VMX: Fix for kexec VMCLEAR and VMXON cleanup

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: <72839897-3ed7-8151-178b-9f266fefc8ba@redhat.com>
Date:   Fri, 28 Feb 2020 12:02:13 +0100
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Sean Christopherson <sean.j.christopherson@...el.com>
Cc:     Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/3] KVM: VMX: Fix for kexec VMCLEAR and VMXON cleanup

On 27/02/20 23:30, Sean Christopherson wrote:
> Patch 1 fixes a a theoretical bug where a crashdump NMI that arrives
> while KVM is messing with the percpu VMCS list would result in one or more
> VMCSes not being cleared, potentially causing memory corruption in the new
> kexec'd kernel.
> 
> Patch 2 isn't directly related, but it conflicts with the crash cleanup
> changes, both from a code and a semantics perspective.  Without the crash
> cleanup, IMO hardware_enable() should do crash_disable_local_vmclear()
> if VMXON fails, i.e. clean up after itself.  But hardware_disable()
> doesn't even do crash_disable_local_vmclear() (which is what got me
> looking at that code in the first place).  Basing the VMXON change on top
> of the crash cleanup avoids the debate entirely.
> 
> Patch 3 is a simple clean up (no functional change intended ;-) ).
> 
> I verified my analysis of the NMI bug by simulating what would happen if
> an NMI arrived in the middle of list_add() and list_del().  The below
> output matches expectations, e.g. nothing hangs, the entry being added
> doesn't show up, and the entry being deleted _does_ show up.
> 
> [    8.205898] KVM: testing NMI in list_add()
> [    8.205898] KVM: testing NMI in list_del()
> [    8.205899] KVM: found e3
> [    8.205899] KVM: found e2
> [    8.205899] KVM: found e1
> [    8.205900] KVM: found e3
> [    8.205900] KVM: found e1
> 
> static void vmx_test_list(struct list_head *list, struct list_head *e1,
> 			  struct list_head *e2, struct list_head *e3)
> {
> 	struct list_head *tmp;
> 
> 	list_for_each(tmp, list) {
> 		if (tmp == e1)
> 			pr_warn("KVM: found e1\n");
> 		else if (tmp == e2)
> 			pr_warn("KVM: found e2\n");
> 		else if (tmp == e3)
> 			pr_warn("KVM: found e3\n");
> 		else
> 			pr_warn("KVM: kaboom\n");
> 	}
> }
> 
> static int __init vmx_init(void)
> {
> 	LIST_HEAD(list);
> 	LIST_HEAD(e1);
> 	LIST_HEAD(e2);
> 	LIST_HEAD(e3);
> 
> 	pr_warn("KVM: testing NMI in list_add()\n");
> 
> 	list.next->prev = &e1;
> 	vmx_test_list(&list, &e1, &e2, &e3);
> 
> 	e1.next = list.next;
> 	vmx_test_list(&list, &e1, &e2, &e3);
> 
> 	e1.prev = &list;
> 	vmx_test_list(&list, &e1, &e2, &e3);
> 
> 	INIT_LIST_HEAD(&list);
> 	INIT_LIST_HEAD(&e1);
> 
> 	list_add(&e1, &list);
> 	list_add(&e2, &list);
> 	list_add(&e3, &list);
> 
> 	pr_warn("KVM: testing NMI in list_del()\n");
> 
> 	e3.prev = &e1;
> 	vmx_test_list(&list, &e1, &e2, &e3);
> 
> 	list_del(&e2);
> 	list.prev = &e1;
> 	vmx_test_list(&list, &e1, &e2, &e3);
> }
> 
> Sean Christopherson (3):
>   KVM: VMX: Always VMCLEAR in-use VMCSes during crash with kexec support
>   KVM: VMX: Gracefully handle faults on VMXON
>   KVM: VMX: Make loaded_vmcs_init() a static function
> 
>  arch/x86/kvm/vmx/vmx.c | 101 +++++++++++++++++------------------------
>  arch/x86/kvm/vmx/vmx.h |   1 -
>  2 files changed, 41 insertions(+), 61 deletions(-)
> 

Queued, thanks (but still awaiting "counter-review" on the patch 1
suggestions).

Paolo