On 06/05/19 11:25, Ard Biesheuvel wrote:
> On Tue, 4 Jun 2019 at 23:44, Laszlo Ersek <ler...@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.william...@redhat.com>
>> Cc: Hao A Wu <hao.a...@intel.com>
>> Cc: Jian J Wang <jian.j.w...@intel.com>
>> Cc: Ray Ni <ray...@intel.com>
>> Cc: Star Zeng <star.z...@intel.com>
>> Signed-off-by: Laszlo Ersek <ler...@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,
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;
>> + }
>> +
>> CapabilityID = (UINT16) CapabilityEntry;
>>
>> if (CapabilityID == 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+ow...@edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub
>> [ard.biesheu...@linaro.org]
>> ------------
>>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#41934): https://edk2.groups.io/g/devel/message/41934
Mute This Topic: https://groups.io/mt/31931246/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
