On Mon, Nov 13, 2023 at 4:58 PM Laszlo Ersek <[email protected]> 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 <[email protected]
> > <mailto:[email protected]>> wrote:
> >
> > On 11/7/23 07:19, Ranbir Singh wrote:
> > > From: Ranbir Singh <[email protected]>
> > >
> > > 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 <[email protected] <mailto:[email protected]>>
> > > Co-authored-by: Veeresh Sangolli <[email protected]
> > <mailto:[email protected]>>
> > > Signed-off-by: Ranbir Singh <[email protected]>
> > > Signed-off-by: Ranbir Singh <[email protected]
> > <mailto:[email protected]>>
> > > ---
> > > 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/21656
Group Owner: [email protected]
Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-
