Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue

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

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

EFI_NOT_READY was listed as one of the error return values in the function
header of StartPciDevices(). So, I considered it here.

Maybe we can go by a dual approach, including CpuDeadLoop() in
StartPciDevices() as well as add return value check at the call site in
PciBusDriverBindingStart().

On Tue, Nov 7, 2023 at 10:18 PM Laszlo Ersek <lersek@redhat.com> wrote:

> On 11/7/23 07:19, Ranbir Singh wrote:
> > From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> >
> > The function StartPciDevices has a check
> >
> >     ASSERT (RootBridge != NULL);
> >
> > but this comes into play only in DEBUG mode. In Release mode, there
> > is no handling if the RootBridge value is NULL and the code proceeds
> > to unconditionally dereference "RootBridge" which will lead to CRASH.
> >
> > Hence, for safety add NULL pointer checks always and return
> > EFI_NOT_READY if RootBridge value is NULL which is one of the return
> > values as mentioned in the function description header.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239
> >
> > 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/PciBusDxe/PciDeviceSupport.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> > index 581e9075ad41..3de80d98370e 100644
> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> > @@ -772,7 +772,10 @@ StartPciDevices (
> >    LIST_ENTRY     *CurrentLink;
> >
> >    RootBridge = GetRootBridgeByHandle (Controller);
> > -  ASSERT (RootBridge != NULL);
> > +  if (RootBridge == NULL) {
> > +    return EFI_NOT_READY;
> > +  }
> > +
> >    ThisHostBridge = RootBridge->PciRootBridgeIo->ParentHandle;
> >
> >    CurrentLink = mPciDevicePool.ForwardLink;
>
> I don't think this is a good fix.
>
> There is one call site, namely in PciBusDriverBindingStart(). That call
> site does not check the return value. (Of course /s)
>
> I think that this ASSERT() can indeed never fail. Therefore I suggest
> CpuDeadLoop() instead.
>
> If you insist that CpuDeadLoop() is "too risky" here, then the patch is
> acceptable, but then the StartPciDevices() call site in
> PciBusDriverBindingStart() must check the error properly: we must not
> install "gEfiPciEnumerationCompleteProtocolGuid", and the function must
> propagate the error outwards.
>
> Laszlo
>
>


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