From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=D6veyjuu; spf=pass (domain: linaro.org, ip: 209.85.166.195, mailfrom: ard.biesheuvel@linaro.org) Received: from mail-it1-f195.google.com (mail-it1-f195.google.com [209.85.166.195]) by groups.io with SMTP; Wed, 05 Jun 2019 06:12:46 -0700 Received: by mail-it1-f195.google.com with SMTP id h9so3352592itk.3 for ; Wed, 05 Jun 2019 06:12:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=y14lm54AzrHGrrvcgz6iOPS2ZJhpqTGA4xiui82UvVs=; b=D6veyjuuQI/NQ7073fonGboXvhr/mD7Ug9+z/CcVmcCRPTMzxdi4A/wQATXNvzu57E 53YRRumExOrGK1ZoTiSV2g7X0J/CU8Akdk/u52Dg1YwbZcVRgS3PWazwav71F0/J8wjd 1sWH4Y+NMlT8YdR9wr6GNv1I3XAxUOoS4DZsAIZmk4oZZH3Lsu5u0wWNM+hkztOStCxr N3sSRIgGmH8ddlEnopE1mxA43J3hvJ9Sk/Ne8T6nY7XvMu9BN8PxzS+F8zOCrH/8uKy8 HWl96MAZqx60f1t/wPcgZLXRjS5zfi6DmB3PR89AzGI7LR22QNiQgQNYG+emLABuW4hb lVHA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=y14lm54AzrHGrrvcgz6iOPS2ZJhpqTGA4xiui82UvVs=; b=hmNu6J28MdZeg6ez1JahHdzHQmb/vibKS00+7GBpu7DKTwgALhLGOR8d4hjJoMoY4e N5ocSAq8t4m2cnW7CmOXp0VZjUBNM5Z5EcapGplqUsYBZ8hCXQt5VX0y581ToPKX+rCx WeCBPSlEvQ7QA9RhDqnipKJdilK8+ojA+bIkz3TuxQMuDI1WF3WGt1fIiS8QbGf0tSal elOE+mMYuWryPO1ufmydHnLqLLZKT+F9zGYu1Oh2wqgJ7LcS1lCP7ZtBYLC+UXHkAGv8 v8xcABkBofPnTd5K/Mr3vQfJ06fTm3GGAKqTNmtTdz8BKN1Kx9z5WtPxw/KUeUx2tHFn t1bA== X-Gm-Message-State: APjAAAXcoR827VmpQNc7CGcLKbfKFvmOEFEuMPAlQE64yXNB96AvZQcO Mx9qHd3sO0qkgnFO2Vq4SCs95BdpN0XRXk7TvRjX6g== X-Google-Smtp-Source: APXvYqxUY6jTEtCNgYWXgKbqHL60XFZec7k7lY1DsUoXRC6tAB2NGY5I4JnOawN9m1nNoYywo/GXisAe2oHrv7S8ZeM= X-Received: by 2002:a24:4f88:: with SMTP id c130mr1105158itb.104.1559740366185; Wed, 05 Jun 2019 06:12:46 -0700 (PDT) MIME-Version: 1.0 References: <20190604214424.456-1-lersek@redhat.com> <78d6c780-bf99-9c3a-7d14-1f10b82022ad@redhat.com> In-Reply-To: <78d6c780-bf99-9c3a-7d14-1f10b82022ad@redhat.com> From: "Ard Biesheuvel" Date: Wed, 5 Jun 2019 15:12:32 +0200 Message-ID: Subject: Re: [edk2-devel] [PATCH for-next] MdeModulePkg/PciBusDxe: catch unimplemented extended config space reads To: Laszlo Ersek Cc: edk2-devel-groups-io , Alex Williamson , Hao A Wu , Jian J Wang , Ray Ni , Star Zeng Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, 5 Jun 2019 at 12:15, Laszlo Ersek wrote: > > 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) { > > > > 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 for the background Acked-by: Ard Biesheuvel