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 <ler...@redhat.com> wrote: > On 11/7/23 07:19, Ranbir Singh wrote: > > From: Ranbir Singh <ranbir.sin...@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...@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/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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
