From patchwork Tue Jan 3 10:40:17 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Wanpeng Li X-Patchwork-Id: 9494703 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 4ABE0606A7 for ; Tue, 3 Jan 2017 10:40:45 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 573A0269DA for ; Tue, 3 Jan 2017 10:40:45 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 4923626BE9; Tue, 3 Jan 2017 10:40:45 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.3 required=2.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_HI, RCVD_IN_SORBS_SPAM, T_DKIM_INVALID autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id D8273269DA for ; Tue, 3 Jan 2017 10:40:44 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934467AbdACKkV (ORCPT ); Tue, 3 Jan 2017 05:40:21 -0500 Received: from mail-wj0-f196.google.com ([209.85.210.196]:32769 "EHLO mail-wj0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934189AbdACKkT (ORCPT ); Tue, 3 Jan 2017 05:40:19 -0500 Received: by mail-wj0-f196.google.com with SMTP id kp2so71321682wjc.0; Tue, 03 Jan 2017 02:40:19 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=0YCnCtuDJee1C7b67FJIoQ+hbEjmStHSuR/6ekVWXzM=; b=twpegCNEE3iLfPVwGHdttc2BFTw72M7Q+XZycq06mnKvQh3YRoZx0ElDqZj5VXOj8d 3G4QNHkKrUgWS1xiimIjV0oRGJvBNC/qT9MyW0/PHtfQalh+VFmm45AsB5yKRO11Hqsv g2xdnCX/bsVGsVPsprLJYjwVoDelqTEJk+R44YhQ3Xt9e8JTXEaYAzIXOPRp2voKFXLS f/KmwrbE2QwOYmRQ2Qe2t8s9IlwWo9xYyG/16rGh+cY8exoRg2K7xFQWcsacKvrW8983 DTktG7yjNruT5p2lBSIRm9sushhaf1RhA5KNWSavt+A4rjeaF+5Z1r80difsTyqjItD1 tXlQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=0YCnCtuDJee1C7b67FJIoQ+hbEjmStHSuR/6ekVWXzM=; b=C6QCbWUHGli3sbKDI+/65ZZFc3xzxgSkcZ3kJPw8LCgiu3XYCkJDhcaF5NjYUQK1SY hkY9VRIz1bGwrGNvifj1mrQnTRKgY4OQcdXZOgBc4pjyG5Ht23ewofuwMIlbxtlhmNDT VxLWkrgmacaet2JtpuOapqnFBufpdJ11D740l3C8jR/FLfCdyfeWAytPYP4p6r+jls7b fH3CQcOsPJZxChjrXpc+93WSQ1iR1SrEnJq3U6z6lSqbIxH8F8ML2xBPtuQr1TtWZR2Y 76wGfIWZTj3S6ANmAqJNNS4TFBJBfZKb6MWb8KVVRUwAiMyIcIoxQBXO/bKceW3qbtx2 Ve8Q== X-Gm-Message-State: AIkVDXLf9Z8BAJoBzeBFtyAk7kHM/bQSKfGZS0uECQLUuof6F2JStO4SHyE2d+RKG/VYGp30ufZsSivoaLNKUw== X-Received: by 10.194.119.68 with SMTP id ks4mr50571080wjb.171.1483440018066; Tue, 03 Jan 2017 02:40:18 -0800 (PST) MIME-Version: 1.0 Received: by 10.80.152.163 with HTTP; Tue, 3 Jan 2017 02:40:17 -0800 (PST) In-Reply-To: References: <1483242289-12323-1-git-send-email-wanpeng.li@hotmail.com> From: Wanpeng Li Date: Tue, 3 Jan 2017 18:40:17 +0800 Message-ID: Subject: Re: [PATCH] KVM: ioapic: fix NULL deref ioapic->lock To: Dmitry Vyukov Cc: Paolo Bonzini , LKML , KVM list , =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , Wanpeng Li Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 2017-01-03 17:27 GMT+08:00 Dmitry Vyukov : > On Mon, Jan 2, 2017 at 7:01 PM, Paolo Bonzini wrote: >> >> >> On 02/01/2017 11:17, Dmitry Vyukov wrote: >>> On Mon, Jan 2, 2017 at 11:09 AM, Paolo Bonzini wrote: >>>> >>>> >>>> >>>> On 01/01/2017 04:44, Wanpeng Li wrote: >>>>> From: Wanpeng Li >>>>> >>>>> 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 >>>>> Cc: Paolo Bonzini >>>>> Cc: Radim Krčmář >>>>> Signed-off-by: Wanpeng Li >>>>> --- >>>>> 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: } --- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html 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;