linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] KVM: Fix NULL-ptr defer after kvm_create_vm fails
@ 2019-11-04  6:27 Wanpeng Li
  2019-11-04  6:27 ` [PATCH 2/2] KVM: Fix rcu splat if vm creation fails Wanpeng Li
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Wanpeng Li @ 2019-11-04  6:27 UTC (permalink / raw)
  To: linux-kernel, kvm
  Cc: Paolo Bonzini, Radim Krčmář,
	Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel

From: Wanpeng Li <wanpengli@tencent.com>

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.

syz repro: https://syzkaller.appspot.com/x/repro.syz?x=13509b84e00000

Reported-by: syzbot+89a8060879fa0bd2db4f@syzkaller.appspotmail.com
Fixes: 9121923c457d ("kvm: Allocate memslots and buses before calling kvm_arch_init_vm") 
Cc: Jim Mattson <jmattson@google.com>
Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
---
 virt/kvm/kvm_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index d6f0696..8c272eb 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -675,6 +675,7 @@ static struct kvm *kvm_create_vm(unsigned long type)
 	INIT_HLIST_HEAD(&kvm->irq_ack_notifier_list);
 #endif
 
+	r = -ENOMEM;
 	if (init_srcu_struct(&kvm->srcu))
 		goto out_err_no_srcu;
 	if (init_srcu_struct(&kvm->irq_srcu))
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/2] KVM: Fix rcu splat if vm creation fails
  2019-11-04  6:27 [PATCH 1/2] KVM: Fix NULL-ptr defer after kvm_create_vm fails Wanpeng Li
@ 2019-11-04  6:27 ` Wanpeng Li
  2019-11-04 11:18   ` Paolo Bonzini
  2019-11-04 11:19 ` [PATCH 1/2] KVM: Fix NULL-ptr defer after kvm_create_vm fails Paolo Bonzini
  2019-11-06  8:29 ` Christian Borntraeger
  2 siblings, 1 reply; 9+ messages in thread
From: Wanpeng Li @ 2019-11-04  6:27 UTC (permalink / raw)
  To: linux-kernel, kvm
  Cc: Paolo Bonzini, Radim Krčmář,
	Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel

From: Wanpeng Li <wanpengli@tencent.com>

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 dec this count. Or, we can move the sets refcount after 
kvm_arch_init_vm().

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>
Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
---
 virt/kvm/kvm_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index d6f0696..62ae0c9 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -662,11 +662,11 @@ 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;
 
+	refcount_set(&kvm->users_count, 1);
 	r = hardware_enable_all();
 	if (r)
 		goto out_err_no_disable;
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] KVM: Fix rcu splat if vm creation fails
  2019-11-04  6:27 ` [PATCH 2/2] KVM: Fix rcu splat if vm creation fails Wanpeng Li
@ 2019-11-04 11:18   ` Paolo Bonzini
  2019-11-04 12:16     ` Wanpeng Li
  0 siblings, 1 reply; 9+ messages in thread
From: Paolo Bonzini @ 2019-11-04 11:18 UTC (permalink / raw)
  To: Wanpeng Li, linux-kernel, kvm
  Cc: Radim Krčmář,
	Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel

On 04/11/19 07:27, Wanpeng Li wrote:
> From: Wanpeng Li <wanpengli@tencent.com>
> 
> 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 dec this count. Or, we can move the sets refcount after 
> kvm_arch_init_vm().

I don't understand this one, hasn't

        WARN_ON_ONCE(!refcount_dec_and_test(&kvm->users_count));

decreased the conut already?  With your patch the refcount would then
underflow.

Paolo

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] KVM: Fix NULL-ptr defer after kvm_create_vm fails
  2019-11-04  6:27 [PATCH 1/2] KVM: Fix NULL-ptr defer after kvm_create_vm fails Wanpeng Li
  2019-11-04  6:27 ` [PATCH 2/2] KVM: Fix rcu splat if vm creation fails Wanpeng Li
@ 2019-11-04 11:19 ` Paolo Bonzini
  2019-11-06  8:29 ` Christian Borntraeger
  2 siblings, 0 replies; 9+ messages in thread
From: Paolo Bonzini @ 2019-11-04 11:19 UTC (permalink / raw)
  To: Wanpeng Li, linux-kernel, kvm
  Cc: Radim Krčmář,
	Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel

On 04/11/19 07:27, Wanpeng Li wrote:
> 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.

This is not enough, as syzkaller also reported 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]

I'm thinking of something like this (not tested yet):

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);

Paolo

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] KVM: Fix rcu splat if vm creation fails
  2019-11-04 11:18   ` Paolo Bonzini
@ 2019-11-04 12:16     ` Wanpeng Li
  2019-11-04 12:21       ` Paolo Bonzini
  0 siblings, 1 reply; 9+ messages in thread
From: Wanpeng Li @ 2019-11-04 12:16 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: LKML, kvm, Radim Krčmář,
	Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel

On Mon, 4 Nov 2019 at 19:18, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 04/11/19 07:27, Wanpeng Li wrote:
> > From: Wanpeng Li <wanpengli@tencent.com>
> >
> > 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 dec this count. Or, we can move the sets refcount after
> > kvm_arch_init_vm().
>
> I don't understand this one, hasn't
>
>         WARN_ON_ONCE(!refcount_dec_and_test(&kvm->users_count));
>
> decreased the conut already?  With your patch the refcount would then
> underflow.

r = kvm_arch_init_vm(kvm, type);
if (r)
    goto out_err_no_arch_destroy_vm;

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:

So, if kvm_arch_init_vm() fails, we will not execute
WARN_ON_ONCE(!refcount_dec_and_test(&kvm->users_count));

    Wanpeng

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] KVM: Fix rcu splat if vm creation fails
  2019-11-04 12:16     ` Wanpeng Li
@ 2019-11-04 12:21       ` Paolo Bonzini
  2019-11-04 12:25         ` Wanpeng Li
  0 siblings, 1 reply; 9+ messages in thread
From: Paolo Bonzini @ 2019-11-04 12:21 UTC (permalink / raw)
  To: Wanpeng Li
  Cc: LKML, kvm, Radim Krčmář,
	Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel

On 04/11/19 13:16, Wanpeng Li wrote:
>> I don't understand this one, hasn't
>>
>>         WARN_ON_ONCE(!refcount_dec_and_test(&kvm->users_count));
>>
>> decreased the conut already?  With your patch the refcount would then
>> underflow.
> 
> r = kvm_arch_init_vm(kvm, type);
> if (r)
>     goto out_err_no_arch_destroy_vm;
> 
> 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:
> 
> So, if kvm_arch_init_vm() fails, we will not execute
> WARN_ON_ONCE(!refcount_dec_and_test(&kvm->users_count));

Uuh of course.  But I'd rather do the opposite: move the refcount_set
earlier so that refcount_dec_and_test can be moved after
no_arch_destroy_vm.  Moving the refcount_set is not strictly necessary,
but avoids the introduction of yet another label.

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++)



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] KVM: Fix rcu splat if vm creation fails
  2019-11-04 12:21       ` Paolo Bonzini
@ 2019-11-04 12:25         ` Wanpeng Li
  2019-11-04 12:26           ` Paolo Bonzini
  0 siblings, 1 reply; 9+ messages in thread
From: Wanpeng Li @ 2019-11-04 12:25 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: LKML, kvm, Radim Krčmář,
	Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel

On Mon, 4 Nov 2019 at 20:21, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 04/11/19 13:16, Wanpeng Li wrote:
> >> I don't understand this one, hasn't
> >>
> >>         WARN_ON_ONCE(!refcount_dec_and_test(&kvm->users_count));
> >>
> >> decreased the conut already?  With your patch the refcount would then
> >> underflow.
> >
> > r = kvm_arch_init_vm(kvm, type);
> > if (r)
> >     goto out_err_no_arch_destroy_vm;
> >
> > 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:
> >
> > So, if kvm_arch_init_vm() fails, we will not execute
> > WARN_ON_ONCE(!refcount_dec_and_test(&kvm->users_count));
>
> Uuh of course.  But I'd rather do the opposite: move the refcount_set
> earlier so that refcount_dec_and_test can be moved after
> no_arch_destroy_vm.  Moving the refcount_set is not strictly necessary,
> but avoids the introduction of yet another label.
>
> 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++)

Good point, I will handle these two patches later.

    Wanpeng

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] KVM: Fix rcu splat if vm creation fails
  2019-11-04 12:25         ` Wanpeng Li
@ 2019-11-04 12:26           ` Paolo Bonzini
  0 siblings, 0 replies; 9+ messages in thread
From: Paolo Bonzini @ 2019-11-04 12:26 UTC (permalink / raw)
  To: Wanpeng Li
  Cc: LKML, kvm, Radim Krčmář,
	Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel

On 04/11/19 13:25, Wanpeng Li wrote:
> On Mon, 4 Nov 2019 at 20:21, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> On 04/11/19 13:16, Wanpeng Li wrote:
>>>> I don't understand this one, hasn't
>>>>
>>>>         WARN_ON_ONCE(!refcount_dec_and_test(&kvm->users_count));
>>>>
>>>> decreased the conut already?  With your patch the refcount would then
>>>> underflow.
>>>
>>> r = kvm_arch_init_vm(kvm, type);
>>> if (r)
>>>     goto out_err_no_arch_destroy_vm;
>>>
>>> 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:
>>>
>>> So, if kvm_arch_init_vm() fails, we will not execute
>>> WARN_ON_ONCE(!refcount_dec_and_test(&kvm->users_count));
>>
>> Uuh of course.  But I'd rather do the opposite: move the refcount_set
>> earlier so that refcount_dec_and_test can be moved after
>> no_arch_destroy_vm.  Moving the refcount_set is not strictly necessary,
>> but avoids the introduction of yet another label.
>>
>> 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++)
> 
> Good point, I will handle these two patches later.

No problem, I can send v2 after testing.

Paolo


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] KVM: Fix NULL-ptr defer after kvm_create_vm fails
  2019-11-04  6:27 [PATCH 1/2] KVM: Fix NULL-ptr defer after kvm_create_vm fails Wanpeng Li
  2019-11-04  6:27 ` [PATCH 2/2] KVM: Fix rcu splat if vm creation fails Wanpeng Li
  2019-11-04 11:19 ` [PATCH 1/2] KVM: Fix NULL-ptr defer after kvm_create_vm fails Paolo Bonzini
@ 2019-11-06  8:29 ` Christian Borntraeger
  2 siblings, 0 replies; 9+ messages in thread
From: Christian Borntraeger @ 2019-11-06  8:29 UTC (permalink / raw)
  To: Wanpeng Li, linux-kernel, kvm
  Cc: Paolo Bonzini, Radim Krčmář,
	Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel



On 04.11.19 07:27, Wanpeng Li wrote:
> From: Wanpeng Li <wanpengli@tencent.com>
> 
> 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.
> 
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=13509b84e00000
> 
> Reported-by: syzbot+89a8060879fa0bd2db4f@syzkaller.appspotmail.com
> Fixes: 9121923c457d ("kvm: Allocate memslots and buses before calling kvm_arch_init_vm") 
> Cc: Jim Mattson <jmattson@google.com>
> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>

Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>

> ---
>  virt/kvm/kvm_main.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index d6f0696..8c272eb 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -675,6 +675,7 @@ static struct kvm *kvm_create_vm(unsigned long type)
>  	INIT_HLIST_HEAD(&kvm->irq_ack_notifier_list);
>  #endif
> 
> +	r = -ENOMEM;
>  	if (init_srcu_struct(&kvm->srcu))
>  		goto out_err_no_srcu;
>  	if (init_srcu_struct(&kvm->irq_srcu))
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-11-06  8:31 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-04  6:27 [PATCH 1/2] KVM: Fix NULL-ptr defer after kvm_create_vm fails Wanpeng Li
2019-11-04  6:27 ` [PATCH 2/2] KVM: Fix rcu splat if vm creation fails Wanpeng Li
2019-11-04 11:18   ` Paolo Bonzini
2019-11-04 12:16     ` Wanpeng Li
2019-11-04 12:21       ` Paolo Bonzini
2019-11-04 12:25         ` Wanpeng Li
2019-11-04 12:26           ` Paolo Bonzini
2019-11-04 11:19 ` [PATCH 1/2] KVM: Fix NULL-ptr defer after kvm_create_vm fails Paolo Bonzini
2019-11-06  8:29 ` Christian Borntraeger

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).