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

["Ranbir Singh" <rsingh@ventanamicro.com>]

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