1. Patchew
  2. EDK2
  3. View series

[edk2-devel] [PATCH] ShellPkg/Pci: Add valid check for PCI extended config space parser

IanX Kuo posted 1 patch 3 years ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/edk2 tags/patchew/568b955d98de0cc3dc52467bf394324e391d4b7c.1615954984.git.vincentx.ke@intel.com
ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
[edk2-devel] [PATCH] ShellPkg/Pci: Add valid check for PCI extended config space parser
Posted by IanX Kuo 3 years ago 
From: VincentX Ke <vincentx.ke@intel.com>

Bugzilla: 3262 (https://bugzilla.tianocore.org/show_bug.cgi?id=3262)

No need to print PCIe details while CapabilityId is 0xFFFF.
Limit the NextCapabilityOffset to PCI configuration space.

Signed-off-by: VincentX Ke <vincentx.ke@intel.com>
---
 ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c b/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
index a2f04d8db5..1e5dc75e27 100644
--- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
+++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
@@ -2038,12 +2038,14 @@ LocatePciCapability (
 
   @param[in] PciExpressCap       PCI Express capability buffer.
   @param[in] ExtendedConfigSpace PCI Express extended configuration space.
+  @param[in] ExtendedConfigSize  PCI Express extended configuration size.
   @param[in] ExtendedCapability  PCI Express extended capability ID to explain.
 **/
 VOID
 PciExplainPciExpress (
   IN  PCI_CAPABILITY_PCIEXP                  *PciExpressCap,
   IN  UINT8                                  *ExtendedConfigSpace,
+  IN  UINTN                                  ExtendedConfigSize,
   IN CONST UINT16                            ExtendedCapability
   );
 
@@ -2921,6 +2923,7 @@ ShellCommandRunPci (
         PciExplainPciExpress (
           (PCI_CAPABILITY_PCIEXP *) ((UINT8 *) &ConfigSpace + PcieCapabilityPtr),
           ExtendedConfigSpace,
+          ExtendedConfigSize,
           ExtendedCapability
           );
       }
@@ -5698,12 +5701,14 @@ PrintPciExtendedCapabilityDetails(
 
   @param[in] PciExpressCap       PCI Express capability buffer.
   @param[in] ExtendedConfigSpace PCI Express extended configuration space.
+  @param[in] ExtendedConfigSize  PCI Express extended configuration size.
   @param[in] ExtendedCapability  PCI Express extended capability ID to explain.
 **/
 VOID
 PciExplainPciExpress (
   IN  PCI_CAPABILITY_PCIEXP                  *PciExpressCap,
   IN  UINT8                                  *ExtendedConfigSpace,
+  IN  UINTN                                  ExtendedConfigSize,
   IN CONST UINT16                            ExtendedCapability
   )
 {
@@ -5786,7 +5791,7 @@ PciExplainPciExpress (
   }
 
   ExtHdr = (PCI_EXP_EXT_HDR*)ExtendedConfigSpace;
-  while (ExtHdr->CapabilityId != 0 && ExtHdr->CapabilityVersion != 0) {
+  while (ExtHdr->CapabilityId != 0 && ExtHdr->CapabilityVersion != 0 && ExtHdr->CapabilityId != 0xFFFF) {
     //
     // Process this item
     //
@@ -5800,7 +5805,8 @@ PciExplainPciExpress (
     //
     // Advance to the next item if it exists
     //
-    if (ExtHdr->NextCapabilityOffset != 0) {
+    if (ExtHdr->NextCapabilityOffset != 0 &&
+       (ExtHdr->NextCapabilityOffset <= (UINT32) (ExtendedConfigSize + EFI_PCIE_CAPABILITY_BASE_OFFSET - sizeof (PCI_EXP_EXT_HDR)))) {
       ExtHdr = (PCI_EXP_EXT_HDR*)(ExtendedConfigSpace + ExtHdr->NextCapabilityOffset - EFI_PCIE_CAPABILITY_BASE_OFFSET);
     } else {
       break;
-- 
2.18.0.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72953): https://edk2.groups.io/g/devel/message/72953
Mute This Topic: https://groups.io/mt/81397147/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] ShellPkg/Pci: Add valid check for PCI extended config space parser
Posted by Laszlo Ersek 3 years, 1 month ago 
Vincent, Ian:

something is seriously broken in your email setup. I have seen three
messages from you guys on the list, and each one of those is "from the
future". Here are the Date headers from the messages:

- Date: Thu,  8 Apr 2021 05:50:21 +0800
  https://edk2.groups.io/g/devel/message/72748

- Date: Sat, 10 Apr 2021 01:34:45 +0800
  https://edk2.groups.io/g/devel/message/72862

- Date: Sat, 10 Apr 2021 22:15:09 +0800
  https://edk2.groups.io/g/devel/message/72953

Please fix your clock setup, and post a new version.

This is why I am asking: the git-am manual says,

       --ignore-date
           By default the command records the date from the e-mail
           message as the commit author date, and uses the time of
           commit creation as the committer date. [...]

When we merge a patch using a github.com Pull Request, the "mergify bot"
preserves the Author Date field. That's a good thing in itself, but it
means that the edk2 commit history would have a patch from the future --
a patch committed in March, but "authored" in April. That's bogus.

Whoever actually applies the patch (probably the ShellPkg maintainer)
can work around the issue, by specifying the "--ignore-date" flag for
"git-am". But that's easy to forget, and not the right thing anyway.

So please just fix your broken clock, and post a new version.

Also: you should have used v1, v2, v3 in the subject prefixes (just pass
-v1, -v2, -v3 to git-format-patch). The next version that you post
should be marked "v4".

Thanks
Laszlo


On 04/10/21 16:15, IanX Kuo wrote:
> From: VincentX Ke <vincentx.ke@intel.com>
> 
> Bugzilla: 3262 (https://bugzilla.tianocore.org/show_bug.cgi?id=3262)
> 
> No need to print PCIe details while CapabilityId is 0xFFFF.
> Limit the NextCapabilityOffset to PCI configuration space.
> 
> Signed-off-by: VincentX Ke <vincentx.ke@intel.com>
> ---
>  ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c b/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
> index a2f04d8db5..1e5dc75e27 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
> @@ -2038,12 +2038,14 @@ LocatePciCapability (
>  
>    @param[in] PciExpressCap       PCI Express capability buffer.
>    @param[in] ExtendedConfigSpace PCI Express extended configuration space.
> +  @param[in] ExtendedConfigSize  PCI Express extended configuration size.
>    @param[in] ExtendedCapability  PCI Express extended capability ID to explain.
>  **/
>  VOID
>  PciExplainPciExpress (
>    IN  PCI_CAPABILITY_PCIEXP                  *PciExpressCap,
>    IN  UINT8                                  *ExtendedConfigSpace,
> +  IN  UINTN                                  ExtendedConfigSize,
>    IN CONST UINT16                            ExtendedCapability
>    );
>  
> @@ -2921,6 +2923,7 @@ ShellCommandRunPci (
>          PciExplainPciExpress (
>            (PCI_CAPABILITY_PCIEXP *) ((UINT8 *) &ConfigSpace + PcieCapabilityPtr),
>            ExtendedConfigSpace,
> +          ExtendedConfigSize,
>            ExtendedCapability
>            );
>        }
> @@ -5698,12 +5701,14 @@ PrintPciExtendedCapabilityDetails(
>  
>    @param[in] PciExpressCap       PCI Express capability buffer.
>    @param[in] ExtendedConfigSpace PCI Express extended configuration space.
> +  @param[in] ExtendedConfigSize  PCI Express extended configuration size.
>    @param[in] ExtendedCapability  PCI Express extended capability ID to explain.
>  **/
>  VOID
>  PciExplainPciExpress (
>    IN  PCI_CAPABILITY_PCIEXP                  *PciExpressCap,
>    IN  UINT8                                  *ExtendedConfigSpace,
> +  IN  UINTN                                  ExtendedConfigSize,
>    IN CONST UINT16                            ExtendedCapability
>    )
>  {
> @@ -5786,7 +5791,7 @@ PciExplainPciExpress (
>    }
>  
>    ExtHdr = (PCI_EXP_EXT_HDR*)ExtendedConfigSpace;
> -  while (ExtHdr->CapabilityId != 0 && ExtHdr->CapabilityVersion != 0) {
> +  while (ExtHdr->CapabilityId != 0 && ExtHdr->CapabilityVersion != 0 && ExtHdr->CapabilityId != 0xFFFF) {
>      //
>      // Process this item
>      //
> @@ -5800,7 +5805,8 @@ PciExplainPciExpress (
>      //
>      // Advance to the next item if it exists
>      //
> -    if (ExtHdr->NextCapabilityOffset != 0) {
> +    if (ExtHdr->NextCapabilityOffset != 0 &&
> +       (ExtHdr->NextCapabilityOffset <= (UINT32) (ExtendedConfigSize + EFI_PCIE_CAPABILITY_BASE_OFFSET - sizeof (PCI_EXP_EXT_HDR)))) {
>        ExtHdr = (PCI_EXP_EXT_HDR*)(ExtendedConfigSpace + ExtHdr->NextCapabilityOffset - EFI_PCIE_CAPABILITY_BASE_OFFSET);
>      } else {
>        break;
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72991): https://edk2.groups.io/g/devel/message/72991
Mute This Topic: https://groups.io/mt/81397147/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] ShellPkg/Pci: Add valid check for PCI extended config space parser
Posted by IanX Kuo 3 years, 1 month ago 
Hi Laszlo

Thanks for the remind. I will take care the date and Patch v4 in our next patch.

Thanks,
Ian Kuo
-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com> 
Sent: Thursday, March 18, 2021 5:01 AM
To: devel@edk2.groups.io; Kuo, IanX <ianx.kuo@intel.com>
Cc: Ke, VincentX <vincentx.ke@intel.com>
Subject: Re: [edk2-devel] [PATCH] ShellPkg/Pci: Add valid check for PCI extended config space parser

Vincent, Ian:

something is seriously broken in your email setup. I have seen three messages from you guys on the list, and each one of those is "from the future". Here are the Date headers from the messages:

- Date: Thu,  8 Apr 2021 05:50:21 +0800
  https://edk2.groups.io/g/devel/message/72748

- Date: Sat, 10 Apr 2021 01:34:45 +0800
  https://edk2.groups.io/g/devel/message/72862

- Date: Sat, 10 Apr 2021 22:15:09 +0800
  https://edk2.groups.io/g/devel/message/72953

Please fix your clock setup, and post a new version.

This is why I am asking: the git-am manual says,

       --ignore-date
           By default the command records the date from the e-mail
           message as the commit author date, and uses the time of
           commit creation as the committer date. [...]

When we merge a patch using a github.com Pull Request, the "mergify bot"
preserves the Author Date field. That's a good thing in itself, but it means that the edk2 commit history would have a patch from the future -- a patch committed in March, but "authored" in April. That's bogus.

Whoever actually applies the patch (probably the ShellPkg maintainer) can work around the issue, by specifying the "--ignore-date" flag for "git-am". But that's easy to forget, and not the right thing anyway.

So please just fix your broken clock, and post a new version.

Also: you should have used v1, v2, v3 in the subject prefixes (just pass -v1, -v2, -v3 to git-format-patch). The next version that you post should be marked "v4".

Thanks
Laszlo


On 04/10/21 16:15, IanX Kuo wrote:
> From: VincentX Ke <vincentx.ke@intel.com>
> 
> Bugzilla: 3262 (https://bugzilla.tianocore.org/show_bug.cgi?id=3262)
> 
> No need to print PCIe details while CapabilityId is 0xFFFF.
> Limit the NextCapabilityOffset to PCI configuration space.
> 
> Signed-off-by: VincentX Ke <vincentx.ke@intel.com>
> ---
>  ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c 
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
> index a2f04d8db5..1e5dc75e27 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
> @@ -2038,12 +2038,14 @@ LocatePciCapability (
>  
>    @param[in] PciExpressCap       PCI Express capability buffer.
>    @param[in] ExtendedConfigSpace PCI Express extended configuration space.
> +  @param[in] ExtendedConfigSize  PCI Express extended configuration size.
>    @param[in] ExtendedCapability  PCI Express extended capability ID to explain.
>  **/
>  VOID
>  PciExplainPciExpress (
>    IN  PCI_CAPABILITY_PCIEXP                  *PciExpressCap,
>    IN  UINT8                                  *ExtendedConfigSpace,
> +  IN  UINTN                                  ExtendedConfigSize,
>    IN CONST UINT16                            ExtendedCapability
>    );
>  
> @@ -2921,6 +2923,7 @@ ShellCommandRunPci (
>          PciExplainPciExpress (
>            (PCI_CAPABILITY_PCIEXP *) ((UINT8 *) &ConfigSpace + PcieCapabilityPtr),
>            ExtendedConfigSpace,
> +          ExtendedConfigSize,
>            ExtendedCapability
>            );
>        }
> @@ -5698,12 +5701,14 @@ PrintPciExtendedCapabilityDetails(
>  
>    @param[in] PciExpressCap       PCI Express capability buffer.
>    @param[in] ExtendedConfigSpace PCI Express extended configuration space.
> +  @param[in] ExtendedConfigSize  PCI Express extended configuration size.
>    @param[in] ExtendedCapability  PCI Express extended capability ID to explain.
>  **/
>  VOID
>  PciExplainPciExpress (
>    IN  PCI_CAPABILITY_PCIEXP                  *PciExpressCap,
>    IN  UINT8                                  *ExtendedConfigSpace,
> +  IN  UINTN                                  ExtendedConfigSize,
>    IN CONST UINT16                            ExtendedCapability
>    )
>  {
> @@ -5786,7 +5791,7 @@ PciExplainPciExpress (
>    }
>  
>    ExtHdr = (PCI_EXP_EXT_HDR*)ExtendedConfigSpace;
> -  while (ExtHdr->CapabilityId != 0 && ExtHdr->CapabilityVersion != 0) 
> {
> +  while (ExtHdr->CapabilityId != 0 && ExtHdr->CapabilityVersion != 0 
> + && ExtHdr->CapabilityId != 0xFFFF) {
>      //
>      // Process this item
>      //
> @@ -5800,7 +5805,8 @@ PciExplainPciExpress (
>      //
>      // Advance to the next item if it exists
>      //
> -    if (ExtHdr->NextCapabilityOffset != 0) {
> +    if (ExtHdr->NextCapabilityOffset != 0 &&
> +       (ExtHdr->NextCapabilityOffset <= (UINT32) (ExtendedConfigSize 
> + + EFI_PCIE_CAPABILITY_BASE_OFFSET - sizeof (PCI_EXP_EXT_HDR)))) {
>        ExtHdr = (PCI_EXP_EXT_HDR*)(ExtendedConfigSpace + ExtHdr->NextCapabilityOffset - EFI_PCIE_CAPABILITY_BASE_OFFSET);
>      } else {
>        break;
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72992): https://edk2.groups.io/g/devel/message/72992
Mute This Topic: https://groups.io/mt/81397147/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.