2017-01-03 17:27 GMT+08:00 Dmitry Vyukov <dvyukov@xxxxxxxxxx>:
> On Mon, Jan 2, 2017 at 7:01 PM, Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
>>
>>
>> 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).
>
> 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;
}
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
