[PATCH for-next] MdeModulePkg/PciBusDxe: catch unimplemented extended config space reads

[PATCH for-next] MdeModulePkg/PciBusDxe: catch unimplemented extended config space reads

[Laszlo Ersek]

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) {
+      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

Re: [edk2-devel] [PATCH for-next] MdeModulePkg/PciBusDxe: catch unimplemented extended config space reads

[Philippe Mathieu-Daudé]

On 6/4/19 11:44 PM, 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 <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) {
> +      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) {
> 

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

Re: [edk2-devel] [PATCH for-next] MdeModulePkg/PciBusDxe: catch unimplemented extended config space reads

[Ard Biesheuvel]

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.

> +      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+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub  [ard.biesheuvel@linaro.org]
> ------------
>

Re: [edk2-devel] [PATCH for-next] MdeModulePkg/PciBusDxe: catch unimplemented extended config space reads

[Laszlo Ersek]

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,
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+owner@edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub  [ard.biesheuvel@linaro.org]
>> ------------
>>

Re: [edk2-devel] [PATCH for-next] MdeModulePkg/PciBusDxe: catch unimplemented extended config space reads

[Ard Biesheuvel]

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>

Re: [edk2-devel] [PATCH for-next] MdeModulePkg/PciBusDxe: catch unimplemented extended config space reads

[Wu, Hao A]

Hello Ray,

Do you have comments on this patch?

> -----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 <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,
> 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;
> >> +    }
> >> +

Acked-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu

> >>      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+owner@edk2.groups.io
> >> Unsubscribe: https://edk2.groups.io/g/devel/unsub
> [ard.biesheuvel@linaro.org]
> >> ------------
> >>
> 
> 
> 

Re: [edk2-devel] [PATCH for-next] MdeModulePkg/PciBusDxe: catch unimplemented extended config space reads

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Monday, June 10, 2019 3:04 PM
> To: devel@edk2.groups.io; lersek@redhat.com; Ni, Ray <ray.ni@intel.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Zeng,
> Star <star.zeng@intel.com>
> Subject: RE: [edk2-devel] [PATCH for-next] MdeModulePkg/PciBusDxe:
> catch unimplemented extended config space reads
> 
> Hello Ray,
> 
> Do you have comments on this patch?
> 
> > -----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 <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,
> > 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;
> > >> +    }
> > >> +
> 
> Acked-by: Hao A Wu <hao.a.wu@intel.com>
> 
> Best Regards,
> Hao Wu
> 
> > >>      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+owner@edk2.groups.io
> > >> Unsubscribe: https://edk2.groups.io/g/devel/unsub
> > [ard.biesheuvel@linaro.org]
> > >> ------------
> > >>
> >
> >
> > 

Re: [edk2-devel] [PATCH for-next] MdeModulePkg/PciBusDxe: catch unimplemented extended config space reads

[Laszlo Ersek]

On 06/04/19 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 <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) {
> +      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) {
> 

Thank you everyone, patch pushed as commit e5b4d825afc4.

Laszlo