Thanks for the detailed analysis Laszlo. I don't deny that there cannot be false positives by the tool.
For now, I can move forward with your proposal of replacing continue with CpuDeadLoop and will post v3. On Tue, Nov 7, 2023 at 9:17 PM Laszlo Ersek <ler...@redhat.com> wrote: > Hi Ranbir, > > On 11/7/23 06:06, Ranbir Singh wrote: > > From: Ranbir Singh <ranbir.sin...@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...@intel.com> > > Co-authored-by: Veeresh Sangolli <veeresh.sango...@dellteam.com> > > Signed-off-by: Ranbir Singh <ranbir.sin...@dell.com> > > Signed-off-by: Ranbir Singh <rsi...@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 (#110897): https://edk2.groups.io/g/devel/message/110897 Mute This Topic: https://groups.io/mt/102437647/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
