* [PATCH 0/2] KVM: fix issues with kvm_create_vm failures
@ 2019-11-11 13:25 Paolo Bonzini
2019-11-11 13:25 ` [PATCH 1/2] KVM: Fix NULL-ptr deref after kvm_create_vm fails Paolo Bonzini
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Paolo Bonzini @ 2019-11-11 13:25 UTC (permalink / raw
To: linux-kernel, kvm; +Cc: jmattson, wanpengli, Junaid Shahid
Fix problems with the recent introduction of kvm_arch_destroy_vm
on VM creation failure. An updated version of the patches already
sent by Wanpeng.
Paolo
Paolo Bonzini (2):
KVM: Fix NULL-ptr defer after kvm_create_vm fails
KVM: fix placement of refcount initialization
virt/kvm/kvm_main.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] KVM: Fix NULL-ptr deref after kvm_create_vm fails
2019-11-11 13:25 [PATCH 0/2] KVM: fix issues with kvm_create_vm failures Paolo Bonzini
@ 2019-11-11 13:25 ` Paolo Bonzini
2019-11-12 18:49 ` Jim Mattson
2019-11-11 13:25 ` [PATCH 2/2] KVM: fix placement of refcount initialization Paolo Bonzini
2019-11-12 0:52 ` [PATCH 0/2] KVM: fix issues with kvm_create_vm failures Wanpeng Li
2 siblings, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2019-11-11 13:25 UTC (permalink / raw
To: linux-kernel, kvm; +Cc: jmattson, wanpengli, Junaid Shahid
Reported by syzkaller:
kasan: CONFIG_KASAN_INLINE enabled
kasan: GPF could be caused by NULL-ptr deref or user memory access
general protection fault: 0000 [#1] PREEMPT SMP KASAN
CPU: 0 PID: 14727 Comm: syz-executor.3 Not tainted 5.4.0-rc4+ #0
RIP: 0010:kvm_coalesced_mmio_init+0x5d/0x110 arch/x86/kvm/../../../virt/kvm/coalesced_mmio.c:121
Call Trace:
kvm_dev_ioctl_create_vm arch/x86/kvm/../../../virt/kvm/kvm_main.c:3446 [inline]
kvm_dev_ioctl+0x781/0x1490 arch/x86/kvm/../../../virt/kvm/kvm_main.c:3494
vfs_ioctl fs/ioctl.c:46 [inline]
file_ioctl fs/ioctl.c:509 [inline]
do_vfs_ioctl+0x196/0x1150 fs/ioctl.c:696
ksys_ioctl+0x62/0x90 fs/ioctl.c:713
__do_sys_ioctl fs/ioctl.c:720 [inline]
__se_sys_ioctl fs/ioctl.c:718 [inline]
__x64_sys_ioctl+0x6e/0xb0 fs/ioctl.c:718
do_syscall_64+0xca/0x5d0 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x49/0xbe
Commit 9121923c457d ("kvm: Allocate memslots and buses before calling kvm_arch_init_vm")
moves memslots and buses allocations around, however, if kvm->srcu/irq_srcu fails
initialization, NULL will be returned instead of error code, NULL will not be intercepted
in kvm_dev_ioctl_create_vm() and be deferenced by kvm_coalesced_mmio_init(), this patch
fixes it.
Moving the initialization is required anyway to avoid an incorrect synchronize_srcu that
was also reported by syzkaller:
wait_for_completion+0x29c/0x440 kernel/sched/completion.c:136
__synchronize_srcu+0x197/0x250 kernel/rcu/srcutree.c:921
synchronize_srcu_expedited kernel/rcu/srcutree.c:946 [inline]
synchronize_srcu+0x239/0x3e8 kernel/rcu/srcutree.c:997
kvm_page_track_unregister_notifier+0xe7/0x130 arch/x86/kvm/page_track.c:212
kvm_mmu_uninit_vm+0x1e/0x30 arch/x86/kvm/mmu.c:5828
kvm_arch_destroy_vm+0x4a2/0x5f0 arch/x86/kvm/x86.c:9579
kvm_create_vm arch/x86/kvm/../../../virt/kvm/kvm_main.c:702 [inline]
so do it.
Reported-by: syzbot+89a8060879fa0bd2db4f@syzkaller.appspotmail.com
Reported-by: syzbot+e27e7027eb2b80e44225@syzkaller.appspotmail.com
Fixes: 9121923c457d ("kvm: Allocate memslots and buses before calling kvm_arch_init_vm")
Cc: Jim Mattson <jmattson@google.com>
Cc: Wanpeng Li <wanpengli@tencent.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
virt/kvm/kvm_main.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index d6f0696d98ef..e22ff63e5b1a 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -645,6 +645,11 @@ static struct kvm *kvm_create_vm(unsigned long type)
BUILD_BUG_ON(KVM_MEM_SLOTS_NUM > SHRT_MAX);
+ if (init_srcu_struct(&kvm->srcu))
+ goto out_err_no_srcu;
+ if (init_srcu_struct(&kvm->irq_srcu))
+ goto out_err_no_irq_srcu;
+
for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++) {
struct kvm_memslots *slots = kvm_alloc_memslots();
@@ -675,11 +680,6 @@ static struct kvm *kvm_create_vm(unsigned long type)
INIT_HLIST_HEAD(&kvm->irq_ack_notifier_list);
#endif
- if (init_srcu_struct(&kvm->srcu))
- goto out_err_no_srcu;
- if (init_srcu_struct(&kvm->irq_srcu))
- goto out_err_no_irq_srcu;
-
r = kvm_init_mmu_notifier(kvm);
if (r)
goto out_err;
@@ -693,10 +693,6 @@ static struct kvm *kvm_create_vm(unsigned long type)
return kvm;
out_err:
- cleanup_srcu_struct(&kvm->irq_srcu);
-out_err_no_irq_srcu:
- cleanup_srcu_struct(&kvm->srcu);
-out_err_no_srcu:
hardware_disable_all();
out_err_no_disable:
kvm_arch_destroy_vm(kvm);
@@ -706,6 +702,10 @@ static struct kvm *kvm_create_vm(unsigned long type)
kfree(kvm_get_bus(kvm, i));
for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++)
kvm_free_memslots(kvm, __kvm_memslots(kvm, i));
+ cleanup_srcu_struct(&kvm->irq_srcu);
+out_err_no_irq_srcu:
+ cleanup_srcu_struct(&kvm->srcu);
+out_err_no_srcu:
kvm_arch_free_vm(kvm);
mmdrop(current->mm);
return ERR_PTR(r);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] KVM: fix placement of refcount initialization
2019-11-11 13:25 [PATCH 0/2] KVM: fix issues with kvm_create_vm failures Paolo Bonzini
2019-11-11 13:25 ` [PATCH 1/2] KVM: Fix NULL-ptr deref after kvm_create_vm fails Paolo Bonzini
@ 2019-11-11 13:25 ` Paolo Bonzini
2019-11-12 18:50 ` Jim Mattson
2019-11-12 0:52 ` [PATCH 0/2] KVM: fix issues with kvm_create_vm failures Wanpeng Li
2 siblings, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2019-11-11 13:25 UTC (permalink / raw
To: linux-kernel, kvm; +Cc: jmattson, wanpengli, Junaid Shahid
Reported by syzkaller:
=============================
WARNING: suspicious RCU usage
-----------------------------
./include/linux/kvm_host.h:536 suspicious rcu_dereference_check() usage!
other info that might help us debug this:
rcu_scheduler_active = 2, debug_locks = 1
no locks held by repro_11/12688.
stack backtrace:
Call Trace:
dump_stack+0x7d/0xc5
lockdep_rcu_suspicious+0x123/0x170
kvm_dev_ioctl+0x9a9/0x1260 [kvm]
do_vfs_ioctl+0x1a1/0xfb0
ksys_ioctl+0x6d/0x80
__x64_sys_ioctl+0x73/0xb0
do_syscall_64+0x108/0xaa0
entry_SYSCALL_64_after_hwframe+0x49/0xbe
Commit a97b0e773e4 (kvm: call kvm_arch_destroy_vm if vm creation fails)
sets users_count to 1 before kvm_arch_init_vm(), however, if kvm_arch_init_vm()
fails, we need to decrease this count. By moving it earlier, we can push
the decrease to out_err_no_arch_destroy_vm without introducing yet another
error label.
syzkaller source: https://syzkaller.appspot.com/x/repro.c?x=15209b84e00000
Reported-by: syzbot+75475908cd0910f141ee@syzkaller.appspotmail.com
Fixes: a97b0e773e49 ("kvm: call kvm_arch_destroy_vm if vm creation fails")
Cc: Jim Mattson <jmattson@google.com>
Analyzed-by: Wanpeng Li <wanpengli@tencent.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
virt/kvm/kvm_main.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index e22ff63e5b1a..e7a07132cd7f 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -650,6 +650,7 @@ static struct kvm *kvm_create_vm(unsigned long type)
if (init_srcu_struct(&kvm->irq_srcu))
goto out_err_no_irq_srcu;
+ refcount_set(&kvm->users_count, 1);
for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++) {
struct kvm_memslots *slots = kvm_alloc_memslots();
@@ -667,7 +668,6 @@ static struct kvm *kvm_create_vm(unsigned long type)
goto out_err_no_arch_destroy_vm;
}
- refcount_set(&kvm->users_count, 1);
r = kvm_arch_init_vm(kvm, type);
if (r)
goto out_err_no_arch_destroy_vm;
@@ -696,8 +696,8 @@ static struct kvm *kvm_create_vm(unsigned long type)
hardware_disable_all();
out_err_no_disable:
kvm_arch_destroy_vm(kvm);
- WARN_ON_ONCE(!refcount_dec_and_test(&kvm->users_count));
out_err_no_arch_destroy_vm:
+ WARN_ON_ONCE(!refcount_dec_and_test(&kvm->users_count));
for (i = 0; i < KVM_NR_BUSES; i++)
kfree(kvm_get_bus(kvm, i));
for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] KVM: fix issues with kvm_create_vm failures
2019-11-11 13:25 [PATCH 0/2] KVM: fix issues with kvm_create_vm failures Paolo Bonzini
2019-11-11 13:25 ` [PATCH 1/2] KVM: Fix NULL-ptr deref after kvm_create_vm fails Paolo Bonzini
2019-11-11 13:25 ` [PATCH 2/2] KVM: fix placement of refcount initialization Paolo Bonzini
@ 2019-11-12 0:52 ` Wanpeng Li
2 siblings, 0 replies; 6+ messages in thread
From: Wanpeng Li @ 2019-11-12 0:52 UTC (permalink / raw
To: Paolo Bonzini; +Cc: LKML, kvm, Jim Mattson, Wanpeng Li, Junaid Shahid
On Mon, 11 Nov 2019 at 21:26, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Fix problems with the recent introduction of kvm_arch_destroy_vm
> on VM creation failure. An updated version of the patches already
> sent by Wanpeng.
>
> Paolo
>
> Paolo Bonzini (2):
> KVM: Fix NULL-ptr defer after kvm_create_vm fails
> KVM: fix placement of refcount initialization
Looks good to me.
Wanpeng
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] KVM: Fix NULL-ptr deref after kvm_create_vm fails
2019-11-11 13:25 ` [PATCH 1/2] KVM: Fix NULL-ptr deref after kvm_create_vm fails Paolo Bonzini
@ 2019-11-12 18:49 ` Jim Mattson
0 siblings, 0 replies; 6+ messages in thread
From: Jim Mattson @ 2019-11-12 18:49 UTC (permalink / raw
To: Paolo Bonzini; +Cc: LKML, kvm list, Wanpeng Li, Junaid Shahid
On Mon, Nov 11, 2019 at 5:25 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Reported by syzkaller:
>
> kasan: CONFIG_KASAN_INLINE enabled
> kasan: GPF could be caused by NULL-ptr deref or user memory access
> general protection fault: 0000 [#1] PREEMPT SMP KASAN
> CPU: 0 PID: 14727 Comm: syz-executor.3 Not tainted 5.4.0-rc4+ #0
> RIP: 0010:kvm_coalesced_mmio_init+0x5d/0x110 arch/x86/kvm/../../../virt/kvm/coalesced_mmio.c:121
> Call Trace:
> kvm_dev_ioctl_create_vm arch/x86/kvm/../../../virt/kvm/kvm_main.c:3446 [inline]
> kvm_dev_ioctl+0x781/0x1490 arch/x86/kvm/../../../virt/kvm/kvm_main.c:3494
> vfs_ioctl fs/ioctl.c:46 [inline]
> file_ioctl fs/ioctl.c:509 [inline]
> do_vfs_ioctl+0x196/0x1150 fs/ioctl.c:696
> ksys_ioctl+0x62/0x90 fs/ioctl.c:713
> __do_sys_ioctl fs/ioctl.c:720 [inline]
> __se_sys_ioctl fs/ioctl.c:718 [inline]
> __x64_sys_ioctl+0x6e/0xb0 fs/ioctl.c:718
> do_syscall_64+0xca/0x5d0 arch/x86/entry/common.c:290
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> Commit 9121923c457d ("kvm: Allocate memslots and buses before calling kvm_arch_init_vm")
> moves memslots and buses allocations around, however, if kvm->srcu/irq_srcu fails
> initialization, NULL will be returned instead of error code, NULL will not be intercepted
> in kvm_dev_ioctl_create_vm() and be deferenced by kvm_coalesced_mmio_init(), this patch
> fixes it.
>
> Moving the initialization is required anyway to avoid an incorrect synchronize_srcu that
> was also reported by syzkaller:
>
> wait_for_completion+0x29c/0x440 kernel/sched/completion.c:136
> __synchronize_srcu+0x197/0x250 kernel/rcu/srcutree.c:921
> synchronize_srcu_expedited kernel/rcu/srcutree.c:946 [inline]
> synchronize_srcu+0x239/0x3e8 kernel/rcu/srcutree.c:997
> kvm_page_track_unregister_notifier+0xe7/0x130 arch/x86/kvm/page_track.c:212
> kvm_mmu_uninit_vm+0x1e/0x30 arch/x86/kvm/mmu.c:5828
> kvm_arch_destroy_vm+0x4a2/0x5f0 arch/x86/kvm/x86.c:9579
> kvm_create_vm arch/x86/kvm/../../../virt/kvm/kvm_main.c:702 [inline]
>
> so do it.
Thanks for doing this!
> Reported-by: syzbot+89a8060879fa0bd2db4f@syzkaller.appspotmail.com
> Reported-by: syzbot+e27e7027eb2b80e44225@syzkaller.appspotmail.com
> Fixes: 9121923c457d ("kvm: Allocate memslots and buses before calling kvm_arch_init_vm")
> Cc: Jim Mattson <jmattson@google.com>
> Cc: Wanpeng Li <wanpengli@tencent.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> virt/kvm/kvm_main.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index d6f0696d98ef..e22ff63e5b1a 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -645,6 +645,11 @@ static struct kvm *kvm_create_vm(unsigned long type)
>
> BUILD_BUG_ON(KVM_MEM_SLOTS_NUM > SHRT_MAX);
Nit: I would keep the BUILD_BUG_ON closer to the memslot allocation.
> + if (init_srcu_struct(&kvm->srcu))
> + goto out_err_no_srcu;
> + if (init_srcu_struct(&kvm->irq_srcu))
> + goto out_err_no_irq_srcu;
> +
> for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++) {
> struct kvm_memslots *slots = kvm_alloc_memslots();
>
> @@ -675,11 +680,6 @@ static struct kvm *kvm_create_vm(unsigned long type)
> INIT_HLIST_HEAD(&kvm->irq_ack_notifier_list);
> #endif
>
> - if (init_srcu_struct(&kvm->srcu))
> - goto out_err_no_srcu;
> - if (init_srcu_struct(&kvm->irq_srcu))
> - goto out_err_no_irq_srcu;
> -
> r = kvm_init_mmu_notifier(kvm);
> if (r)
> goto out_err;
> @@ -693,10 +693,6 @@ static struct kvm *kvm_create_vm(unsigned long type)
> return kvm;
>
> out_err:
> - cleanup_srcu_struct(&kvm->irq_srcu);
> -out_err_no_irq_srcu:
> - cleanup_srcu_struct(&kvm->srcu);
> -out_err_no_srcu:
> hardware_disable_all();
> out_err_no_disable:
> kvm_arch_destroy_vm(kvm);
> @@ -706,6 +702,10 @@ static struct kvm *kvm_create_vm(unsigned long type)
> kfree(kvm_get_bus(kvm, i));
> for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++)
> kvm_free_memslots(kvm, __kvm_memslots(kvm, i));
> + cleanup_srcu_struct(&kvm->irq_srcu);
> +out_err_no_irq_srcu:
> + cleanup_srcu_struct(&kvm->srcu);
> +out_err_no_srcu:
> kvm_arch_free_vm(kvm);
> mmdrop(current->mm);
> return ERR_PTR(r);
> --
> 1.8.3.1
Reviewed-by: Jim Mattson <jmattson@google.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] KVM: fix placement of refcount initialization
2019-11-11 13:25 ` [PATCH 2/2] KVM: fix placement of refcount initialization Paolo Bonzini
@ 2019-11-12 18:50 ` Jim Mattson
0 siblings, 0 replies; 6+ messages in thread
From: Jim Mattson @ 2019-11-12 18:50 UTC (permalink / raw
To: Paolo Bonzini; +Cc: LKML, kvm list, Wanpeng Li, Junaid Shahid
On Mon, Nov 11, 2019 at 5:25 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Reported by syzkaller:
>
> =============================
> WARNING: suspicious RCU usage
> -----------------------------
> ./include/linux/kvm_host.h:536 suspicious rcu_dereference_check() usage!
>
> other info that might help us debug this:
>
> rcu_scheduler_active = 2, debug_locks = 1
> no locks held by repro_11/12688.
>
> stack backtrace:
> Call Trace:
> dump_stack+0x7d/0xc5
> lockdep_rcu_suspicious+0x123/0x170
> kvm_dev_ioctl+0x9a9/0x1260 [kvm]
> do_vfs_ioctl+0x1a1/0xfb0
> ksys_ioctl+0x6d/0x80
> __x64_sys_ioctl+0x73/0xb0
> do_syscall_64+0x108/0xaa0
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> Commit a97b0e773e4 (kvm: call kvm_arch_destroy_vm if vm creation fails)
> sets users_count to 1 before kvm_arch_init_vm(), however, if kvm_arch_init_vm()
> fails, we need to decrease this count. By moving it earlier, we can push
> the decrease to out_err_no_arch_destroy_vm without introducing yet another
> error label.
>
> syzkaller source: https://syzkaller.appspot.com/x/repro.c?x=15209b84e00000
>
> Reported-by: syzbot+75475908cd0910f141ee@syzkaller.appspotmail.com
> Fixes: a97b0e773e49 ("kvm: call kvm_arch_destroy_vm if vm creation fails")
> Cc: Jim Mattson <jmattson@google.com>
> Analyzed-by: Wanpeng Li <wanpengli@tencent.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Jim Mattson <jmattson@google.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-11-12 18:50 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-11-11 13:25 [PATCH 0/2] KVM: fix issues with kvm_create_vm failures Paolo Bonzini
2019-11-11 13:25 ` [PATCH 1/2] KVM: Fix NULL-ptr deref after kvm_create_vm fails Paolo Bonzini
2019-11-12 18:49 ` Jim Mattson
2019-11-11 13:25 ` [PATCH 2/2] KVM: fix placement of refcount initialization Paolo Bonzini
2019-11-12 18:50 ` Jim Mattson
2019-11-12 0:52 ` [PATCH 0/2] KVM: fix issues with kvm_create_vm failures Wanpeng Li
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.