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 65F457803CD for ; Mon, 20 Nov 2023 14:06:03 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=+2w1NS/ZUZ6zSmRyTd3p3lTfG4/74d6HeHtJF9qxdLo=; 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=1700489162; v=1; b=ZYgOilmVVdkpQLZMZNDJxNwTRbfadMoj5FF+ZM8z4QkYalPF3RFjT1pEHvTwoDCXgBbyZvBs z7jIiaULCu94YRHliW0lUmBiMVSnwCudAIa7i3WkfTy6vXVRzsrTKzh2YqjnHry4ZtJPfp/MPIV IfUt3lqbbT+2KWambeBo1fug= X-Received: by 127.0.0.2 with SMTP id l1koYY7687511xGYvCJ2WN0V; Mon, 20 Nov 2023 06:06:02 -0800 X-Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web11.50979.1700489161496466735 for ; Mon, 20 Nov 2023 06:06:01 -0800 X-Received: from [192.168.4.22] (unknown [47.201.241.198]) by linux.microsoft.com (Postfix) with ESMTPSA id 22D6220B74C0; Mon, 20 Nov 2023 06:06:00 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 22D6220B74C0 Message-ID: <94443346-7305-4e5d-b1bd-d4274b764121@linux.microsoft.com> Date: Mon, 20 Nov 2023 09:05:58 -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, lersek@redhat.com, michael.d.kinney@intel.com, "rsingh@ventanamicro.com" , "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> <308b4c23-d8a6-4f21-ad25-d0d4a2496518@linux.microsoft.com> <41d5de9a-a5e5-7f9b-e088-dd09798f73a5@redhat.com> From: "Michael Kubacki" In-Reply-To: <41d5de9a-a5e5-7f9b-e088-dd09798f73a5@redhat.com> 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: I6ZpWpkKfdS2PXeRP17Sa2zix7686176AA= 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=ZYgOilmV; 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/15/2023 5:10 AM, Laszlo Ersek wrote: > On 11/14/23 20:37, Michael Kubacki wrote: >> On 11/14/2023 11:21 AM, Michael D Kinney wrote: >>> Hi Ranbir, >>> >>> First I want to recognize your efforts to collect Coverity issues and >>> propose changes to address >>> >>> them. >>> >>> I still disagree with adding CpuDealLoop() for any static analysis >>> issues. >>> >>> 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 choice to log or reb= oot >>> or hang, etc.=C2=A0 Not the >>> >>> code that detected the condition. >>> >>> https://edk2.groups.io/g/devel/message/86597 >>> >>> >> After going through hundreds of edk2 static analysis findings, we found >> a small number of cases where an interface such as PanicLib was useful >> and recently added an implementation >> https://github.com/microsoft/mu_basecore/blob/release/202302/MdePkg/Incl= ude/Library/PanicLib.h. >> >> For example, the return value from calls to MpInitLibWhoAmI() in >> exception related code often goes unchecked and it's been used there. >> Being able to separate the library instance implementation linked to a >> given module from a more broad library class like DebugLib for these >> cases has also been helpful. >=20 > Ah, great reminder that we have ANALYZER_UNREACHABLE. I've totally > forgotten about that; my apologies. >=20 > ... I initially thought that a plain "CONST CHAR8 *Description" was not > too flexible, but on second thought, it should be exactly right. Reason > being, it's very easy to print. Format specifiers and variable arguments > (PrintLib style) may be too complex to implement safely within > PanicReport(). Arguably, no PanicReport() implementation should be > obligated (by the interface) to depend on, or to reimplement, PrintLib. > If the calling context permits, the caller can just use PrintLib to > format the message to a local buffer (on the stack), and pass that to > PanicReport. >=20 > So this looks very useful to me; can you upstream it? >=20 Thanks for taking a look. We definitely want to align with edk2 on a=20 consistent way to handle these scenarios. We'll send a patch for further=20 discussion and review. We've waited to do so to allow the original author (Ken Lautner) to=20 return from his time out of office to send that patch. - Michael > Laszlo >=20 >=20 >> >>> 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. >>> >>> Best regards, >>> >>> Mike >>> >>> *From:* devel@edk2.groups.io *On Behalf Of >>> *Ranbir Singh >>> *Sent:* Tuesday, November 14, 2023 7:08 AM >>> *To:* Laszlo Ersek >>> *Cc:* devel@edk2.groups.io; Ni, Ray ; Veeresh >>> Sangolli >>> *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: >>> >>> =C2=A0=C2=A0=C2=A0 On 11/10/23 07:11, Ranbir Singh wrote: >>> =C2=A0=C2=A0=C2=A0=C2=A0 > EFI_NOT_READY was listed as one of the erro= r return values in the >>> =C2=A0=C2=A0=C2=A0=C2=A0 > function header of StartPciDevices(). So, I= considered it here. >>> =C2=A0=C2=A0=C2=A0=C2=A0 > >>> =C2=A0=C2=A0=C2=A0=C2=A0 > Maybe we can go by a dual approach, includi= ng CpuDeadLoop() in >>> =C2=A0=C2=A0=C2=A0=C2=A0 > StartPciDevices() as well as add return val= ue check at the call >>> =C2=A0=C2=A0=C2=A0 site in >>> =C2=A0=C2=A0=C2=A0=C2=A0 > PciBusDriverBindingStart(). >>> >>> =C2=A0=C2=A0=C2=A0 I don't think this makes much sense, given that exe= cution is not >>> =C2=A0=C2=A0=C2=A0 supposed to continue past CpuDeadLoop(), so the new= error check would >>> =C2=A0=C2=A0=C2=A0 not be reached. >>> >>> =C2=A0=C2=A0=C2=A0 I think two options are realistic: >>> >>> =C2=A0=C2=A0=C2=A0 - replace the assert() with a CpuDeadLoop() -- this= is my preference >>> >>> =C2=A0=C2=A0=C2=A0 - keep the assert, add the "if", and in the caller,= handle the >>> =C2=A0=C2=A0=C2=A0 EFI_NOT_READY error. This is workable too. (Keeping= the assert is >>> goog >>> =C2=A0=C2=A0=C2=A0 because it shows that we don't expect the "if" to f= ire.) >>> >>> =C2=A0=C2=A0=C2=A0 Anyway, now that we have no way to verify the chang= e against >>> Coverity, I >>> =C2=A0=C2=A0=C2=A0 don't know if this patch is worth the churn. :( As = I said, this >>> ASSERT() >>> =C2=A0=C2=A0=C2=A0 is one of those few justified ones in edk2 core tha= t can indeed never >>> =C2=A0=C2=A0=C2=A0 fail, IMO. >>> >>> =C2=A0=C2=A0=C2=A0 Laszlo >>> >>> 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 } >>> >>> + >>> >>> 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. >>> >>> >>> =C2=A0=C2=A0=C2=A0=C2=A0 > >>> =C2=A0=C2=A0=C2=A0=C2=A0 > On Tue, Nov 7, 2023 at 10:18=E2=80=AFPM Las= zlo Ersek >> =C2=A0=C2=A0=C2=A0 >>> =C2=A0=C2=A0=C2=A0=C2=A0 > >> wrote: >>> =C2=A0=C2=A0=C2=A0=C2=A0 > >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0On 11/7/23 07:19, Ranbir= Singh wrote: >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0> From: Ranbir Singh >> =C2=A0=C2=A0=C2=A0 > >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0> >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0> The function StartPciD= evices has a check >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0> >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0>=C2=A0 =C2=A0 =C2=A0ASS= ERT (RootBridge !=3D NULL); >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0> >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0> but this comes into pl= ay only in DEBUG mode. In Release >>> =C2=A0=C2=A0=C2=A0 mode, there >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0> is no handling if the = RootBridge value is NULL and the code >>> =C2=A0=C2=A0=C2=A0 proceeds >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0> to unconditionally der= eference "RootBridge" which will lead >>> =C2=A0=C2=A0=C2=A0 to CRASH. >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0> >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0> Hence, for safety add = NULL pointer checks always and return >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0> EFI_NOT_READY if RootB= ridge value is NULL which is one of >>> =C2=A0=C2=A0=C2=A0 the return >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0> values as mentioned in= the function description header. >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0> >>> =C2=A0=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 =C2=A0>> =C2=A0=C2=A0=C2=A0 > >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =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 >= > >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0> Co-authored-by: Veeres= h Sangolli >>> =C2=A0=C2=A0=C2=A0 >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0>> =C2=A0=C2=A0=C2=A0 >> >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=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> 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>> =C2=A0=C2=A0=C2=A0 >> >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0> --- >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0>=C2=A0 MdeModulePkg/Bus= /Pci/PciBusDxe/PciDeviceSupport.c | 5 ++++- >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0>=C2=A0 1 file changed, = 4 insertions(+), 1 deletion(-) >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0> >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0> diff --git >>> a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0b/MdeModulePkg/Bus/Pci/P= ciBusDxe/PciDeviceSupport.c >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0> index 581e9075ad41..3d= e80d98370e 100644 >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0> --- a/MdeModulePkg/Bus= /Pci/PciBusDxe/PciDeviceSupport.c >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0> +++ b/MdeModulePkg/Bus= /Pci/PciBusDxe/PciDeviceSupport.c >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0> @@ -772,7 +772,10 @@ S= tartPciDevices ( >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0>=C2=A0 =C2=A0 LIST_ENTR= Y=C2=A0 =C2=A0 =C2=A0*CurrentLink; >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0> >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0>=C2=A0 =C2=A0 RootBridg= e =3D GetRootBridgeByHandle (Controller); >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0> -=C2=A0 ASSERT (RootBr= idge !=3D NULL); >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0> +=C2=A0 if (RootBridge= =3D=3D NULL) { >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=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 >=C2=A0 =C2=A0 =C2=A0> + >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0>=C2=A0 =C2=A0 ThisHostB= ridge =3D >>> RootBridge->PciRootBridgeIo->ParentHandle; >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0> >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0>=C2=A0 =C2=A0 CurrentLi= nk =3D mPciDevicePool.ForwardLink; >>> =C2=A0=C2=A0=C2=A0=C2=A0 > >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0I don't think this is a = good fix. >>> =C2=A0=C2=A0=C2=A0=C2=A0 > >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0There is one call site, = namely in PciBusDriverBindingStart(). >>> =C2=A0=C2=A0=C2=A0 That call >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0site does not check the = return value. (Of course /s) >>> =C2=A0=C2=A0=C2=A0=C2=A0 > >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0I think that this ASSERT= () can indeed never fail. Therefore I >>> =C2=A0=C2=A0=C2=A0 suggest >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0CpuDeadLoop() instead. >>> =C2=A0=C2=A0=C2=A0=C2=A0 > >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0If you insist that CpuDe= adLoop() is "too risky" here, then >>> =C2=A0=C2=A0=C2=A0 the patch is >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0acceptable, but then the= StartPciDevices() call site in >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0PciBusDriverBindingStart= () must check the error properly: we >>> =C2=A0=C2=A0=C2=A0 must not >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0install "gEfiPciEnumerat= ionCompleteProtocolGuid", and the >>> =C2=A0=C2=A0=C2=A0 function must >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0propagate the error outw= ards. >>> =C2=A0=C2=A0=C2=A0=C2=A0 > >>> =C2=A0=C2=A0=C2=A0=C2=A0 >=C2=A0 =C2=A0 =C2=A0Laszlo >>> =C2=A0=C2=A0=C2=A0=C2=A0 > >>> >>> >> >=20 >=20 >=20 >=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 (#111482): https://edk2.groups.io/g/devel/message/111482 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-