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.kin...@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; rsi...@ventanamicro.com > > Cc: Ni, Ray <ray...@intel.com>; Veeresh Sangolli > > <veeresh.sango...@dellteam.com> > > Subject: [edk2-devel] [PATCH v3 1/2] > > MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues > > > > 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 > > 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...@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..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+ow...@edk2.groups.io > > Unsubscribe: https://edk2.groups.io/g/devel/unsub > > [michael.d.kin...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
