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 8DE89740039 for ; Tue, 14 Nov 2023 19:37:09 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=+AdLyffzqLnh1wPiRywE/VIJEdSX8quXZ/hd11p/+Ds=; c=relaxed/simple; d=groups.io; h=DKIM-Filter:Message-ID:Date:MIME-Version:User-Agent:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1699990628; v=1; b=atwNNnVK3DLiXwWPMIZjxJXKNvinW+MscvRNby58gxqtFTOnVqSA6AHa9Bdxhtu3HishZ5FI 6kAVekW0fzWORnkOqs6NEk/l5JlC0sVWLV9Yfi/nMfrjSa7F3rlRCLhHhl3o0Cjsv6vVDO3aigL NRwnqD1QPKadwBZYhTQdnhWY= X-Received: by 127.0.0.2 with SMTP id knR1YY7687511xmbyfOlNeJ2; Tue, 14 Nov 2023 11:37:08 -0800 X-Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web11.2324.1699990627442546968 for ; Tue, 14 Nov 2023 11:37:07 -0800 X-Received: from [192.168.4.22] (unknown [47.201.241.95]) by linux.microsoft.com (Postfix) with ESMTPSA id E457A20B74C1; Tue, 14 Nov 2023 11:37:05 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com E457A20B74C1 Message-ID: <308b4c23-d8a6-4f21-ad25-d0d4a2496518@linux.microsoft.com> Date: Tue, 14 Nov 2023 14:37:04 -0500 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue To: devel@edk2.groups.io, michael.d.kinney@intel.com, "rsingh@ventanamicro.com" , Laszlo Ersek , "Andrew Fish (afish@apple.com)" , "quic_llindhol@quicinc.com" Cc: "Ni, Ray" , Veeresh Sangolli References: <20231107061959.113213-1-rsingh@ventanamicro.com> <20231107061959.113213-5-rsingh@ventanamicro.com> <5a7f3c56-d18f-9e4c-0e70-9113923ee36d@redhat.com> From: "Michael Kubacki" In-Reply-To: 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,mikuback@linux.microsoft.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: DD7GooBSwohXXdx99S6Z7ckTx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=atwNNnVK; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=linux.microsoft.com (policy=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 On 11/14/2023 11:21 AM, Michael D Kinney wrote: > Hi Ranbir, >=20 > First I want to recognize your efforts to collect Coverity issues and=20 > propose changes to address >=20 > them. >=20 > I still disagree with adding CpuDealLoop() for any static analysis issues= . >=20 > There have been previous discussions about adding a PANIC() or FATAL()=20 > macro that would >=20 > perform platform specific actions if a condition is detected where the=20 > boot of the platform >=20 > can not continue.=C2=A0 A platform get to make the choice to log or reboo= t or=20 > hang, etc.=C2=A0 Not the >=20 > code that detected the condition. >=20 > https://edk2.groups.io/g/devel/message/86597=20 > >=20 After going through hundreds of edk2 static analysis findings, we found=20 a small number of cases where an interface such as PanicLib was useful=20 and recently added an implementation=20 https://github.com/microsoft/mu_basecore/blob/release/202302/MdePkg/Include= /Library/PanicLib.h. For example, the return value from calls to MpInitLibWhoAmI() in=20 exception related code often goes unchecked and it's been used there.=20 Being able to separate the library instance implementation linked to a=20 given module from a more broad library class like DebugLib for these=20 cases has also been helpful. > Unfortunately, in order to fix some of these static analysis issues=20 > correctly, we are going >=20 > to have to identify the use of ASSERT() that really is a fatal condition= =20 > that can not continue >=20 > and introduce an implementation approach that provides a platform=20 > handler and >=20 > satisfies the static analysis tools. >=20 > We also have to evaluate if a return error status with a DEBUG_ERROR=20 > message would be a better >=20 > choice than an ASSERT() that can be filtered out by build options. >=20 > Best regards, >=20 > Mike >=20 > *From:* devel@edk2.groups.io *On Behalf Of=20 > *Ranbir Singh > *Sent:* Tuesday, November 14, 2023 7:08 AM > *To:* Laszlo Ersek > *Cc:* devel@edk2.groups.io; Ni, Ray ; Veeresh Sangolli= =20 > > *Subject:* Re: [edk2-devel] [PATCH v2 4/5]=20 > MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue >=20 > On Mon, Nov 13, 2023 at 4:58=E2=80=AFPM Laszlo Ersek > wrote: >=20 > 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(). >=20 > 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. >=20 > I think two options are realistic: >=20 > - replace the assert() with a CpuDeadLoop() -- this is my preference >=20 > - keep the assert, add the "if", and in the caller, handle the > EFI_NOT_READY error. This is workable too. (Keeping the assert is goo= g > because it shows that we don't expect the "if" to fire.) >=20 > 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 ASSER= T() > is one of those few justified ones in edk2 core that can indeed never > fail, IMO. >=20 > Laszlo >=20 > See, Does the following=C2=A0change look acceptable to you ? >=20 > =C2=A0 =C2=A0 ASSERT (RootBridge !=3D NULL); > +=C2=A0 if (RootBridge =3D=3D NULL) { >=20 > +=C2=A0 =C2=A0 CpuDeadLoop (); > +=C2=A0 =C2=A0 return EFI_NOT_READY; > +=C2=A0 } >=20 > + >=20 > which retains the existing assert, adds the NULL pointer check and=20 > includes CpuDeadLoop () within the if block. As a result of CpuDeadLoop= =20 > (), the return statement afterwards will never reach execution (=3D> no= =20 > need to handle this return value at the call sites), but will satisfy=20 > static analysis tools as the "RootBridge" dereference scenario doesn't=20 > arise thereafter. >=20 >=20 > > > > On Tue, Nov 7, 2023 at 10:18=E2=80=AFPM Laszlo Ersek > > >> wrote: > > > >=C2=A0 =C2=A0 =C2=A0On 11/7/23 07:19, Ranbir Singh wrote: > >=C2=A0 =C2=A0 =C2=A0> From: Ranbir Singh > > >=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 NU= LL); > >=C2=A0 =C2=A0 =C2=A0> > >=C2=A0 =C2=A0 =C2=A0> but this comes into play only in DEBUG mode. = In Release > mode, there > >=C2=A0 =C2=A0 =C2=A0> is no handling if the RootBridge value is NUL= L and the code > proceeds > >=C2=A0 =C2=A0 =C2=A0> to unconditionally dereference "RootBridge" w= hich will lead > to CRASH. > >=C2=A0 =C2=A0 =C2=A0> > >=C2=A0 =C2=A0 =C2=A0> Hence, for safety add NULL pointer checks alw= ays and return > >=C2=A0 =C2=A0 =C2=A0> EFI_NOT_READY if RootBridge value is NULL whi= ch is one of > the return > >=C2=A0 =C2=A0 =C2=A0> values as mentioned in the function descripti= on 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 > > >=C2=A0 =C2=A0 =C2=A0> > >=C2=A0 =C2=A0 =C2=A0> Cc: Ray Ni > >> > >=C2=A0 =C2=A0 =C2=A0> Co-authored-by: Veeresh Sangolli > > >=C2=A0 =C2=A0 =C2=A0 >> > >=C2=A0 =C2=A0 =C2=A0> Signed-off-by: Ranbir Singh > > >=C2=A0 =C2=A0 =C2=A0> Signed-off-by: Ranbir Singh > >=C2=A0 =C2=A0 =C2=A0 >> > >=C2=A0 =C2=A0 =C2=A0> --- > >=C2=A0 =C2=A0 =C2=A0>=C2=A0 MdeModulePkg/Bus/Pci/PciBusDxe/PciDevic= eSupport.c | 5 ++++- > >=C2=A0 =C2=A0 =C2=A0>=C2=A0 1 file changed, 4 insertions(+), 1 dele= tion(-) > >=C2=A0 =C2=A0 =C2=A0> > >=C2=A0 =C2=A0 =C2=A0> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/P= ciDeviceSupport.c > >=C2=A0 =C2=A0 =C2=A0b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSuppo= rt.c > >=C2=A0 =C2=A0 =C2=A0> index 581e9075ad41..3de80d98370e 100644 > >=C2=A0 =C2=A0 =C2=A0> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDevic= eSupport.c > >=C2=A0 =C2=A0 =C2=A0> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDevic= eSupport.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*C= urrentLink; > >=C2=A0 =C2=A0 =C2=A0> > >=C2=A0 =C2=A0 =C2=A0>=C2=A0 =C2=A0 RootBridge =3D GetRootBridgeByHa= ndle (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->P= ciRootBridgeIo->ParentHandle; > >=C2=A0 =C2=A0 =C2=A0> > >=C2=A0 =C2=A0 =C2=A0>=C2=A0 =C2=A0 CurrentLink =3D mPciDevicePool.F= orwardLink; > > > >=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 PciBusDriverB= indingStart(). > That call > >=C2=A0 =C2=A0 =C2=A0site does not check the return value. (Of cours= e /s) > > > >=C2=A0 =C2=A0 =C2=A0I think that this ASSERT() can indeed never fai= l. Therefore 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= site in > >=C2=A0 =C2=A0 =C2=A0PciBusDriverBindingStart() must check the error= properly: we > must not > >=C2=A0 =C2=A0 =C2=A0install "gEfiPciEnumerationCompleteProtocolGuid= ", and the > function must > >=C2=A0 =C2=A0 =C2=A0propagate the error outwards. > > > >=C2=A0 =C2=A0 =C2=A0Laszlo > > >=20 >=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 (#111214): https://edk2.groups.io/g/devel/message/111214 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-