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

["Michael D Kinney" <michael.d.kinney@intel.com>]

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