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: 4914 bytes --]

Options before us till now -

1. Add array overrun check and Debug statement before CpuDeadLoop within
2. Status Quo (not everything can be ideal :-))

Question before us
     - Is 1 better than 2 ?


On Fri, Nov 10, 2023 at 8:41 AM Ranbir Singh <rsingh@ventanamicro.com>
wrote:

> 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 (#111009): https://edk2.groups.io/g/devel/message/111009
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: 7566 bytes --]