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

["Laszlo Ersek" <lersek@redhat.com>]

Hi Ranbir,

On 11/7/23 06:06, Ranbir Singh wrote:
> 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

137 should be 937

> 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: 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);

The ASSERT() will never fire. But I agree that it is hard to see.

I propose that we should add

  if (Index == TypeMax) {
    CpuDeadLoop ();
  }

instead of "continue".

Here's why the ASSERT() will never fire.

- The outer loop (using Index1) will run five times exactly.

- In each execution of the outer loop, we have two branches. Each branch flips *at most* one element in ResNodeHandled from FALSE to TRUE.

While each branch writes to exactly one ResNodeHandled element (storing TRUE), the original ResNodeHandled value may be FALSE, or may be TRUE. (TRUE as original value is not easy to see, but consider that the first branch of the outer loop body may notice ResNone for a particular resource type *after* the second branch of the outer loop body has assigned a resource to that type. That *is* a bug, but a *different* one!)

The point is that the FALSE->TRUE *transition* may happen for at most one resource type per outer loop iteration. This means that in the Nth iteration of the outer loop (Index1=0, 1, ... 4 inclusive), there are initially *at least* (5 - Index1) FALSE elements in ResNodeHandled. In the last iteration of the outer loop (Index1=4), there is at least 5 - 4 = 1 FALSE element in ResNodeHandled.

- This means that the Index2-based inner loop will *always find* an Index2 where ResNodeHandled is FALSE.

- For the first such Index2 in the inner loop body, Index will be assigned, because MaxAlignment starts with 0, and the Alignment field has type UINT64.

Therefore the ASSERT will never fire -- it is a correct assertion.

Basically the assert states that we have a resource type to assign at that point -- and that claim is correct. So, unfortunately, Coverity is wrong here. We should add a CpuDeadLoop() therefore, to tell coverity that we're willing to hang there even in RELEASE builds.

"continue" is not useful in any case, because if there are no more resource types to assign, then continuing the outer loop makes no sense. That is, "break" would make more sense. (But again, that too would never be reached.)

... For making the code easier to understand, I'd perhaps propose (this is displayed with "git diff -b"):

diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
index d573e532bac8..87c85e9df771 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
@@ -833,11 +833,12 @@ NotifyPhase (
   EFI_STATUS                Status;
   EFI_STATUS                ReturnStatus;
   PCI_RESOURCE_TYPE         Index;
-  PCI_RESOURCE_TYPE         Index1;
   PCI_RESOURCE_TYPE         Index2;
   BOOLEAN                   ResNodeHandled[TypeMax];
   UINT64                    MaxAlignment;
   UINT64                    Translation;
+  UINTN                     ToAssign;
+  UINTN                     Assigned;
 
   HostBridge = PCI_HOST_BRIDGE_FROM_THIS (This);
 
@@ -911,17 +912,20 @@ NotifyPhase (
            ; Link = GetNextNode (&HostBridge->RootBridges, Link)
            )
       {
+        ToAssign = 0;
         for (Index = TypeIo; Index < TypeBus; Index++) {
+          if (RootBridge->ResAllocNode[Index].Status == ResNone) {
+            ResNodeHandled[Index] = TRUE;
+          } else {
             ResNodeHandled[Index] = FALSE;
+            ToAssign++;
+          }
         }
 
         RootBridge = ROOT_BRIDGE_FROM_LINK (Link);
         DEBUG ((DEBUG_INFO, " RootBridge: %s\n", RootBridge->DevicePathStr));
 
-        for (Index1 = TypeIo; Index1 < TypeBus; Index1++) {
-          if (RootBridge->ResAllocNode[Index1].Status == ResNone) {
-            ResNodeHandled[Index1] = TRUE;
-          } else {
+        for (Assigned = 0; Assigned < ToAssign; Assigned++) {
           //
           // Allocate the resource node with max alignment at first
           //
@@ -1091,7 +1095,6 @@ NotifyPhase (
           }
         }
       }
-      }
 
       if (ReturnStatus == EFI_OUT_OF_RESOURCES) {
         ResourceConflict (HostBridge);

Thanks
Laszlo



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