[edk2-devel] [PATCH v1 1/2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues

[edk2-devel] [PATCH v1 1/2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues

[Ranbir Singh]

From: Ranbir Singh <Ranbir.Singh3@Dell.com>

The function NotifyPhase has a check

    ASSERT (Index < TypeMax);

but this comes into play only in DEBUG mode. In Release mode, there is
no handling if the Index value is within array limits or not. If for
whatever reasons, the Index does not get re-assigned to Index2 at line
137, then it remains at TypeMax as assigned earlier at line 929. This
poses array overrun risk at lines 942 and 943. It is better to deploy
a safety check on Index limit before accessing array elements.

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4212

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
---
 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
index d573e532bac8..519e1369f85e 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
@@ -939,6 +939,11 @@ NotifyPhase (
             }
 
             ASSERT (Index < TypeMax);
+
+            if (Index >= TypeMax) {
+                continue;
+            }
+
             ResNodeHandled[Index] = TRUE;
             Alignment             = RootBridge->ResAllocNode[Index].Alignment;
             BitsOfAlignment       = LowBitSet64 (Alignment + 1);
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106997): https://edk2.groups.io/g/devel/message/106997
Mute This Topic: https://groups.io/mt/100212112/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 0/2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix issues pointed by Coverity

[Ranbir Singh]

Ranbir Singh (2):
  MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues
  MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix MISSING_BREAK Coverity
    issue

 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 9 +++++++++
 1 file changed, 9 insertions(+)

-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107001): https://edk2.groups.io/g/devel/message/107001
Mute This Topic: https://groups.io/mt/100212111/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 1/2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues

[Ranbir Singh]

From: Ranbir Singh <Ranbir.Singh3@Dell.com>

The function NotifyPhase has a check

    ASSERT (Index < TypeMax);

but this comes into play only in DEBUG mode. In Release mode, there is
no handling if the Index value is within array limits or not. If for
whatever reasons, the Index does not get re-assigned to Index2 at line
937, then it remains at TypeMax as assigned earlier at line 929. This
poses array overrun risk at lines 942 and 943. It is better to deploy
a safety check on Index limit before accessing array elements.

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4212

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
---
 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
index d573e532bac8..519e1369f85e 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
@@ -939,6 +939,11 @@ NotifyPhase (
             }
 
             ASSERT (Index < TypeMax);
+
+            if (Index >= TypeMax) {
+                continue;
+            }
+
             ResNodeHandled[Index] = TRUE;
             Alignment             = RootBridge->ResAllocNode[Index].Alignment;
             BitsOfAlignment       = LowBitSet64 (Alignment + 1);
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107002): https://edk2.groups.io/g/devel/message/107002
Mute This Topic: https://groups.io/mt/100212116/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 2/2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix MISSING_BREAK Coverity issue

[Ranbir Singh]

From: Ranbir Singh <Ranbir.Singh3@Dell.com>

The function SubmitResources has a switch-case code in which the
case ACPI_ADDRESS_SPACE_TYPE_MEM: which falls through to
case ACPI_ADDRESS_SPACE_TYPE_IO: if there is no scenario of
return EFI_INVALID_PARAMETER;

While this may be intentional, it is not evident to any general code
reader as well as any static analyzer tool. Just adding

    // No break; here as this is an intentional fallthrough.

as comment in between makes any reader as well as Coverity happy.

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4212

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
---
 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
index 519e1369f85e..3bd91e2787fd 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
@@ -1531,6 +1531,10 @@ SubmitResources (
               return EFI_INVALID_PARAMETER;
             }
 
+            //
+            // No break; here as this is an intentional fall through.
+            //
+
           case ACPI_ADDRESS_SPACE_TYPE_IO:
             //
             // Check aligment, it should be of the form 2^n-1
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107003): https://edk2.groups.io/g/devel/message/107003
Mute This Topic: https://groups.io/mt/100212117/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix issues pointed by Coverity

[Ranbir Singh]

[-- Attachment #1: Type: text/plain, Size: 809 bytes --]

Soft reminder to have a look at the series

On Mon, Jul 17, 2023 at 5:24 PM Ranbir Singh <rsingh@ventanamicro.com>
wrote:

> Ranbir Singh (2):
>   MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues
>   MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix MISSING_BREAK Coverity
>     issue
>
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> --
> 2.34.1
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109036): https://edk2.groups.io/g/devel/message/109036
Mute This Topic: https://groups.io/mt/100212111/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 1529 bytes --]