From patchwork Fri Aug 23 00:02:52 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Bjorn Helgaas X-Patchwork-Id: 2848467 X-Patchwork-Delegate: bhelgaas@google.com Return-Path: X-Original-To: patchwork-linux-pci@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id DFA1C9F271 for ; Fri, 23 Aug 2013 00:03:19 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id D265D2033F for ; Fri, 23 Aug 2013 00:03:18 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 9DB4E20328 for ; Fri, 23 Aug 2013 00:03:17 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754470Ab3HWADA (ORCPT ); Thu, 22 Aug 2013 20:03:00 -0400 Received: from mail-oa0-f47.google.com ([209.85.219.47]:39458 "EHLO mail-oa0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754081Ab3HWAC7 (ORCPT ); Thu, 22 Aug 2013 20:02:59 -0400 Received: by mail-oa0-f47.google.com with SMTP id g12so4858520oah.20 for ; Thu, 22 Aug 2013 17:02:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=S8HPHs4L5A/DLaG00lUf2DarccXQrDS8zDjMk4EOynA=; b=iK7pueh7Vr73HMnmSJ9ln7oyj2P3b5AyLAIMDByVJX1QVysBzUS1OgmYwdvbsIgkM6 iP8M9LMHcd9IPl7l/qt8lZTYivSxefuP4hd6chTqQeOgCzHUkCTDgcGaKHYVtw/ntkvr Aj8mI/GyRHtF4MgRIKCajgdlz7BxJspPmhYOLX8KHrXYkSREr0dsrDzOBnPwBHEsV297 l+7xfBr9mM8H9Y9a7+WoDyI8Vi8svOcuGsytQkEFNiyS0lZXI7Vn3LVMePN6raq1Dxl7 ykEFJMW3wiydrgyP1N4sjHwH6EdsMw6IdJ9Le9pnxzkPKqb+cx5/UqAXugt0np5AHs1L z4zA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-type:content-disposition :content-transfer-encoding:in-reply-to:user-agent; bh=S8HPHs4L5A/DLaG00lUf2DarccXQrDS8zDjMk4EOynA=; b=mj2bVFAT9pNGlQF0Wmq5UhMwabtaZ8NVXHjeedgzcd4WmvgmuN1q3dSSoKtfxQiC7t uD3zqt1eyrXVAz5XKaDDhysS7s8eYSfs06AktS4oe4Ty+3cTK+lNNzkGw4GDruH/209Q bCghhu2BIAmhe46K2rc+fGCJZf6MJFtUz2ncDthi9JTPp7oYMwsHvFMeqWn0GyUhhm4h /rAz1rB1R+ykzmTDhrJx2M9AhNx4Yot10+gjoYHWGEHw6QJyewPTaFiHywEjlA50g13I T+XR5QZRJYmgMtOMg6zbx+u1V9t+CYHIJkStZUepEV1rcnVh5UF33oZ4W8RYXob/R+WW 7ppg== X-Gm-Message-State: ALoCoQlSmDPlKXEuepCe84ghbr+THcMDvV8dDfpZbbuulirGptWQGUzLxLYrFwLBT7lChh3OhSQt409l+r/+XprL7z4Z1ld0dx/ZKGL00qRARhOwnghT7/AHv9HBzxV6TuM7Cj7th2o+FKR+JAOmSK38GMij22BGej+irHuXui1jelgJRbqXEtcnay1RxfS2OUDz6N0aJotGCLFInug/+gPmvfXTNDb/yA== X-Received: by 10.60.65.37 with SMTP id u5mr4642024oes.72.1377216178242; Thu, 22 Aug 2013 17:02:58 -0700 (PDT) Received: from google.com ([172.16.48.26]) by mx.google.com with ESMTPSA id rr6sm22502543oeb.0.1969.12.31.16.00.00 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Thu, 22 Aug 2013 17:02:57 -0700 (PDT) Date: Thu, 22 Aug 2013 18:02:52 -0600 From: Bjorn Helgaas To: Radim =?utf-8?B?S3LEjW3DocWZ?= Cc: linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org, "Michael S. Tsirkin" , Alex Williamson , Myron Stowe , Joe Lawrence , Kenji Kaneshige , Isaku Yamahata Subject: Re: [PATCH] PCI: avoid NULL deref in alloc_pcie_link_state Message-ID: <20130823000252.GA29441@google.com> References: <1375970227-14794-1-git-send-email-rkrcmar@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1375970227-14794-1-git-send-email-rkrcmar@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org X-Spam-Status: No, score=-9.6 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, T_DKIM_INVALID, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Thu, Aug 08, 2013 at 03:57:07PM +0200, Radim Kr?má? wrote: > PCIe switch can be connected directly to the PCIe root complex in QEMU; > ASPM does not expect this topology and dereferences NULL pointer when > initializing. > > Downstream port can be also connected to the root complex without > upstream one, so code checks for both, otherwise they dereference NULL > on line drivers/pci/pcie/aspm.c:530 (alloc_pcie_link_state+13): > parent = pdev->bus->parent->self->link_state; > "pdev->bus->parent->self == NULL" if upstream port is connected directly > to the root bus and "pdev->bus->parent == NULL" in the second case. > > v1 -> v2: (https://lkml.org/lkml/2013/6/19/753) > - Initialization is aborted in pcie_aspm_init_link_state, where other > special cases are being handled > - pci_is_root_bus is used > - Warning is printed > > Reproducer for "downstream -- root" and "downstream -- upstream -- root" > (used qemu-kvm 1.5, q35 machine type might be missing on older ones) > > for parent in pcie.0 upstream; do > qemu-kvm -m 128 -M q35 -nographic -no-reboot \ > -device x3130-upstream,bus=pcie.0,id=upstream \ > -device xio3130-downstream,bus=$parent,id=downstream,chassis=1 \ > -device virtio-blk-pci,bus=downstream,id=virtio-zero,drive=zero \ > -drive file=/dev/zero,id=zero,format=raw \ > -kernel bzImage -append "console=ttyS0 panic=3" # pcie_aspm=off > done > > ASPM in QEMU works if we connect upstream through root port > -device ioh3420,bus=pcie.0,id=root.0 \ > -device x3130-upstream,bus=root.0,id=upstream > > Signed-off-by: Radim Kr?má? > --- > drivers/pci/pcie/aspm.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > index 403a443..209cd7f 100644 > --- a/drivers/pci/pcie/aspm.c > +++ b/drivers/pci/pcie/aspm.c > @@ -570,6 +570,15 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev) > pdev->bus->self) > return; > > + /* We require at least two ports between downstream and root bus */ > + if (pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM && > + (pci_is_root_bus(pdev->bus) || > + pci_is_root_bus(pdev->bus->parent))) { > + dev_warn(&pdev->dev, "ASPM disabled" > + " (connected directly to root bus)\n"); > + return; > + } I don't really want to detect invalid topologies piecemeal -- we will likely find other areas (MPS, AER, link speed management, etc.) that have similar dependencies. I'd rather do it generically, maybe with something like the following patch. I tried this with the following qemu invocation: $ /usr/local/bin/qemu-system-x86_64 -M q35 -enable-kvm -m 512 -device x3130-upstream,bus=pcie.0,id=upstream -device xio3130-downstream,bus=upstream,id=downstream,chassis=1 -drive file=ubuntu.img,if=none,id=mydisk -device ide-drive,drive=mydisk,bus=ide.0 -drive file=scratch.img,id=disk1 -device virtio-blk-pci,bus=downstream,id=virtio-disk1,drive=disk1 -nographic -kernel ~/linux/arch/x86/boot/bzImage -append "console=ttyS0,115200n8 root=/dev/sda1 ignore_loglevel" With unmodified v3.11-rc2, I see the NULL pointer dereference, but with the patch below applied, we just ignore the 00:03.0 device and the kernel boots fine. Bjorn --- arch/powerpc/kernel/pci_of_scan.c | 7 ++++++- arch/sparc/kernel/pci.c | 7 ++++++- drivers/pci/probe.c | 37 +++++++++++++++++++++++++++++++++---- include/linux/pci.h | 2 +- 4 files changed, 46 insertions(+), 7 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-pci" 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/powerpc/kernel/pci_of_scan.c b/arch/powerpc/kernel/pci_of_scan.c index 6b0ba58..f6ef4dd 100644 --- a/arch/powerpc/kernel/pci_of_scan.c +++ b/arch/powerpc/kernel/pci_of_scan.c @@ -143,7 +143,6 @@ struct pci_dev *of_create_pci_dev(struct device_node *node, dev->devfn = devfn; dev->multifunction = 0; /* maybe a lie? */ dev->needs_freset = 0; /* pcie fundamental reset required */ - set_pcie_port_type(dev); list_for_each_entry(slot, &dev->bus->slots, list) if (PCI_SLOT(dev->devfn) == slot->number) @@ -164,6 +163,12 @@ struct pci_dev *of_create_pci_dev(struct device_node *node, pr_debug(" class: 0x%x\n", dev->class); pr_debug(" revision: 0x%x\n", dev->revision); + if (set_pcie_port_type(dev)) { + pci_bus_put(dev->bus); + kfree(dev); + return NULL; + } + dev->current_state = PCI_UNKNOWN; /* unknown power state */ dev->error_state = pci_channel_io_normal; dev->dma_mask = 0xffffffff; diff --git a/arch/sparc/kernel/pci.c b/arch/sparc/kernel/pci.c index bc4d3f5..5600849 100644 --- a/arch/sparc/kernel/pci.c +++ b/arch/sparc/kernel/pci.c @@ -287,7 +287,6 @@ static struct pci_dev *of_create_pci_dev(struct pci_pbm_info *pbm, dev->dev.of_node = of_node_get(node); dev->devfn = devfn; dev->multifunction = 0; /* maybe a lie? */ - set_pcie_port_type(dev); list_for_each_entry(slot, &dev->bus->slots, list) if (PCI_SLOT(dev->devfn) == slot->number) @@ -319,6 +318,12 @@ static struct pci_dev *of_create_pci_dev(struct pci_pbm_info *pbm, printk(" class: 0x%x device name: %s\n", dev->class, pci_name(dev)); + if (set_pcie_port_type(dev)) { + pci_bus_put(dev->bus); + kfree(dev); + return NULL; + } + /* I have seen IDE devices which will not respond to * the bmdma simplex check reads if bus mastering is * disabled. diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index cf57fe7..a8cc929 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -970,20 +970,47 @@ static void pci_read_irq(struct pci_dev *dev) dev->irq = irq; } -void set_pcie_port_type(struct pci_dev *pdev) +int set_pcie_port_type(struct pci_dev *pdev) { - int pos; + int pos, type; u16 reg16; + struct pci_dev *parent; pos = pci_find_capability(pdev, PCI_CAP_ID_EXP); if (!pos) - return; + return 0; + pdev->is_pcie = 1; pdev->pcie_cap = pos; pci_read_config_word(pdev, pos + PCI_EXP_FLAGS, ®16); pdev->pcie_flags_reg = reg16; pci_read_config_word(pdev, pos + PCI_EXP_DEVCAP, ®16); pdev->pcie_mpss = reg16 & PCI_EXP_DEVCAP_PAYLOAD; + + type = pci_pcie_type(pdev); + parent = pdev->bus->self; + + /* An Upstream Port must be below a Root Port or a Downstream Port */ + if (type == PCI_EXP_TYPE_UPSTREAM) { + if (!parent || + (pci_pcie_type(parent) != PCI_EXP_TYPE_ROOT_PORT && + pci_pcie_type(parent) != PCI_EXP_TYPE_DOWNSTREAM)) { + dev_warn(&pdev->dev, + "ignoring improperly connected PCIe switch\n"); + return -EINVAL; + } + } + + /* A Downstream Port must be directly below an Upstream Port */ + if (type == PCI_EXP_TYPE_DOWNSTREAM) { + if (!parent || + pci_pcie_type(parent) != PCI_EXP_TYPE_UPSTREAM) { + dev_warn(&pdev->dev, "ignoring malformed PCIe switch (no Upstream Port)\n"); + return -EINVAL; + } + } + + return 0; } void set_pcie_hotplug_bridge(struct pci_dev *pdev) @@ -1025,7 +1052,6 @@ int pci_setup_device(struct pci_dev *dev) dev->hdr_type = hdr_type & 0x7f; dev->multifunction = !!(hdr_type & 0x80); dev->error_state = pci_channel_io_normal; - set_pcie_port_type(dev); list_for_each_entry(slot, &dev->bus->slots, list) if (PCI_SLOT(dev->devfn) == slot->number) @@ -1046,6 +1072,9 @@ int pci_setup_device(struct pci_dev *dev) dev_printk(KERN_DEBUG, &dev->dev, "[%04x:%04x] type %02x class %#08x\n", dev->vendor, dev->device, dev->hdr_type, dev->class); + if (set_pcie_port_type(dev)) + return -EINVAL; + /* need to have dev->class ready */ dev->cfg_size = pci_cfg_space_size(dev); diff --git a/include/linux/pci.h b/include/linux/pci.h index 89ed123..a61134b 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -982,7 +982,7 @@ void pci_disable_ltr(struct pci_dev *dev); int pci_set_ltr(struct pci_dev *dev, int snoop_lat_ns, int nosnoop_lat_ns); /* For use by arch with custom probe code */ -void set_pcie_port_type(struct pci_dev *pdev); +int set_pcie_port_type(struct pci_dev *pdev); void set_pcie_hotplug_bridge(struct pci_dev *pdev); /* Functions for PCI Hotplug drivers to use */