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: 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 (#110800): https://edk2.groups.io/g/devel/message/110800
Mute This Topic: https://groups.io/mt/102437647/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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 <lersek@redhat.com> wrote:
> 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 (#110897): https://edk2.groups.io/g/devel/message/110897
Mute This Topic: https://groups.io/mt/102437647/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.