From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 01D467803D9 for ; Tue, 14 Nov 2023 16:53:16 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=ilkdl93uBXvyEL812N2UwSFOWnQncONjX9b8M9uhuqE=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1699980795; v=1; b=nkNpD4X9xNo4z+8V0pCeisQl8vrkmkXoovohLc9AitMrg8YHIB3TF4naTXlW9jlQDjFeaHuc Mvc/dtm5r8dUC1HOv4T2+oCy3/7v6JV5daWWLIZ3Z/neoVhd6VLOqESXOUzEyiZbi9HchIQBeTw T4cP0SmS4ZbsK624oUPktRp4= X-Received: by 127.0.0.2 with SMTP id EzJxYY7687511xwc8Oef1Xvi; Tue, 14 Nov 2023 08:53:15 -0800 X-Received: from mail-pg1-f177.google.com (mail-pg1-f177.google.com [209.85.215.177]) by mx.groups.io with SMTP id smtpd.web10.17691.1699980794769618206 for ; Tue, 14 Nov 2023 08:53:14 -0800 X-Received: by mail-pg1-f177.google.com with SMTP id 41be03b00d2f7-5c18a3387f5so1501404a12.1 for ; Tue, 14 Nov 2023 08:53:14 -0800 (PST) X-Gm-Message-State: k2CjqcN3BjYDRfzteo5yV1fdx7686176AA= X-Google-Smtp-Source: AGHT+IFBb3V7HsMEC8MXkqq2fpRPXlO7k800BwOfrGJgTzRQJStea+MITfu18wmoC1SMpV6ix6Vwxct6pxY9gLDOZts= X-Received: by 2002:a17:90b:1646:b0:27d:1538:e324 with SMTP id il6-20020a17090b164600b0027d1538e324mr8200759pjb.32.1699980794030; Tue, 14 Nov 2023 08:53:14 -0800 (PST) MIME-Version: 1.0 References: <20231107061959.113213-1-rsingh@ventanamicro.com> <20231107061959.113213-5-rsingh@ventanamicro.com> <5a7f3c56-d18f-9e4c-0e70-9113923ee36d@redhat.com> In-Reply-To: From: "Ranbir Singh" Date: Tue, 14 Nov 2023 22:23:03 +0530 Message-ID: Subject: Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue To: "Kinney, Michael D" Cc: "devel@edk2.groups.io" , Laszlo Ersek , "Andrew Fish (afish@apple.com)" , "quic_llindhol@quicinc.com" , Michael Kubacki , "Ni, Ray" , Veeresh Sangolli Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,rsingh@ventanamicro.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: multipart/alternative; boundary="0000000000005a8487060a1f9f11" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=nkNpD4X9; dmarc=none; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io --0000000000005a8487060a1f9f11 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, Nov 14, 2023 at 9:51=E2=80=AFPM Kinney, Michael D < michael.d.kinney@intel.com> wrote: > Hi Ranbir, > > > > First I want to recognize your efforts to collect Coverity issues and > propose changes to address > > them. > Thanks Mike. > > > I still disagree with adding CpuDealLoop() for any static analysis issues= . > A bit of correction here. CpuDeadLoop() is not exactly an addition for static analysis issues. For that, the NULL pointer check and return statement in the if block are sufficient. However, CpuDeadLoop() is added as suggested by Laszlo to introduce a hang behaviour in RELEASE builds as well when the situation is anyway not safe to progress normally. That said, it may not still be required at every point and hence needs to be assessed on a case to case basis. > > There have been previous discussions about adding a PANIC() or FATAL() > macro that would > > perform platform specific actions if a condition is detected where the > boot of the platform > > can not continue. A platform get to make the choice to log or reboot or > hang, etc. Not the > > code that detected the condition. > > > > https://edk2.groups.io/g/devel/message/86597 > > > > Unfortunately, in order to fix some of these static analysis issues > correctly, we are going > > to have to identify the use of ASSERT() that really is a fatal condition > that can not continue > > and introduce an implementation approach that provides a platform handler > and > > satisfies the static analysis tools. > > > > We also have to evaluate if a return error status with a DEBUG_ERROR > message would be a better > > choice than an ASSERT() that can be filtered out by build options. > Note that the existing ASSERT will give a DEBUG message even when CpuDeadLoop is added in the code later. > > > Best regards, > > > > Mike > > > Generally speaking, there now seems to be different views coming from you and Laszlo. We might have to wait for some sort of agreement to be reached. > > > *From:* devel@edk2.groups.io * On Behalf Of *Ranbi= r > Singh > *Sent:* Tuesday, November 14, 2023 7:08 AM > *To:* Laszlo Ersek > *Cc:* devel@edk2.groups.io; Ni, Ray ; Veeresh Sangolli = < > veeresh.sangolli@dellteam.com> > *Subject:* Re: [edk2-devel] [PATCH v2 4/5] > MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue > > > > > > > > On Mon, Nov 13, 2023 at 4:58=E2=80=AFPM Laszlo Ersek = 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 !=3D NULL); > + if (RootBridge =3D=3D 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 (=3D> 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=E2=80=AFPM Laszlo Ersek > > wrote: > > > > On 11/7/23 07:19, Ranbir Singh wrote: > > > From: Ranbir Singh > > > > > > The function StartPciDevices has a check > > > > > > ASSERT (RootBridge !=3D NULL); > > > > > > but this comes into play only in DEBUG mode. In Release mode, the= re > > > 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=3D4239 > > > > > > > > Cc: Ray Ni > > > > Co-authored-by: Veeresh Sangolli > > > > > Signed-off-by: Ranbir Singh > > > Signed-off-by: Ranbir Singh > > > > > --- > > > 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 =3D GetRootBridgeByHandle (Controller); > > > - ASSERT (RootBridge !=3D NULL); > > > + if (RootBridge =3D=3D NULL) { > > > + return EFI_NOT_READY; > > > + } > > > + > > > ThisHostBridge =3D RootBridge->PciRootBridgeIo->ParentHandle; > > > > > > CurrentLink =3D 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 sugge= st > > CpuDeadLoop() instead. > > > > If you insist that CpuDeadLoop() is "too risky" here, then the patc= h > is > > acceptable, but then the StartPciDevices() call site in > > PciBusDriverBindingStart() must check the error properly: we must n= ot > > install "gEfiPciEnumerationCompleteProtocolGuid", and the function > must > > propagate the error outwards. > > > > Laszlo > > > >=20 > -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111209): https://edk2.groups.io/g/devel/message/111209 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] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- --0000000000005a8487060a1f9f11 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Tue, Nov 14, 2023 at 9:51=E2=80=AF= PM Kinney, Michael D <mich= ael.d.kinney@intel.com> wrote:

Hi Ranbir,

=C2=A0

First I want to recognize your efforts to collect Co= verity issues and propose changes to address

them.


Thanks Mike.
=C2=A0

=C2=A0

I still disagree with adding CpuDealLoop() for any s= tatic analysis issues.


A bit of correction here.=C2=A0=C2=A0
CpuDeadLoop() is not exac= tly an addition for static analysis issues. For that, the NULL pointer chec= k and return statement in the if block are sufficient.
However, C= puDeadLoop() is added as suggested by Laszlo to introduce a hang behaviour = in RELEASE builds as well when the situation is anyway not safe to progress= normally. That said, it may not still be required at every point and hence= needs to be assessed on a case to case basis.

=C2=A0

There have been previous discussions about adding a = PANIC() or FATAL() macro that would

perform platform specific actions if a condition is = detected where the boot of the platform

can not continue.=C2=A0 A platform get to make the c= hoice to log or reboot or hang, etc.=C2=A0 Not the

code that detected the condition.

=C2=A0

https://edk2.groups.io/= g/devel/message/86597

=C2=A0

Unfortunately, in order to fix some of these static = analysis issues correctly, we are going

to have to identify the use of ASSERT() that really = is a fatal condition that can not continue

and introduce an implementation approach that provid= es a platform handler and

satisfies the static analysis tools.

=C2=A0

We also have to evaluate if a return error status wi= th a DEBUG_ERROR message would be a better

choice than an ASSERT() that can be filtered out by = build options.


Note t= hat the existing ASSERT will give a DEBUG message even when CpuDeadLoop is = added in the code later.
=C2=A0

=C2=A0

Best regards,

=C2=A0

Mike

=C2=A0


Generally speaking, there now seems to be different views c= oming from you and Laszlo. We might have to wait for some sort of agreement= to be reached.
=C2=A0

=C2=A0

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ranbir Singh
Sent: Tuesday, November 14, 2023 7:08 AM
To: Laszlo Ersek <lersek@redhat.com>
Cc: devel@= edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Veeresh Sangolli <veeresh.sangolli@delltea= m.com>
Subject: Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBus= Dxe: Fix NULL_RETURNS Coverity issue

=C2=A0

=C2=A0

=C2=A0

On Mon, Nov 13, 2023 at 4:58=E2=80=AFPM Laszlo Ersek= <lersek@redhat.c= om> wrote:

On 11/10/23 07:11, Ranb= ir 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 i= n
> 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

=C2=A0

See, Does the following=C2=A0change look acceptable = to you ?

=C2=A0

=C2=A0 =C2=A0 ASSERT (RootBridge !=3D NULL);
+=C2=A0 if (RootBridge =3D=3D NULL) {

+=C2=A0 =C2=A0 CpuDeadLoop ();
+=C2=A0 =C2=A0 return EFI_NOT_READY;
+=C2=A0 }

+

=C2=A0

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

=C2=A0


>
> On Tue, Nov 7, 2023 at 10:18=E2=80=AFPM Laszlo Ersek <lersek@redhat.com
> <mailto:lers= ek@redhat.com>> wrote:
>
>=C2=A0 =C2=A0 =C2=A0On 11/7/23 07:19, Ranbir Singh wrote:
>=C2=A0 =C2=A0 =C2=A0> From: Ranbir Singh <Ranbir.Singh3@Dell.com>
>=C2=A0 =C2=A0 =C2=A0>
>=C2=A0 =C2=A0 =C2=A0> The function StartPciDevices has a check
>=C2=A0 =C2=A0 =C2=A0>
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 =C2=A0 =C2=A0ASSERT (RootBridge !=3D NUL= L);
>=C2=A0 =C2=A0 =C2=A0>
>=C2=A0 =C2=A0 =C2=A0> but this comes into play only in DEBUG mode. I= n Release mode, there
>=C2=A0 =C2=A0 =C2=A0> is no handling if the RootBridge value is NULL= and the code proceeds
>=C2=A0 =C2=A0 =C2=A0> to unconditionally dereference "RootBridg= e" which will lead to CRASH.
>=C2=A0 =C2=A0 =C2=A0>
>=C2=A0 =C2=A0 =C2=A0> Hence, for safety add NULL pointer checks alwa= ys and return
>=C2=A0 =C2=A0 =C2=A0> EFI_NOT_READY if RootBridge value is NULL whic= h is one of the return
>=C2=A0 =C2=A0 =C2=A0> values as mentioned in the function descriptio= n header.
>=C2=A0 =C2=A0 =C2=A0>
>=C2=A0 =C2=A0 =C2=A0> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D4239
>=C2=A0 =C2=A0 =C2=A0<https://bugzilla.tianocore.org/show_bu= g.cgi?id=3D4239>
>=C2=A0 =C2=A0 =C2=A0>
>=C2=A0 =C2=A0 =C2=A0> Cc: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com>>
>=C2=A0 =C2=A0 =C2=A0> Co-authored-by: Veeresh Sangolli <veeresh.sangoll= i@dellteam.com
>=C2=A0 =C2=A0 =C2=A0<mailto:veeresh.sangolli@dellteam.com>>
>=C2=A0 =C2=A0 =C2=A0> Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>= ;
>=C2=A0 =C2=A0 =C2=A0> Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com<= br> >=C2=A0 =C2=A0 =C2=A0<mailto:rsingh@ventanamicro.com>>
>=C2=A0 =C2=A0 =C2=A0> ---
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 MdeModulePkg/Bus/Pci/PciBusDxe/PciDevice= Support.c | 5 ++++-
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 1 file changed, 4 insertions(+), 1 delet= ion(-)
>=C2=A0 =C2=A0 =C2=A0>
>=C2=A0 =C2=A0 =C2=A0> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/Pc= iDeviceSupport.c
>=C2=A0 =C2=A0 =C2=A0b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c=
>=C2=A0 =C2=A0 =C2=A0> index 581e9075ad41..3de80d98370e 100644
>=C2=A0 =C2=A0 =C2=A0> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDevice= Support.c
>=C2=A0 =C2=A0 =C2=A0> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDevice= Support.c
>=C2=A0 =C2=A0 =C2=A0> @@ -772,7 +772,10 @@ StartPciDevices (
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 =C2=A0 LIST_ENTRY=C2=A0 =C2=A0 =C2=A0*Cu= rrentLink;
>=C2=A0 =C2=A0 =C2=A0>=C2=A0
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 =C2=A0 RootBridge =3D GetRootBridgeByHan= dle (Controller);
>=C2=A0 =C2=A0 =C2=A0> -=C2=A0 ASSERT (RootBridge !=3D NULL);
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 if (RootBridge =3D=3D NULL) {
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 return EFI_NOT_READY;
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 }
>=C2=A0 =C2=A0 =C2=A0> +
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 =C2=A0 ThisHostBridge =3D RootBridge->= ;PciRootBridgeIo->ParentHandle;
>=C2=A0 =C2=A0 =C2=A0>=C2=A0
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 =C2=A0 CurrentLink =3D mPciDevicePool.Fo= rwardLink;
>
>=C2=A0 =C2=A0 =C2=A0I don't think this is a good fix.
>
>=C2=A0 =C2=A0 =C2=A0There is one call site, namely in PciBusDriverBindi= ngStart(). That call
>=C2=A0 =C2=A0 =C2=A0site does not check the return value. (Of course /s= )
>
>=C2=A0 =C2=A0 =C2=A0I think that this ASSERT() can indeed never fail. T= herefore I suggest
>=C2=A0 =C2=A0 =C2=A0CpuDeadLoop() instead.
>
>=C2=A0 =C2=A0 =C2=A0If you insist that CpuDeadLoop() is "too risky= " here, then the patch is
>=C2=A0 =C2=A0 =C2=A0acceptable, but then the StartPciDevices() call sit= e in
>=C2=A0 =C2=A0 =C2=A0PciBusDriverBindingStart() must check the error pro= perly: we must not
>=C2=A0 =C2=A0 =C2=A0install "gEfiPciEnumerationCompleteProtocolGui= d", and the function must
>=C2=A0 =C2=A0 =C2=A0propagate the error outwards.
>
>=C2=A0 =C2=A0 =C2=A0Laszlo
>

_._,_._,_
Groups.io Links:

=20 You receive all messages sent to this group. =20 =20

View/Reply Online (#111209) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--0000000000005a8487060a1f9f11--