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

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: <CANRm+CwC=duTn7oPykbE+ixnLUBQwt8KMMHkCQWZOS9i2Mzz0A@mail.gmail.com>
Date:   Tue, 3 Jan 2017 18:40:17 +0800
From:   Wanpeng Li <kernellwp@...il.com>
To:     Dmitry Vyukov <dvyukov@...gle.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>,
        KVM list <kvm@...r.kernel.org>,
        Radim Krčmář <rkrcmar@...hat.com>,
        Wanpeng Li <wanpeng.li@...mail.com>
Subject: Re: [PATCH] KVM: ioapic: fix NULL deref ioapic->lock

2017-01-03 17:27 GMT+08:00 Dmitry Vyukov <dvyukov@...gle.com>:
> On Mon, Jan 2, 2017 at 7:01 PM, Paolo Bonzini <pbonzini@...hat.com> wrote:
>>
>>
>> On 02/01/2017 11:17, Dmitry Vyukov wrote:
>>> On Mon, Jan 2, 2017 at 11:09 AM, Paolo Bonzini <pbonzini@...hat.com> wrote:
>>>>
>>>>
>>>>
>>>> On 01/01/2017 04:44, Wanpeng Li wrote:
>>>>> From: Wanpeng Li <wanpeng.li@...mail.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@...gle.com>
>>>>> Cc: Paolo Bonzini <pbonzini@...hat.com>
>>>>> Cc: Radim Krčmář <rkrcmar@...hat.com>
>>>>> Signed-off-by: Wanpeng Li <wanpeng.li@...mail.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;
     }