* [PATCH] KVM: ioapic: fix NULL deref ioapic->lock
@ 2017-01-01 3:44 Wanpeng Li
2017-01-02 10:09 ` Paolo Bonzini
0 siblings, 1 reply; 10+ messages in thread
From: Wanpeng Li @ 2017-01-01 3:44 UTC (permalink / raw)
To: linux-kernel, kvm
Cc: Paolo Bonzini, Radim Krčmář, Dmitry Vyukov, Wanpeng Li
From: Wanpeng Li <wanpeng.li@hotmail.com>
This was reported by syzkaller:
BUG: unable to handle kernel NULL pointer dereference at 00000000000001b0
IP: _raw_spin_lock+0xc/0x30
PGD 3e28eb067
PUD 3f0ac6067
PMD 0
Oops: 0002 [#1] SMP
CPU: 0 PID: 2431 Comm: test Tainted: G OE 4.10.0-rc1+ #3
Call Trace:
? kvm_ioapic_scan_entry+0x3e/0x110 [kvm]
kvm_arch_vcpu_ioctl_run+0x10a8/0x15f0 [kvm]
? pick_next_task_fair+0xe1/0x4e0
? kvm_arch_vcpu_load+0xea/0x260 [kvm]
kvm_vcpu_ioctl+0x33a/0x600 [kvm]
? hrtimer_try_to_cancel+0x29/0x130
? do_nanosleep+0x97/0xf0
do_vfs_ioctl+0xa1/0x5d0
? __hrtimer_init+0x90/0x90
? do_nanosleep+0x5b/0xf0
SyS_ioctl+0x79/0x90
do_syscall_64+0x6e/0x180
entry_SYSCALL64_slow_path+0x25/0x25
RIP: _raw_spin_lock+0xc/0x30 RSP: ffffa43688973cc0
KVM will skip over create pic/ioapic if there is a created vCPU. However,
there is no guarantee whether ioapic is present when rescan ioapic which
results in NULL dereference ioapic->lock. This patch fix it by adding the
ioapic present check to ioapic scan.
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
arch/x86/kvm/x86.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 51ccfe0..9ca175c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6556,7 +6556,8 @@ static void vcpu_scan_ioapic(struct kvm_vcpu *vcpu)
else {
if (vcpu->arch.apicv_active)
kvm_x86_ops->sync_pir_to_irr(vcpu);
- kvm_ioapic_scan_entry(vcpu, vcpu->arch.ioapic_handled_vectors);
+ if (ioapic_irqchip(vcpu->kvm))
+ kvm_ioapic_scan_entry(vcpu, vcpu->arch.ioapic_handled_vectors);
}
bitmap_or((ulong *)eoi_exit_bitmap, vcpu->arch.ioapic_handled_vectors,
vcpu_to_synic(vcpu)->vec_bitmap, 256);
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] KVM: ioapic: fix NULL deref ioapic->lock
2017-01-01 3:44 [PATCH] KVM: ioapic: fix NULL deref ioapic->lock Wanpeng Li
@ 2017-01-02 10:09 ` Paolo Bonzini
2017-01-02 10:17 ` Dmitry Vyukov
0 siblings, 1 reply; 10+ messages in thread
From: Paolo Bonzini @ 2017-01-02 10:09 UTC (permalink / raw)
To: Wanpeng Li, linux-kernel, kvm
Cc: Radim Krčmář, Dmitry Vyukov, Wanpeng Li
On 01/01/2017 04:44, Wanpeng Li wrote:
> From: Wanpeng Li <wanpeng.li@hotmail.com>
>
> This was reported by syzkaller:
>
> BUG: unable to handle kernel NULL pointer dereference at 00000000000001b0
> IP: _raw_spin_lock+0xc/0x30
> PGD 3e28eb067
> PUD 3f0ac6067
> PMD 0
> Oops: 0002 [#1] SMP
> CPU: 0 PID: 2431 Comm: test Tainted: G OE 4.10.0-rc1+ #3
> Call Trace:
> ? kvm_ioapic_scan_entry+0x3e/0x110 [kvm]
> kvm_arch_vcpu_ioctl_run+0x10a8/0x15f0 [kvm]
> ? pick_next_task_fair+0xe1/0x4e0
> ? kvm_arch_vcpu_load+0xea/0x260 [kvm]
> kvm_vcpu_ioctl+0x33a/0x600 [kvm]
> ? hrtimer_try_to_cancel+0x29/0x130
> ? do_nanosleep+0x97/0xf0
> do_vfs_ioctl+0xa1/0x5d0
> ? __hrtimer_init+0x90/0x90
> ? do_nanosleep+0x5b/0xf0
> SyS_ioctl+0x79/0x90
> do_syscall_64+0x6e/0x180
> entry_SYSCALL64_slow_path+0x25/0x25
> RIP: _raw_spin_lock+0xc/0x30 RSP: ffffa43688973cc0
>
> KVM will skip over create pic/ioapic if there is a created vCPU. However,
> there is no guarantee whether ioapic is present when rescan ioapic which
> results in NULL dereference ioapic->lock. This patch fix it by adding the
> ioapic present check to ioapic scan.
>
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> ---
> arch/x86/kvm/x86.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 51ccfe0..9ca175c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6556,7 +6556,8 @@ static void vcpu_scan_ioapic(struct kvm_vcpu *vcpu)
> else {
> if (vcpu->arch.apicv_active)
> kvm_x86_ops->sync_pir_to_irr(vcpu);
> - kvm_ioapic_scan_entry(vcpu, vcpu->arch.ioapic_handled_vectors);
> + if (ioapic_irqchip(vcpu->kvm))
> + kvm_ioapic_scan_entry(vcpu, vcpu->arch.ioapic_handled_vectors);
> }
> bitmap_or((ulong *)eoi_exit_bitmap, vcpu->arch.ioapic_handled_vectors,
> vcpu_to_synic(vcpu)->vec_bitmap, 256);
Commit message for fuzzing bugs have usually included a beautified
reproducer. However, you are not even saying if it is a race, or it is
deterministic.
The fix seems wrong to me at first impression, because "LAPIC enabled"
and "irqchip not split" should imply the existence of an in-kernel
IOAPIC. However, I cannot suggest the right course of action without
seeing a testcase.
Paolo
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] KVM: ioapic: fix NULL deref ioapic->lock
2017-01-02 10:09 ` Paolo Bonzini
@ 2017-01-02 10:17 ` Dmitry Vyukov
2017-01-02 18:01 ` Paolo Bonzini
0 siblings, 1 reply; 10+ messages in thread
From: Dmitry Vyukov @ 2017-01-02 10:17 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Wanpeng Li, LKML, KVM list, Radim Krčmář, Wanpeng Li
On Mon, Jan 2, 2017 at 11:09 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
>
> On 01/01/2017 04:44, Wanpeng Li wrote:
> > From: Wanpeng Li <wanpeng.li@hotmail.com>
> >
> > This was reported by syzkaller:
> >
> > BUG: unable to handle kernel NULL pointer dereference at 00000000000001b0
> > IP: _raw_spin_lock+0xc/0x30
> > PGD 3e28eb067
> > PUD 3f0ac6067
> > PMD 0
> > Oops: 0002 [#1] SMP
> > CPU: 0 PID: 2431 Comm: test Tainted: G OE 4.10.0-rc1+ #3
> > Call Trace:
> > ? kvm_ioapic_scan_entry+0x3e/0x110 [kvm]
> > kvm_arch_vcpu_ioctl_run+0x10a8/0x15f0 [kvm]
> > ? pick_next_task_fair+0xe1/0x4e0
> > ? kvm_arch_vcpu_load+0xea/0x260 [kvm]
> > kvm_vcpu_ioctl+0x33a/0x600 [kvm]
> > ? hrtimer_try_to_cancel+0x29/0x130
> > ? do_nanosleep+0x97/0xf0
> > do_vfs_ioctl+0xa1/0x5d0
> > ? __hrtimer_init+0x90/0x90
> > ? do_nanosleep+0x5b/0xf0
> > SyS_ioctl+0x79/0x90
> > do_syscall_64+0x6e/0x180
> > entry_SYSCALL64_slow_path+0x25/0x25
> > RIP: _raw_spin_lock+0xc/0x30 RSP: ffffa43688973cc0
> >
> > KVM will skip over create pic/ioapic if there is a created vCPU. However,
> > there is no guarantee whether ioapic is present when rescan ioapic which
> > results in NULL dereference ioapic->lock. This patch fix it by adding the
> > ioapic present check to ioapic scan.
> >
> > Reported-by: Dmitry Vyukov <dvyukov@google.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Radim Krčmář <rkrcmar@redhat.com>
> > Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> > ---
> > arch/x86/kvm/x86.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 51ccfe0..9ca175c 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -6556,7 +6556,8 @@ static void vcpu_scan_ioapic(struct kvm_vcpu *vcpu)
> > else {
> > if (vcpu->arch.apicv_active)
> > kvm_x86_ops->sync_pir_to_irr(vcpu);
> > - kvm_ioapic_scan_entry(vcpu, vcpu->arch.ioapic_handled_vectors);
> > + if (ioapic_irqchip(vcpu->kvm))
> > + kvm_ioapic_scan_entry(vcpu, vcpu->arch.ioapic_handled_vectors);
> > }
> > bitmap_or((ulong *)eoi_exit_bitmap, vcpu->arch.ioapic_handled_vectors,
> > vcpu_to_synic(vcpu)->vec_bitmap, 256);
>
> Commit message for fuzzing bugs have usually included a beautified
> reproducer. However, you are not even saying if it is a race, or it is
> deterministic.
>
> The fix seems wrong to me at first impression, because "LAPIC enabled"
> and "irqchip not split" should imply the existence of an in-kernel
> IOAPIC. However, I cannot suggest the right course of action without
> seeing a testcase.
I've created a reasonably beautified reproducer here:
https://groups.google.com/forum/#!msg/syzkaller/6V-KXaMDYi8/rOvBl-69DAAJ
FWIW the patch has fixed the crash, but there is another similar one
that happens slightly earlier:
general protection fault: 0000 [#1] SMP KASAN
Dumping ftrace buffer:
(ftrace buffer empty)
Modules linked in:
CPU: 1 PID: 17462 Comm: syz-executor4 Not tainted 4.10.0-rc1+ #119
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
task: ffff88003d6a2280 task.stack: ffff8800357e8000
RIP: 0010:kvm_apic_hw_enabled arch/x86/kvm/lapic.h:156 [inline]
RIP: 0010:vcpu_scan_ioapic arch/x86/kvm/x86.c:6559 [inline]
RIP: 0010:vcpu_enter_guest arch/x86/kvm/x86.c:6691 [inline]
RIP: 0010:vcpu_run arch/x86/kvm/x86.c:6944 [inline]
RIP: 0010:kvm_arch_vcpu_ioctl_run+0x272c/0x4660 arch/x86/kvm/x86.c:7102
RSP: 0018:ffff8800357ef7d8 EFLAGS: 00010206
RAX: 0000000000000000 RBX: ffff88003a459170 RCX: ffffc90000a76000
RDX: 0000000000000014 RSI: ffffffff810ec1ce RDI: 00000000000000a0
RBP: ffff8800357efa50 R08: ffff88003d7f2560 R09: 0000000000000000
R10: 38260856151e7596 R11: dffffc0000000000 R12: dffffc0000000000
R13: ffff8800357ef9e0 R14: ffff88003a459140 R15: 0000000000000000
FS: 00007faa53a40700(0000) GS:ffff88003fd00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 000000003d5c6000 CR4: 00000000000026e0
Call Trace:
kvm_vcpu_ioctl+0x673/0x1120 arch/x86/kvm/../../../virt/kvm/kvm_main.c:2569
vfs_ioctl fs/ioctl.c:43 [inline]
do_vfs_ioctl+0x1bf/0x1780 fs/ioctl.c:683
SYSC_ioctl fs/ioctl.c:698 [inline]
SyS_ioctl+0x8f/0xc0 fs/ioctl.c:689
entry_SYSCALL_64_fastpath+0x1f/0xc2
RIP: 0033:0x443a19
RSP: 002b:00007faa53a3fb58 EFLAGS: 00000286 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 0000000000000018 RCX: 0000000000443a19
RDX: 0000000000000000 RSI: 000000000000ae80 RDI: 0000000000000018
RBP: 00000000006ddb30 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000286 R12: 00000000007000a8
R13: 00007faa53c23328 R14: 00007faa53c25358 R15: 0000000000000003
Code: e0 03 00 00 48 89 f8 48 c1 e8 03 42 80 3c 20 00 0f 85 82 1a 00
00 49 8b 86 e0 03 00 00 48 8d b8 a0 00 00 00 48 89 fa 48 c1 ea 03 <42>
80 3c 22 00 0f 85 3f 15 00 00 48 8b 80 a0 00 00 00 48 8d b8
RIP: kvm_apic_hw_enabled arch/x86/kvm/lapic.h:156 [inline] RSP: ffff8800357ef7d8
RIP: vcpu_scan_ioapic arch/x86/kvm/x86.c:6559 [inline] RSP: ffff8800357ef7d8
RIP: vcpu_enter_guest arch/x86/kvm/x86.c:6691 [inline] RSP: ffff8800357ef7d8
RIP: vcpu_run arch/x86/kvm/x86.c:6944 [inline] RSP: ffff8800357ef7d8
RIP: kvm_arch_vcpu_ioctl_run+0x272c/0x4660 arch/x86/kvm/x86.c:7102
RSP: ffff8800357ef7d8
---[ end trace 72f3e29e9ea09f21 ]---
Kernel panic - not syncing: Fatal exception
Dumping ftrace buffer:
(ftrace buffer empty)
Kernel Offset: disabled
static void vcpu_scan_ioapic(struct kvm_vcpu *vcpu)
{
u64 eoi_exit_bitmap[4];
if (!kvm_apic_hw_enabled(vcpu->arch.apic)) // <-----HERE
return;
static inline int kvm_apic_hw_enabled(struct kvm_lapic *apic)
{
if (static_key_false(&apic_hw_disabled.key))
return apic->vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE; // <--- HERE
return MSR_IA32_APICBASE_ENABLE;
}
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] KVM: ioapic: fix NULL deref ioapic->lock
2017-01-02 10:17 ` Dmitry Vyukov
@ 2017-01-02 18:01 ` Paolo Bonzini
2017-01-02 22:37 ` Wanpeng Li
2017-01-03 9:27 ` Dmitry Vyukov
0 siblings, 2 replies; 10+ messages in thread
From: Paolo Bonzini @ 2017-01-02 18:01 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Wanpeng Li, LKML, KVM list, Radim Krčmář, Wanpeng Li
On 02/01/2017 11:17, Dmitry Vyukov wrote:
> On Mon, Jan 2, 2017 at 11:09 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>>
>> On 01/01/2017 04:44, Wanpeng Li wrote:
>>> From: Wanpeng Li <wanpeng.li@hotmail.com>
>>>
>>> This was reported by syzkaller:
>>>
>>> BUG: unable to handle kernel NULL pointer dereference at 00000000000001b0
>>> IP: _raw_spin_lock+0xc/0x30
>>> PGD 3e28eb067
>>> PUD 3f0ac6067
>>> PMD 0
>>> Oops: 0002 [#1] SMP
>>> CPU: 0 PID: 2431 Comm: test Tainted: G OE 4.10.0-rc1+ #3
>>> Call Trace:
>>> ? kvm_ioapic_scan_entry+0x3e/0x110 [kvm]
>>> kvm_arch_vcpu_ioctl_run+0x10a8/0x15f0 [kvm]
>>> ? pick_next_task_fair+0xe1/0x4e0
>>> ? kvm_arch_vcpu_load+0xea/0x260 [kvm]
>>> kvm_vcpu_ioctl+0x33a/0x600 [kvm]
>>> ? hrtimer_try_to_cancel+0x29/0x130
>>> ? do_nanosleep+0x97/0xf0
>>> do_vfs_ioctl+0xa1/0x5d0
>>> ? __hrtimer_init+0x90/0x90
>>> ? do_nanosleep+0x5b/0xf0
>>> SyS_ioctl+0x79/0x90
>>> do_syscall_64+0x6e/0x180
>>> entry_SYSCALL64_slow_path+0x25/0x25
>>> RIP: _raw_spin_lock+0xc/0x30 RSP: ffffa43688973cc0
>>>
>>> KVM will skip over create pic/ioapic if there is a created vCPU. However,
>>> there is no guarantee whether ioapic is present when rescan ioapic which
>>> results in NULL dereference ioapic->lock. This patch fix it by adding the
>>> ioapic present check to ioapic scan.
>>>
>>> Reported-by: Dmitry Vyukov <dvyukov@google.com>
>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>> Cc: Radim Krčmář <rkrcmar@redhat.com>
>>> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
>>> ---
>>> arch/x86/kvm/x86.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index 51ccfe0..9ca175c 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -6556,7 +6556,8 @@ static void vcpu_scan_ioapic(struct kvm_vcpu *vcpu)
>>> else {
>>> if (vcpu->arch.apicv_active)
>>> kvm_x86_ops->sync_pir_to_irr(vcpu);
>>> - kvm_ioapic_scan_entry(vcpu, vcpu->arch.ioapic_handled_vectors);
>>> + if (ioapic_irqchip(vcpu->kvm))
>>> + kvm_ioapic_scan_entry(vcpu, vcpu->arch.ioapic_handled_vectors);
>>> }
>>> bitmap_or((ulong *)eoi_exit_bitmap, vcpu->arch.ioapic_handled_vectors,
>>> vcpu_to_synic(vcpu)->vec_bitmap, 256);
>>
>> Commit message for fuzzing bugs have usually included a beautified
>> reproducer. However, you are not even saying if it is a race, or it is
>> deterministic.
>>
>> The fix seems wrong to me at first impression, because "LAPIC enabled"
>> and "irqchip not split" should imply the existence of an in-kernel
>> IOAPIC. However, I cannot suggest the right course of action without
>> seeing a testcase.
>
>
>
> I've created a reasonably beautified reproducer here:
> https://groups.google.com/forum/#!msg/syzkaller/6V-KXaMDYi8/rOvBl-69DAAJ
Thanks, this is beautiful enough. :)
Hmm, the combination of 6c7caebc26c5 ("KVM: introduce
kvm->created_vcpus", 2016-06-16) and 4c5ea0a9cd02 ("locking/static_key:
Fix concurrent static_key_slow_inc()", 2016-06-24) should have fixed it
for good.
Is the ENABLE_CAP necessary to reproduce? Then, the bug is simply that
the ENABLE_CAP should have failed without an irqchip (the
KVM_CREATE_IRQCHIP in turn must have failed with EINVAL).
Paolo
>
> FWIW the patch has fixed the crash, but there is another similar one
> that happens slightly earlier:
>
> general protection fault: 0000 [#1] SMP KASAN
> Dumping ftrace buffer:
> (ftrace buffer empty)
> Modules linked in:
> CPU: 1 PID: 17462 Comm: syz-executor4 Not tainted 4.10.0-rc1+ #119
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> task: ffff88003d6a2280 task.stack: ffff8800357e8000
> RIP: 0010:kvm_apic_hw_enabled arch/x86/kvm/lapic.h:156 [inline]
> RIP: 0010:vcpu_scan_ioapic arch/x86/kvm/x86.c:6559 [inline]
> RIP: 0010:vcpu_enter_guest arch/x86/kvm/x86.c:6691 [inline]
> RIP: 0010:vcpu_run arch/x86/kvm/x86.c:6944 [inline]
> RIP: 0010:kvm_arch_vcpu_ioctl_run+0x272c/0x4660 arch/x86/kvm/x86.c:7102
> RSP: 0018:ffff8800357ef7d8 EFLAGS: 00010206
> RAX: 0000000000000000 RBX: ffff88003a459170 RCX: ffffc90000a76000
> RDX: 0000000000000014 RSI: ffffffff810ec1ce RDI: 00000000000000a0
> RBP: ffff8800357efa50 R08: ffff88003d7f2560 R09: 0000000000000000
> R10: 38260856151e7596 R11: dffffc0000000000 R12: dffffc0000000000
> R13: ffff8800357ef9e0 R14: ffff88003a459140 R15: 0000000000000000
> FS: 00007faa53a40700(0000) GS:ffff88003fd00000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000000 CR3: 000000003d5c6000 CR4: 00000000000026e0
> Call Trace:
> kvm_vcpu_ioctl+0x673/0x1120 arch/x86/kvm/../../../virt/kvm/kvm_main.c:2569
> vfs_ioctl fs/ioctl.c:43 [inline]
> do_vfs_ioctl+0x1bf/0x1780 fs/ioctl.c:683
> SYSC_ioctl fs/ioctl.c:698 [inline]
> SyS_ioctl+0x8f/0xc0 fs/ioctl.c:689
> entry_SYSCALL_64_fastpath+0x1f/0xc2
> RIP: 0033:0x443a19
> RSP: 002b:00007faa53a3fb58 EFLAGS: 00000286 ORIG_RAX: 0000000000000010
> RAX: ffffffffffffffda RBX: 0000000000000018 RCX: 0000000000443a19
> RDX: 0000000000000000 RSI: 000000000000ae80 RDI: 0000000000000018
> RBP: 00000000006ddb30 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000286 R12: 00000000007000a8
> R13: 00007faa53c23328 R14: 00007faa53c25358 R15: 0000000000000003
> Code: e0 03 00 00 48 89 f8 48 c1 e8 03 42 80 3c 20 00 0f 85 82 1a 00
> 00 49 8b 86 e0 03 00 00 48 8d b8 a0 00 00 00 48 89 fa 48 c1 ea 03 <42>
> 80 3c 22 00 0f 85 3f 15 00 00 48 8b 80 a0 00 00 00 48 8d b8
> RIP: kvm_apic_hw_enabled arch/x86/kvm/lapic.h:156 [inline] RSP: ffff8800357ef7d8
> RIP: vcpu_scan_ioapic arch/x86/kvm/x86.c:6559 [inline] RSP: ffff8800357ef7d8
> RIP: vcpu_enter_guest arch/x86/kvm/x86.c:6691 [inline] RSP: ffff8800357ef7d8
> RIP: vcpu_run arch/x86/kvm/x86.c:6944 [inline] RSP: ffff8800357ef7d8
> RIP: kvm_arch_vcpu_ioctl_run+0x272c/0x4660 arch/x86/kvm/x86.c:7102
> RSP: ffff8800357ef7d8
> ---[ end trace 72f3e29e9ea09f21 ]---
> Kernel panic - not syncing: Fatal exception
> Dumping ftrace buffer:
> (ftrace buffer empty)
> Kernel Offset: disabled
>
>
> static void vcpu_scan_ioapic(struct kvm_vcpu *vcpu)
> {
> u64 eoi_exit_bitmap[4];
>
> if (!kvm_apic_hw_enabled(vcpu->arch.apic)) // <-----HERE
> return;
>
>
> static inline int kvm_apic_hw_enabled(struct kvm_lapic *apic)
> {
> if (static_key_false(&apic_hw_disabled.key))
> return apic->vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE; // <--- HERE
> return MSR_IA32_APICBASE_ENABLE;
> }
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] KVM: ioapic: fix NULL deref ioapic->lock
2017-01-02 18:01 ` Paolo Bonzini
@ 2017-01-02 22:37 ` Wanpeng Li
2017-01-03 9:27 ` Dmitry Vyukov
1 sibling, 0 replies; 10+ messages in thread
From: Wanpeng Li @ 2017-01-02 22:37 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Dmitry Vyukov, LKML, KVM list, Radim Krčmář, Wanpeng Li
2017-01-03 2:01 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>
>
> On 02/01/2017 11:17, Dmitry Vyukov wrote:
>> On Mon, Jan 2, 2017 at 11:09 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>
>>>
>>>
>>> On 01/01/2017 04:44, Wanpeng Li wrote:
>>>> From: Wanpeng Li <wanpeng.li@hotmail.com>
>>>>
>>>> This was reported by syzkaller:
>>>>
>>>> BUG: unable to handle kernel NULL pointer dereference at 00000000000001b0
>>>> IP: _raw_spin_lock+0xc/0x30
>>>> PGD 3e28eb067
>>>> PUD 3f0ac6067
>>>> PMD 0
>>>> Oops: 0002 [#1] SMP
>>>> CPU: 0 PID: 2431 Comm: test Tainted: G OE 4.10.0-rc1+ #3
>>>> Call Trace:
>>>> ? kvm_ioapic_scan_entry+0x3e/0x110 [kvm]
>>>> kvm_arch_vcpu_ioctl_run+0x10a8/0x15f0 [kvm]
>>>> ? pick_next_task_fair+0xe1/0x4e0
>>>> ? kvm_arch_vcpu_load+0xea/0x260 [kvm]
>>>> kvm_vcpu_ioctl+0x33a/0x600 [kvm]
>>>> ? hrtimer_try_to_cancel+0x29/0x130
>>>> ? do_nanosleep+0x97/0xf0
>>>> do_vfs_ioctl+0xa1/0x5d0
>>>> ? __hrtimer_init+0x90/0x90
>>>> ? do_nanosleep+0x5b/0xf0
>>>> SyS_ioctl+0x79/0x90
>>>> do_syscall_64+0x6e/0x180
>>>> entry_SYSCALL64_slow_path+0x25/0x25
>>>> RIP: _raw_spin_lock+0xc/0x30 RSP: ffffa43688973cc0
>>>>
>>>> KVM will skip over create pic/ioapic if there is a created vCPU. However,
>>>> there is no guarantee whether ioapic is present when rescan ioapic which
>>>> results in NULL dereference ioapic->lock. This patch fix it by adding the
>>>> ioapic present check to ioapic scan.
>>>>
>>>> Reported-by: Dmitry Vyukov <dvyukov@google.com>
>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>>> Cc: Radim Krčmář <rkrcmar@redhat.com>
>>>> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
>>>> ---
>>>> arch/x86/kvm/x86.c | 3 ++-
>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>>> index 51ccfe0..9ca175c 100644
>>>> --- a/arch/x86/kvm/x86.c
>>>> +++ b/arch/x86/kvm/x86.c
>>>> @@ -6556,7 +6556,8 @@ static void vcpu_scan_ioapic(struct kvm_vcpu *vcpu)
>>>> else {
>>>> if (vcpu->arch.apicv_active)
>>>> kvm_x86_ops->sync_pir_to_irr(vcpu);
>>>> - kvm_ioapic_scan_entry(vcpu, vcpu->arch.ioapic_handled_vectors);
>>>> + if (ioapic_irqchip(vcpu->kvm))
>>>> + kvm_ioapic_scan_entry(vcpu, vcpu->arch.ioapic_handled_vectors);
>>>> }
>>>> bitmap_or((ulong *)eoi_exit_bitmap, vcpu->arch.ioapic_handled_vectors,
>>>> vcpu_to_synic(vcpu)->vec_bitmap, 256);
>>>
>>> Commit message for fuzzing bugs have usually included a beautified
>>> reproducer. However, you are not even saying if it is a race, or it is
>>> deterministic.
>>>
>>> The fix seems wrong to me at first impression, because "LAPIC enabled"
>>> and "irqchip not split" should imply the existence of an in-kernel
>>> IOAPIC. However, I cannot suggest the right course of action without
>>> seeing a testcase.
>>
>>
>>
>> I've created a reasonably beautified reproducer here:
>> https://groups.google.com/forum/#!msg/syzkaller/6V-KXaMDYi8/rOvBl-69DAAJ
>
> Thanks, this is beautiful enough. :)
>
> Hmm, the combination of 6c7caebc26c5 ("KVM: introduce
> kvm->created_vcpus", 2016-06-16) and 4c5ea0a9cd02 ("locking/static_key:
> Fix concurrent static_key_slow_inc()", 2016-06-24) should have fixed it
> for good.
>
> Is the ENABLE_CAP necessary to reproduce? Then, the bug is simply that
> the ENABLE_CAP should have failed without an irqchip (the
The beautified reproducer didn't check KVM_CAP_IRQCHIP.
> KVM_CREATE_IRQCHIP in turn must have failed with EINVAL).
>
> Paolo
>
>>
>> FWIW the patch has fixed the crash, but there is another similar one
>> that happens slightly earlier:
Maybe a irqchip_in_kernel() check at the head of function vcpu_scan_ioapic().
Regards,
Wanpeng Li
>>
>> general protection fault: 0000 [#1] SMP KASAN
>> Dumping ftrace buffer:
>> (ftrace buffer empty)
>> Modules linked in:
>> CPU: 1 PID: 17462 Comm: syz-executor4 Not tainted 4.10.0-rc1+ #119
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>> task: ffff88003d6a2280 task.stack: ffff8800357e8000
>> RIP: 0010:kvm_apic_hw_enabled arch/x86/kvm/lapic.h:156 [inline]
>> RIP: 0010:vcpu_scan_ioapic arch/x86/kvm/x86.c:6559 [inline]
>> RIP: 0010:vcpu_enter_guest arch/x86/kvm/x86.c:6691 [inline]
>> RIP: 0010:vcpu_run arch/x86/kvm/x86.c:6944 [inline]
>> RIP: 0010:kvm_arch_vcpu_ioctl_run+0x272c/0x4660 arch/x86/kvm/x86.c:7102
>> RSP: 0018:ffff8800357ef7d8 EFLAGS: 00010206
>> RAX: 0000000000000000 RBX: ffff88003a459170 RCX: ffffc90000a76000
>> RDX: 0000000000000014 RSI: ffffffff810ec1ce RDI: 00000000000000a0
>> RBP: ffff8800357efa50 R08: ffff88003d7f2560 R09: 0000000000000000
>> R10: 38260856151e7596 R11: dffffc0000000000 R12: dffffc0000000000
>> R13: ffff8800357ef9e0 R14: ffff88003a459140 R15: 0000000000000000
>> FS: 00007faa53a40700(0000) GS:ffff88003fd00000(0000) knlGS:0000000000000000
>> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> CR2: 0000000000000000 CR3: 000000003d5c6000 CR4: 00000000000026e0
>> Call Trace:
>> kvm_vcpu_ioctl+0x673/0x1120 arch/x86/kvm/../../../virt/kvm/kvm_main.c:2569
>> vfs_ioctl fs/ioctl.c:43 [inline]
>> do_vfs_ioctl+0x1bf/0x1780 fs/ioctl.c:683
>> SYSC_ioctl fs/ioctl.c:698 [inline]
>> SyS_ioctl+0x8f/0xc0 fs/ioctl.c:689
>> entry_SYSCALL_64_fastpath+0x1f/0xc2
>> RIP: 0033:0x443a19
>> RSP: 002b:00007faa53a3fb58 EFLAGS: 00000286 ORIG_RAX: 0000000000000010
>> RAX: ffffffffffffffda RBX: 0000000000000018 RCX: 0000000000443a19
>> RDX: 0000000000000000 RSI: 000000000000ae80 RDI: 0000000000000018
>> RBP: 00000000006ddb30 R08: 0000000000000000 R09: 0000000000000000
>> R10: 0000000000000000 R11: 0000000000000286 R12: 00000000007000a8
>> R13: 00007faa53c23328 R14: 00007faa53c25358 R15: 0000000000000003
>> Code: e0 03 00 00 48 89 f8 48 c1 e8 03 42 80 3c 20 00 0f 85 82 1a 00
>> 00 49 8b 86 e0 03 00 00 48 8d b8 a0 00 00 00 48 89 fa 48 c1 ea 03 <42>
>> 80 3c 22 00 0f 85 3f 15 00 00 48 8b 80 a0 00 00 00 48 8d b8
>> RIP: kvm_apic_hw_enabled arch/x86/kvm/lapic.h:156 [inline] RSP: ffff8800357ef7d8
>> RIP: vcpu_scan_ioapic arch/x86/kvm/x86.c:6559 [inline] RSP: ffff8800357ef7d8
>> RIP: vcpu_enter_guest arch/x86/kvm/x86.c:6691 [inline] RSP: ffff8800357ef7d8
>> RIP: vcpu_run arch/x86/kvm/x86.c:6944 [inline] RSP: ffff8800357ef7d8
>> RIP: kvm_arch_vcpu_ioctl_run+0x272c/0x4660 arch/x86/kvm/x86.c:7102
>> RSP: ffff8800357ef7d8
>> ---[ end trace 72f3e29e9ea09f21 ]---
>> Kernel panic - not syncing: Fatal exception
>> Dumping ftrace buffer:
>> (ftrace buffer empty)
>> Kernel Offset: disabled
>>
>>
>> static void vcpu_scan_ioapic(struct kvm_vcpu *vcpu)
>> {
>> u64 eoi_exit_bitmap[4];
>>
>> if (!kvm_apic_hw_enabled(vcpu->arch.apic)) // <-----HERE
>> return;
>>
>>
>> static inline int kvm_apic_hw_enabled(struct kvm_lapic *apic)
>> {
>> if (static_key_false(&apic_hw_disabled.key))
>> return apic->vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE; // <--- HERE
>> return MSR_IA32_APICBASE_ENABLE;
>> }
>>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] KVM: ioapic: fix NULL deref ioapic->lock
2017-01-02 18:01 ` Paolo Bonzini
2017-01-02 22:37 ` Wanpeng Li
@ 2017-01-03 9:27 ` Dmitry Vyukov
2017-01-03 10:40 ` Wanpeng Li
1 sibling, 1 reply; 10+ messages in thread
From: Dmitry Vyukov @ 2017-01-03 9:27 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Wanpeng Li, LKML, KVM list, Radim Krčmář, Wanpeng Li
On Mon, Jan 2, 2017 at 7:01 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 02/01/2017 11:17, Dmitry Vyukov wrote:
>> On Mon, Jan 2, 2017 at 11:09 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>
>>>
>>>
>>> On 01/01/2017 04:44, Wanpeng Li wrote:
>>>> From: Wanpeng Li <wanpeng.li@hotmail.com>
>>>>
>>>> This was reported by syzkaller:
>>>>
>>>> BUG: unable to handle kernel NULL pointer dereference at 00000000000001b0
>>>> IP: _raw_spin_lock+0xc/0x30
>>>> PGD 3e28eb067
>>>> PUD 3f0ac6067
>>>> PMD 0
>>>> Oops: 0002 [#1] SMP
>>>> CPU: 0 PID: 2431 Comm: test Tainted: G OE 4.10.0-rc1+ #3
>>>> Call Trace:
>>>> ? kvm_ioapic_scan_entry+0x3e/0x110 [kvm]
>>>> kvm_arch_vcpu_ioctl_run+0x10a8/0x15f0 [kvm]
>>>> ? pick_next_task_fair+0xe1/0x4e0
>>>> ? kvm_arch_vcpu_load+0xea/0x260 [kvm]
>>>> kvm_vcpu_ioctl+0x33a/0x600 [kvm]
>>>> ? hrtimer_try_to_cancel+0x29/0x130
>>>> ? do_nanosleep+0x97/0xf0
>>>> do_vfs_ioctl+0xa1/0x5d0
>>>> ? __hrtimer_init+0x90/0x90
>>>> ? do_nanosleep+0x5b/0xf0
>>>> SyS_ioctl+0x79/0x90
>>>> do_syscall_64+0x6e/0x180
>>>> entry_SYSCALL64_slow_path+0x25/0x25
>>>> RIP: _raw_spin_lock+0xc/0x30 RSP: ffffa43688973cc0
>>>>
>>>> KVM will skip over create pic/ioapic if there is a created vCPU. However,
>>>> there is no guarantee whether ioapic is present when rescan ioapic which
>>>> results in NULL dereference ioapic->lock. This patch fix it by adding the
>>>> ioapic present check to ioapic scan.
>>>>
>>>> Reported-by: Dmitry Vyukov <dvyukov@google.com>
>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>>> Cc: Radim Krčmář <rkrcmar@redhat.com>
>>>> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
>>>> ---
>>>> arch/x86/kvm/x86.c | 3 ++-
>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>>> index 51ccfe0..9ca175c 100644
>>>> --- a/arch/x86/kvm/x86.c
>>>> +++ b/arch/x86/kvm/x86.c
>>>> @@ -6556,7 +6556,8 @@ static void vcpu_scan_ioapic(struct kvm_vcpu *vcpu)
>>>> else {
>>>> if (vcpu->arch.apicv_active)
>>>> kvm_x86_ops->sync_pir_to_irr(vcpu);
>>>> - kvm_ioapic_scan_entry(vcpu, vcpu->arch.ioapic_handled_vectors);
>>>> + if (ioapic_irqchip(vcpu->kvm))
>>>> + kvm_ioapic_scan_entry(vcpu, vcpu->arch.ioapic_handled_vectors);
>>>> }
>>>> bitmap_or((ulong *)eoi_exit_bitmap, vcpu->arch.ioapic_handled_vectors,
>>>> vcpu_to_synic(vcpu)->vec_bitmap, 256);
>>>
>>> Commit message for fuzzing bugs have usually included a beautified
>>> reproducer. However, you are not even saying if it is a race, or it is
>>> deterministic.
>>>
>>> The fix seems wrong to me at first impression, because "LAPIC enabled"
>>> and "irqchip not split" should imply the existence of an in-kernel
>>> IOAPIC. However, I cannot suggest the right course of action without
>>> seeing a testcase.
>>
>>
>>
>> I've created a reasonably beautified reproducer here:
>> https://groups.google.com/forum/#!msg/syzkaller/6V-KXaMDYi8/rOvBl-69DAAJ
>
> Thanks, this is beautiful enough. :)
>
> Hmm, the combination of 6c7caebc26c5 ("KVM: introduce
> kvm->created_vcpus", 2016-06-16) and 4c5ea0a9cd02 ("locking/static_key:
> Fix concurrent static_key_slow_inc()", 2016-06-24) should have fixed it
> for good.
>
> Is the ENABLE_CAP necessary to reproduce? Then, the bug is simply that
> the ENABLE_CAP should have failed without an irqchip (the
> KVM_CREATE_IRQCHIP in turn must have failed with EINVAL).
ENABLE_CAP is necessary to reproduce.
You are right KVM_CREATE_IRQCHIP fails with EINVAL:
ioctl(4, KVM_CREATE_IRQCHIP, 0) = -1 EINVAL (Invalid argument)
so I guess it is not necessary to reproduce.
It looks like ENABLE_CAP(KVM_CAP_HYPERV_SYNIC) badly races with exit in KVM_RUN.
>> FWIW the patch has fixed the crash, but there is another similar one
>> that happens slightly earlier:
>>
>> general protection fault: 0000 [#1] SMP KASAN
>> Dumping ftrace buffer:
>> (ftrace buffer empty)
>> Modules linked in:
>> CPU: 1 PID: 17462 Comm: syz-executor4 Not tainted 4.10.0-rc1+ #119
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>> task: ffff88003d6a2280 task.stack: ffff8800357e8000
>> RIP: 0010:kvm_apic_hw_enabled arch/x86/kvm/lapic.h:156 [inline]
>> RIP: 0010:vcpu_scan_ioapic arch/x86/kvm/x86.c:6559 [inline]
>> RIP: 0010:vcpu_enter_guest arch/x86/kvm/x86.c:6691 [inline]
>> RIP: 0010:vcpu_run arch/x86/kvm/x86.c:6944 [inline]
>> RIP: 0010:kvm_arch_vcpu_ioctl_run+0x272c/0x4660 arch/x86/kvm/x86.c:7102
>> RSP: 0018:ffff8800357ef7d8 EFLAGS: 00010206
>> RAX: 0000000000000000 RBX: ffff88003a459170 RCX: ffffc90000a76000
>> RDX: 0000000000000014 RSI: ffffffff810ec1ce RDI: 00000000000000a0
>> RBP: ffff8800357efa50 R08: ffff88003d7f2560 R09: 0000000000000000
>> R10: 38260856151e7596 R11: dffffc0000000000 R12: dffffc0000000000
>> R13: ffff8800357ef9e0 R14: ffff88003a459140 R15: 0000000000000000
>> FS: 00007faa53a40700(0000) GS:ffff88003fd00000(0000) knlGS:0000000000000000
>> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> CR2: 0000000000000000 CR3: 000000003d5c6000 CR4: 00000000000026e0
>> Call Trace:
>> kvm_vcpu_ioctl+0x673/0x1120 arch/x86/kvm/../../../virt/kvm/kvm_main.c:2569
>> vfs_ioctl fs/ioctl.c:43 [inline]
>> do_vfs_ioctl+0x1bf/0x1780 fs/ioctl.c:683
>> SYSC_ioctl fs/ioctl.c:698 [inline]
>> SyS_ioctl+0x8f/0xc0 fs/ioctl.c:689
>> entry_SYSCALL_64_fastpath+0x1f/0xc2
>> RIP: 0033:0x443a19
>> RSP: 002b:00007faa53a3fb58 EFLAGS: 00000286 ORIG_RAX: 0000000000000010
>> RAX: ffffffffffffffda RBX: 0000000000000018 RCX: 0000000000443a19
>> RDX: 0000000000000000 RSI: 000000000000ae80 RDI: 0000000000000018
>> RBP: 00000000006ddb30 R08: 0000000000000000 R09: 0000000000000000
>> R10: 0000000000000000 R11: 0000000000000286 R12: 00000000007000a8
>> R13: 00007faa53c23328 R14: 00007faa53c25358 R15: 0000000000000003
>> Code: e0 03 00 00 48 89 f8 48 c1 e8 03 42 80 3c 20 00 0f 85 82 1a 00
>> 00 49 8b 86 e0 03 00 00 48 8d b8 a0 00 00 00 48 89 fa 48 c1 ea 03 <42>
>> 80 3c 22 00 0f 85 3f 15 00 00 48 8b 80 a0 00 00 00 48 8d b8
>> RIP: kvm_apic_hw_enabled arch/x86/kvm/lapic.h:156 [inline] RSP: ffff8800357ef7d8
>> RIP: vcpu_scan_ioapic arch/x86/kvm/x86.c:6559 [inline] RSP: ffff8800357ef7d8
>> RIP: vcpu_enter_guest arch/x86/kvm/x86.c:6691 [inline] RSP: ffff8800357ef7d8
>> RIP: vcpu_run arch/x86/kvm/x86.c:6944 [inline] RSP: ffff8800357ef7d8
>> RIP: kvm_arch_vcpu_ioctl_run+0x272c/0x4660 arch/x86/kvm/x86.c:7102
>> RSP: ffff8800357ef7d8
>> ---[ end trace 72f3e29e9ea09f21 ]---
>> Kernel panic - not syncing: Fatal exception
>> Dumping ftrace buffer:
>> (ftrace buffer empty)
>> Kernel Offset: disabled
>>
>>
>> static void vcpu_scan_ioapic(struct kvm_vcpu *vcpu)
>> {
>> u64 eoi_exit_bitmap[4];
>>
>> if (!kvm_apic_hw_enabled(vcpu->arch.apic)) // <-----HERE
>> return;
>>
>>
>> static inline int kvm_apic_hw_enabled(struct kvm_lapic *apic)
>> {
>> if (static_key_false(&apic_hw_disabled.key))
>> return apic->vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE; // <--- HERE
>> return MSR_IA32_APICBASE_ENABLE;
>> }
>>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] KVM: ioapic: fix NULL deref ioapic->lock
2017-01-03 9:27 ` Dmitry Vyukov
@ 2017-01-03 10:40 ` Wanpeng Li
2017-01-03 12:06 ` David Hildenbrand
0 siblings, 1 reply; 10+ messages in thread
From: Wanpeng Li @ 2017-01-03 10:40 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Paolo Bonzini, LKML, KVM list, Radim Krčmář, Wanpeng Li
2017-01-03 17:27 GMT+08:00 Dmitry Vyukov <dvyukov@google.com>:
> On Mon, Jan 2, 2017 at 7:01 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 02/01/2017 11:17, Dmitry Vyukov wrote:
>>> On Mon, Jan 2, 2017 at 11:09 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>
>>>>
>>>>
>>>> On 01/01/2017 04:44, Wanpeng Li wrote:
>>>>> From: Wanpeng Li <wanpeng.li@hotmail.com>
>>>>>
>>>>> This was reported by syzkaller:
>>>>>
>>>>> BUG: unable to handle kernel NULL pointer dereference at 00000000000001b0
>>>>> IP: _raw_spin_lock+0xc/0x30
>>>>> PGD 3e28eb067
>>>>> PUD 3f0ac6067
>>>>> PMD 0
>>>>> Oops: 0002 [#1] SMP
>>>>> CPU: 0 PID: 2431 Comm: test Tainted: G OE 4.10.0-rc1+ #3
>>>>> Call Trace:
>>>>> ? kvm_ioapic_scan_entry+0x3e/0x110 [kvm]
>>>>> kvm_arch_vcpu_ioctl_run+0x10a8/0x15f0 [kvm]
>>>>> ? pick_next_task_fair+0xe1/0x4e0
>>>>> ? kvm_arch_vcpu_load+0xea/0x260 [kvm]
>>>>> kvm_vcpu_ioctl+0x33a/0x600 [kvm]
>>>>> ? hrtimer_try_to_cancel+0x29/0x130
>>>>> ? do_nanosleep+0x97/0xf0
>>>>> do_vfs_ioctl+0xa1/0x5d0
>>>>> ? __hrtimer_init+0x90/0x90
>>>>> ? do_nanosleep+0x5b/0xf0
>>>>> SyS_ioctl+0x79/0x90
>>>>> do_syscall_64+0x6e/0x180
>>>>> entry_SYSCALL64_slow_path+0x25/0x25
>>>>> RIP: _raw_spin_lock+0xc/0x30 RSP: ffffa43688973cc0
>>>>>
>>>>> KVM will skip over create pic/ioapic if there is a created vCPU. However,
>>>>> there is no guarantee whether ioapic is present when rescan ioapic which
>>>>> results in NULL dereference ioapic->lock. This patch fix it by adding the
>>>>> ioapic present check to ioapic scan.
>>>>>
>>>>> Reported-by: Dmitry Vyukov <dvyukov@google.com>
>>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>>>> Cc: Radim Krčmář <rkrcmar@redhat.com>
>>>>> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
>>>>> ---
>>>>> arch/x86/kvm/x86.c | 3 ++-
>>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>>>> index 51ccfe0..9ca175c 100644
>>>>> --- a/arch/x86/kvm/x86.c
>>>>> +++ b/arch/x86/kvm/x86.c
>>>>> @@ -6556,7 +6556,8 @@ static void vcpu_scan_ioapic(struct kvm_vcpu *vcpu)
>>>>> else {
>>>>> if (vcpu->arch.apicv_active)
>>>>> kvm_x86_ops->sync_pir_to_irr(vcpu);
>>>>> - kvm_ioapic_scan_entry(vcpu, vcpu->arch.ioapic_handled_vectors);
>>>>> + if (ioapic_irqchip(vcpu->kvm))
>>>>> + kvm_ioapic_scan_entry(vcpu, vcpu->arch.ioapic_handled_vectors);
>>>>> }
>>>>> bitmap_or((ulong *)eoi_exit_bitmap, vcpu->arch.ioapic_handled_vectors,
>>>>> vcpu_to_synic(vcpu)->vec_bitmap, 256);
>>>>
>>>> Commit message for fuzzing bugs have usually included a beautified
>>>> reproducer. However, you are not even saying if it is a race, or it is
>>>> deterministic.
>>>>
>>>> The fix seems wrong to me at first impression, because "LAPIC enabled"
>>>> and "irqchip not split" should imply the existence of an in-kernel
>>>> IOAPIC. However, I cannot suggest the right course of action without
>>>> seeing a testcase.
>>>
>>>
>>>
>>> I've created a reasonably beautified reproducer here:
>>> https://groups.google.com/forum/#!msg/syzkaller/6V-KXaMDYi8/rOvBl-69DAAJ
>>
>> Thanks, this is beautiful enough. :)
>>
>> Hmm, the combination of 6c7caebc26c5 ("KVM: introduce
>> kvm->created_vcpus", 2016-06-16) and 4c5ea0a9cd02 ("locking/static_key:
>> Fix concurrent static_key_slow_inc()", 2016-06-24) should have fixed it
>> for good.
>>
>> Is the ENABLE_CAP necessary to reproduce? Then, the bug is simply that
>> the ENABLE_CAP should have failed without an irqchip (the
>> KVM_CREATE_IRQCHIP in turn must have failed with EINVAL).
>
> ENABLE_CAP is necessary to reproduce.
Now I see what Paolo means, how about something like below:
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 51ccfe0..7ec22e2 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3337,7 +3337,10 @@ static int kvm_vcpu_ioctl_enable_cap(struct
kvm_vcpu *vcpu,
switch (cap->cap) {
case KVM_CAP_HYPERV_SYNIC:
- return kvm_hv_activate_synic(vcpu);
+ if (!irqchip_in_kernel(vcpu->kvm))
+ return -EINVAL;
+ else
+ return kvm_hv_activate_synic(vcpu);
default:
return -EINVAL;
}
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] KVM: ioapic: fix NULL deref ioapic->lock
2017-01-03 10:40 ` Wanpeng Li
@ 2017-01-03 12:06 ` David Hildenbrand
2017-01-03 17:23 ` Paolo Bonzini
0 siblings, 1 reply; 10+ messages in thread
From: David Hildenbrand @ 2017-01-03 12:06 UTC (permalink / raw)
To: Wanpeng Li, Dmitry Vyukov
Cc: Paolo Bonzini, LKML, KVM list, Radim Krčmář, Wanpeng Li
>>> Thanks, this is beautiful enough. :)
>>>
>>> Hmm, the combination of 6c7caebc26c5 ("KVM: introduce
>>> kvm->created_vcpus", 2016-06-16) and 4c5ea0a9cd02 ("locking/static_key:
>>> Fix concurrent static_key_slow_inc()", 2016-06-24) should have fixed it
>>> for good.
>>>
>>> Is the ENABLE_CAP necessary to reproduce? Then, the bug is simply that
>>> the ENABLE_CAP should have failed without an irqchip (the
>>> KVM_CREATE_IRQCHIP in turn must have failed with EINVAL).
>>
>> ENABLE_CAP is necessary to reproduce.
>
> Now I see what Paolo means, how about something like below:
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 51ccfe0..7ec22e2 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3337,7 +3337,10 @@ static int kvm_vcpu_ioctl_enable_cap(struct
> kvm_vcpu *vcpu,
>
> switch (cap->cap) {
> case KVM_CAP_HYPERV_SYNIC:
> - return kvm_hv_activate_synic(vcpu);
> + if (!irqchip_in_kernel(vcpu->kvm))
> + return -EINVAL;
> + else
You can simply drop the else and return directly.
Can't really say if this is the right fix, my first thought was that
a request has been set although it should never have been set for
that VCPU. Maybe that is an effect of synic being activated
(because synic code unconditionally later on sets the request).
Fixing the cause of the request seems better than fixing up the result.
--
David
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] KVM: ioapic: fix NULL deref ioapic->lock
2017-01-03 12:06 ` David Hildenbrand
@ 2017-01-03 17:23 ` Paolo Bonzini
2017-01-03 22:03 ` Wanpeng Li
0 siblings, 1 reply; 10+ messages in thread
From: Paolo Bonzini @ 2017-01-03 17:23 UTC (permalink / raw)
To: David Hildenbrand, Wanpeng Li, Dmitry Vyukov
Cc: LKML, KVM list, Radim Krčmář, Wanpeng Li
On 03/01/2017 13:06, David Hildenbrand wrote:
>>
>> switch (cap->cap) {
>> case KVM_CAP_HYPERV_SYNIC:
>> - return kvm_hv_activate_synic(vcpu);
>> + if (!irqchip_in_kernel(vcpu->kvm))
>> + return -EINVAL;
>> + else
>
> You can simply drop the else and return directly.
>
> Can't really say if this is the right fix, my first thought was that
> a request has been set although it should never have been set for
> that VCPU. Maybe that is an effect of synic being activated
> (because synic code unconditionally later on sets the request).
>
> Fixing the cause of the request seems better than fixing up the result.
Yes, I agree. Wanpeng's second patch is fine.
Paolo
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] KVM: ioapic: fix NULL deref ioapic->lock
2017-01-03 17:23 ` Paolo Bonzini
@ 2017-01-03 22:03 ` Wanpeng Li
0 siblings, 0 replies; 10+ messages in thread
From: Wanpeng Li @ 2017-01-03 22:03 UTC (permalink / raw)
To: Paolo Bonzini
Cc: David Hildenbrand, Dmitry Vyukov, LKML, KVM list,
Radim Krčmář,
Wanpeng Li
2017-01-04 1:23 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>
>
> On 03/01/2017 13:06, David Hildenbrand wrote:
>>>
>>> switch (cap->cap) {
>>> case KVM_CAP_HYPERV_SYNIC:
>>> - return kvm_hv_activate_synic(vcpu);
>>> + if (!irqchip_in_kernel(vcpu->kvm))
>>> + return -EINVAL;
>>> + else
>>
>> You can simply drop the else and return directly.
>>
>> Can't really say if this is the right fix, my first thought was that
>> a request has been set although it should never have been set for
>> that VCPU. Maybe that is an effect of synic being activated
>> (because synic code unconditionally later on sets the request).
>>
>> Fixing the cause of the request seems better than fixing up the result.
>
> Yes, I agree. Wanpeng's second patch is fine.
Thanks Paolo, I will send out a formal one soon.
Regards,
Wanpeng Li
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-01-03 22:04 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-01 3:44 [PATCH] KVM: ioapic: fix NULL deref ioapic->lock Wanpeng Li
2017-01-02 10:09 ` Paolo Bonzini
2017-01-02 10:17 ` Dmitry Vyukov
2017-01-02 18:01 ` Paolo Bonzini
2017-01-02 22:37 ` Wanpeng Li
2017-01-03 9:27 ` Dmitry Vyukov
2017-01-03 10:40 ` Wanpeng Li
2017-01-03 12:06 ` David Hildenbrand
2017-01-03 17:23 ` Paolo Bonzini
2017-01-03 22:03 ` Wanpeng Li
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).