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

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

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

As far as I know, from a secure coding perspective, it would be recommended
that array overrun condition check is captured in the code even if it is
felt that it will never hit.

Generally speaking, I won't be in favour of handling other ASSERT
conditions updates even if required if they are not related to array
overrun conditions i.e., the context of the patch.

If someone / PCI maintainers can advise in this patch context what should
be done in the array overrun condition, I will be happy to update,
otherwise, sorry to say I won't be able to pursue this particular one
further and hence would be leaving the related code with the status quo
here.

On Fri, Nov 10, 2023 at 2:10 AM Kinney, Michael D <
michael.d.kinney@intel.com> wrote:

> Hi Ranbir,
>
> A deadloop without even a debug print is not good behavior.
>
> If this condition really represents a condition where it is not possible
> to complete the PCI resource allocation/assignment, then an error status
> code should be returned to the caller of NotifyPhase().  Perhaps
> EFI_OUT_OF_RESOURCES.  The other ASSERT() conditions in this API should
> likely be updated to do the same.
>
> This may also require the caller of this service, the PCI Bus Driver,
> to be reviewed to make sure it handles error conditions from NotifyPhase().
>
> I recommend you get help on the proposed code changes from the PCI
> subsystem maintainers.
>
> Thanks,
>
> Mike
>
>
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ranbir
> > Singh
> > Sent: Thursday, November 9, 2023 9:39 AM
> > To: devel@edk2.groups.io; rsingh@ventanamicro.com
> > Cc: Ni, Ray <ray.ni@intel.com>; Veeresh Sangolli
> > <veeresh.sangolli@dellteam.com>
> > Subject: [edk2-devel] [PATCH v3 1/2]
> > MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues
> >
> > 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
> > 937, 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..c2c143068cd2 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) {
> >
> > +              CpuDeadLoop ();
> >
> > +            }
> >
> > +
> >
> >              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 (#110993):
> > https://edk2.groups.io/g/devel/message/110993
> > Mute This Topic: https://groups.io/mt/102490513/1643496
> > Group Owner: devel+owner@edk2.groups.io
> > Unsubscribe: https://edk2.groups.io/g/devel/unsub
> > [michael.d.kinney@intel.com]
> > -=-=-=-=-=-=
> >
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111007): https://edk2.groups.io/g/devel/message/111007
Mute This Topic: https://groups.io/mt/102490513/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: 6883 bytes --]