From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Wed, 05 Jun 2019 03:15:27 -0700 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id DC635C07014A; Wed, 5 Jun 2019 10:15:23 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-4.ams2.redhat.com [10.36.117.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 7B11B19C65; Wed, 5 Jun 2019 10:15:16 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH for-next] MdeModulePkg/PciBusDxe: catch unimplemented extended config space reads To: Ard Biesheuvel , edk2-devel-groups-io Cc: Alex Williamson , Hao A Wu , Jian J Wang , Ray Ni , Star Zeng References: <20190604214424.456-1-lersek@redhat.com> From: "Laszlo Ersek" Message-ID: <78d6c780-bf99-9c3a-7d14-1f10b82022ad@redhat.com> Date: Wed, 5 Jun 2019 12:15:15 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Wed, 05 Jun 2019 10:15:24 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 06/05/19 11:25, Ard Biesheuvel wrote: > On Tue, 4 Jun 2019 at 23:44, Laszlo Ersek wrote: >> >> When assigning a physical PCIe device to a QEMU/KVM guest, PciBusDxe m= ay >> find that the extended config space is not (fully) implemented. In >> LocatePciExpressCapabilityRegBlock(), "CapabilityEntry" may be read as >> 0xFFFF_FFFF at a given config space offset, after which the loop gets >> stuck spinning on offset 0xFFC (the read at offset 0xFFC returns >> 0xFFFF_FFFF most likely as well). >> >> Another scenario (not related to virtualization) for triggering the ab= ove >> is when a Conventional PCI bus -- exposed by a PCIe-to-PCI bridge in t= he >> topology -- intervenes between a PCI Express Root Port and a PCI Expre= ss >> Endpoint. The Conventional PCI bus limits the accessible config space = of >> the PCI Express Endpoint, even though the endpoint advertizes the PCI >> Express capability. Here's a diagram, courtesy of Alex Williamson: >> >> [PCIe Root Port]--[PCIe-to-PCI]--[PCI-to-PCIe]--[PCIe EP] >> ->| |<- Conventional PCI bus >> >> Catch reads of 0xFFFF_FFFF in LocatePciExpressCapabilityRegBlock(), an= d >> break out of the scan with a warning message. The function will return >> EFI_NOT_FOUND. >> >> Cc: Alex Williamson >> Cc: Hao A Wu >> Cc: Jian J Wang >> Cc: Ray Ni >> Cc: Star Zeng >> Signed-off-by: Laszlo Ersek >> --- >> >> Notes: >> Repo: https://github.com/lersek/edk2.git >> Branch: pcibus_no_ext_conf >> >> MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c b/MdeModulePk= g/Bus/Pci/PciBusDxe/PciCommand.c >> index 214aeecdd40a..6283d602207c 100644 >> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c >> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c >> @@ -236,6 +236,19 @@ LocatePciExpressCapabilityRegBlock ( >> break; >> } >> >> + if (CapabilityEntry =3D=3D MAX_UINT32) { >=20 > Should we check here that the offset > 0x100 ? Otherwise, this affects > more than just the extended config space. A separate function exists for locating caps in the conventional config space (LocateCapabilityRegBlock()). Whereas the function being patched -- LocatePciExpressCapabilityRegBlock() -- is supposed to start with a capability offset into the extended config space, passed in by the caller via *Offset, or else at 0x100 if *Offset is 0. And, my understanding is that an extended cap shall never chain to a conventional cap. The spec says, Next Capability Offset =E2=80=93 This field contains the offset to th= e next PCI Express Capability structure or 000h if no other items exist in the linked list of Capabilities. For Extended Capabilities implemented in Configuration Space, this offset is relative to the beginning of PCI compatible Configuration Space and thus must always be either 000h (for terminating list of Capabilities) or greater than 0FFh. The bottom 2 bits of this offset are Reserved and must be implemented as 00b although software must mask them to allow for future uses of these bits. Additionally, the capability header is different for conventional capabilities: EFI_PCI_CAPABILITY_HDR -- 2 bytes -- vs. PCI_EXPRESS_EXTENDED_CAPABILITIES_HEADER -- 4 bytes. So if this loop ever crossed over into normal config space, it would break horribly, regardless of this patch. A more general question would be how much we should armor such functions -- i.e., capability list scanning -- with sanity checks. My answer to that was authoring PciCapLib, which detects loops in cap lists, oversized capability reads/writes, an absent extended config space in spite of a present express capability; maybe more. Basically everything I could think of and/or had encountered by then. You probably remember that I originally attempted to get PciCapLib and its accessories into MdePkg, with an intent to rebase core PCI drivers to them -- including PciBusDxe. (The original "sales pitch" can be found at .) I hadn't received either positive or negative feedback regarding that idea for a month or so, after which we merged the library into OvmfPkg, in the end. (And it is now used by ArmVirtQemu* and OVMF only, as part of OvmfPkg/PciHotPlugInitDxe and OvmfPkg/Virtio10Dxe). I did file a longer-term reminder BZ at . But, I gave up on that as well in about 4 months. The upshot is that now I can only contribute piecemeal fixes for PciBusDxe, whenever I come across something. This particular issue has bitten us at RH twice by now -- unfortunately, both RHBZs are private, hence I didn't reference them in the commit message. (It's super annoying if you click a BZ link, just to be rejected access.) In summary, adding a standalone check for "next" cap offsets that fall into the forbidden range [1, 255] (inclusive) would be worthwhile, in theory. (In fact PciCapLib happens to contain a check for that too.) But that's a different patch, and we haven't run into that situation yet, in practice. So I'd think it's out of scope specifically for PciBusDxe, at this point. (Key phrase being "piecemeal fixes".) Thanks, Laszlo >=20 >> + DEBUG (( >> + DEBUG_WARN, >> + "%a: [%02x|%02x|%02x] failed to access config space at offset= 0x%x\n", >> + __FUNCTION__, >> + PciIoDevice->BusNumber, >> + PciIoDevice->DeviceNumber, >> + PciIoDevice->FunctionNumber, >> + CapabilityPtr >> + )); >> + break; >> + } >> + >> CapabilityID =3D (UINT16) CapabilityEntry; >> >> if (CapabilityID =3D=3D CapId) { >> -- >> 2.19.1.3.g30247aa5d201 >> >> >> ------------ >> Groups.io Links: You receive all messages sent to this group. >> >> View/Reply Online (#41893): https://edk2.groups.io/g/devel/message/418= 93 >> Mute This Topic: https://groups.io/mt/31931246/1761188 >> Group Owner: devel+owner@edk2.groups.io >> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ard.biesheuvel@lin= aro.org] >> ------------ >>