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

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

[Ranbir Singh]

v1 -> v2:
  - Rebased to latest master HEAD
  - Changed the commit message wrt MISSING_BREAK issues
  - Cc update

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 (#110799): https://edk2.groups.io/g/devel/message/110799
Mute This Topic: https://groups.io/mt/102437645/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 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: 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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Laszlo Ersek]

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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ranbir Singh]

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

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/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: 8684 bytes --]

[edk2-devel] [PATCH v2 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 why there is no break; in between. Adding

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

as comment in between makes it explicit. Incidentally, the comment
satisfies Coverity as well.

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 | 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 (#110801): https://edk2.groups.io/g/devel/message/110801
Mute This Topic: https://groups.io/mt/102437649/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Laszlo Ersek]

On 11/7/23 06:06, Ranbir Singh wrote:
> 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 why there is no break; in between. Adding
> 
>     // No break; here as this is an intentional fallthrough.
> 
> as comment in between makes it explicit. Incidentally, the comment
> satisfies Coverity as well.
> 
> 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 | 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

I agree with adding the comment (I agree it's a fallthrough), but the
semicolon is weird. I think we should say:

  No break here, as this is an intentional fall through.

Laszlo



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