Re: [PATCH] KVM: ioapic: fix NULL deref ioapic->lock

[Paolo Bonzini]

On 02/01/2017 11:17, Dmitry Vyukov wrote:
> On Mon, Jan 2, 2017 at 11:09 AM, Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
>>
>>
>>
>> On 01/01/2017 04:44, Wanpeng Li wrote:
>>> From: Wanpeng Li <wanpeng.li@xxxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxx>
>>> Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
>>> Cc: Radim KrÄmÃÅ <rkrcmar@xxxxxxxxxx>
>>> Signed-off-by: Wanpeng Li <wanpeng.li@xxxxxxxxxxx>
>>> ---
>>>  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;
> }
>