From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.31, mailfrom: ray.ni@intel.com) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by groups.io with SMTP; Tue, 11 Jun 2019 01:55:14 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Jun 2019 01:55:13 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.63,578,1557212400"; d="scan'208";a="183728310" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by fmsmga002.fm.intel.com with ESMTP; 11 Jun 2019 01:55:13 -0700 Received: from shsmsx106.ccr.corp.intel.com (10.239.4.159) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.408.0; Tue, 11 Jun 2019 01:55:13 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.137]) by SHSMSX106.ccr.corp.intel.com ([169.254.10.113]) with mapi id 14.03.0415.000; Tue, 11 Jun 2019 16:55:11 +0800 From: "Ni, Ray" To: "Wu, Hao A" , "devel@edk2.groups.io" , "lersek@redhat.com" CC: Alex Williamson , "Wang, Jian J" , Ard Biesheuvel , "Zeng, Star" Subject: Re: [edk2-devel] [PATCH for-next] MdeModulePkg/PciBusDxe: catch unimplemented extended config space reads Thread-Topic: [edk2-devel] [PATCH for-next] MdeModulePkg/PciBusDxe: catch unimplemented extended config space reads Thread-Index: AQHVGx6+2doFEy4wZUiH/UMhKL+agaaMROAAgAAN6ICAB6YsgIACN3hw Date: Tue, 11 Jun 2019 08:55:10 +0000 Deferred-Delivery: Tue, 11 Jun 2019 08:55:00 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C1A2641@SHSMSX104.ccr.corp.intel.com> References: <20190604214424.456-1-lersek@redhat.com> <78d6c780-bf99-9c3a-7d14-1f10b82022ad@redhat.com> In-Reply-To: Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: ray.ni@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Reviewed-by: Ray Ni > -----Original Message----- > From: Wu, Hao A > Sent: Monday, June 10, 2019 3:04 PM > To: devel@edk2.groups.io; lersek@redhat.com; Ni, Ray > Cc: Alex Williamson ; Wang, Jian J > ; Ard Biesheuvel ; Zen= g, > Star > Subject: RE: [edk2-devel] [PATCH for-next] MdeModulePkg/PciBusDxe: > catch unimplemented extended config space reads >=20 > Hello Ray, >=20 > Do you have comments on this patch? >=20 > > -----Original Message----- > > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > > Laszlo Ersek > > Sent: Wednesday, June 05, 2019 6:15 PM > > To: Ard Biesheuvel; edk2-devel-groups-io > > Cc: Alex Williamson; Wu, Hao A; Wang, Jian J; Ni, Ray; Zeng, Star > > Subject: Re: [edk2-devel] [PATCH for-next] MdeModulePkg/PciBusDxe: > > catch unimplemented extended config space reads > > > > 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 > > may > > >> 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 > > >> above is when a Conventional PCI bus -- exposed by a PCIe-to-PCI > > >> bridge in the topology -- intervenes between a PCI Express Root > > >> Port and a PCI Express 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(), > > >> and 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/MdeModulePkg/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) { > > > > > > 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 - This field contains the offset to the nex= t > > PCI Express Capability structure or 000h if no other items exist i= n > > the linked list of Capabilities. > > > > For Extended Capabilities implemented in Configuration Space, this > > offset is relative to the beginning of PCI compatible Configuratio= n > > 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 > > > lersek@redhat.com>.) > > 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 > > > > > > > >> + 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; > > >> + } > > >> + >=20 > Acked-by: Hao A Wu >=20 > Best Regards, > Hao Wu >=20 > > >> 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/41893 > > >> 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@linaro.org] > > >> ------------ > > >> > > > > > >=20