public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Ard Biesheuvel" <ard.biesheuvel@linaro.org>
To: Laszlo Ersek <lersek@redhat.com>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>,
	Alex Williamson <alex.williamson@redhat.com>,
	 Hao A Wu <hao.a.wu@intel.com>,
	Jian J Wang <jian.j.wang@intel.com>, Ray Ni <ray.ni@intel.com>,
	 Star Zeng <star.zeng@intel.com>
Subject: Re: [edk2-devel] [PATCH for-next] MdeModulePkg/PciBusDxe: catch unimplemented extended config space reads
Date: Wed, 5 Jun 2019 15:12:32 +0200	[thread overview]
Message-ID: <CAKv+Gu95cJV8MJxUq1VgeYbBy6FxGrXJe_ZmmnLx1kU=AtPGBg@mail.gmail.com> (raw)
In-Reply-To: <78d6c780-bf99-9c3a-7d14-1f10b82022ad@redhat.com>

On Wed, 5 Jun 2019 at 12:15, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 06/05/19 11:25, Ard Biesheuvel wrote:
> > On Tue, 4 Jun 2019 at 23:44, Laszlo Ersek <lersek@redhat.com> 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 <alex.williamson@redhat.com>
> >> Cc: Hao A Wu <hao.a.wu@intel.com>
> >> Cc: Jian J Wang <jian.j.wang@intel.com>
> >> Cc: Ray Ni <ray.ni@intel.com>
> >> Cc: Star Zeng <star.zeng@intel.com>
> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> >> ---
> >>
> >> 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 == 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 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
> <http://mid.mail-archive.com/20180504213637.11266-1-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
> <https://bugzilla.tianocore.org/show_bug.cgi?id=957>. 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 <ard.biesheuvel@linaro.org>

  reply	other threads:[~2019-06-05 13:12 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-04 21:44 [PATCH for-next] MdeModulePkg/PciBusDxe: catch unimplemented extended config space reads Laszlo Ersek
2019-06-05  9:12 ` [edk2-devel] " Philippe Mathieu-Daudé
2019-06-05  9:25 ` Ard Biesheuvel
2019-06-05 10:15   ` Laszlo Ersek
2019-06-05 13:12     ` Ard Biesheuvel [this message]
2019-06-10  7:03     ` Wu, Hao A
2019-06-11  8:55       ` Ni, Ray
2019-06-11 16:56 ` Laszlo Ersek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAKv+Gu95cJV8MJxUq1VgeYbBy6FxGrXJe_ZmmnLx1kU=AtPGBg@mail.gmail.com' \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox