On Mon, Nov 13, 2023 at 4:58 PM Laszlo Ersek <lersek@redhat.com> wrote:

> On 11/10/23 07:11, Ranbir Singh wrote:
> > 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().
>
> I don't think this makes much sense, given that execution is not
> supposed to continue past CpuDeadLoop(), so the new error check would
> not be reached.
>
> I think two options are realistic:
>
> - replace the assert() with a CpuDeadLoop() -- this is my preference
>
> - keep the assert, add the "if", and in the caller, handle the
> EFI_NOT_READY error. This is workable too. (Keeping the assert is goog
> because it shows that we don't expect the "if" to fire.)
>
> Anyway, now that we have no way to verify the change against Coverity, I
> don't know if this patch is worth the churn. :( As I said, this ASSERT()
> is one of those few justified ones in edk2 core that can indeed never
> fail, IMO.
>
> Laszlo
>
>
See, Does the following change look acceptable to you ?

    ASSERT (RootBridge != NULL);
+  if (RootBridge == NULL) {
+    CpuDeadLoop ();
+    return EFI_NOT_READY;
+  }
+

which retains the existing assert, adds the NULL pointer check and includes
CpuDeadLoop () within the if block. As a result of CpuDeadLoop (), the
return statement afterwards will never reach execution (=> no need to
handle this return value at the call sites), but will satisfy static
analysis tools as the "RootBridge" dereference scenario doesn't arise
thereafter.


> >
> > On Tue, Nov 7, 2023 at 10:18 PM Laszlo Ersek <lersek@redhat.com
> > <mailto: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
> >     <https://bugzilla.tianocore.org/show_bug.cgi?id=4239>
> >     >
> >     > Cc: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com>>
> >     > Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com
> >     <mailto:veeresh.sangolli@dellteam.com>>
> >     > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> >     > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com
> >     <mailto: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 (#111202): https://edk2.groups.io/g/devel/message/111202
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]
-=-=-=-=-=-=-=-=-=-=-=-