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 7111094144F for ; Wed, 15 Nov 2023 10:10:30 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=PsOK6WUWXEdZbwGRzwq9ngcLu8kuJoC1vWXHkV3lXPc=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version: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=1700043029; v=1; b=E8JL5KVTVpVhCx6cFtNv782jdBJJymEeg46yfxJIq/Okzh9wea3nlqbIlvQj/d+BfujpsRwB MNBW7tjUe5vKgMBYVITvVl0NufXn9bvKbzRbLy5UphDfrYbqC5F2+4boOi9Zdk7lmfk1dNz7OrD 9H0Usmrm30DXSRdejF1QsbZU= X-Received: by 127.0.0.2 with SMTP id UQ2GYY7687511xMOQr2i4E0Q; Wed, 15 Nov 2023 02:10:29 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web10.10075.1700043028374704717 for ; Wed, 15 Nov 2023 02:10:28 -0800 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-271-3-bj1v9eMBShgBiWo5QjRQ-1; Wed, 15 Nov 2023 05:10:23 -0500 X-MC-Unique: 3-bj1v9eMBShgBiWo5QjRQ-1 X-Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 412098007B3; Wed, 15 Nov 2023 10:10:23 +0000 (UTC) X-Received: from [10.39.192.211] (unknown [10.39.192.211]) by smtp.corp.redhat.com (Postfix) with ESMTPS id BE9AA2026D4C; Wed, 15 Nov 2023 10:10:21 +0000 (UTC) Message-ID: <41d5de9a-a5e5-7f9b-e088-dd09798f73a5@redhat.com> Date: Wed, 15 Nov 2023 11:10:20 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue To: Michael Kubacki , devel@edk2.groups.io, 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> From: "Laszlo Ersek" In-Reply-To: <308b4c23-d8a6-4f21-ad25-d0d4a2496518@linux.microsoft.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.4 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: 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,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: JAgQ6oCbfWgOBh9i3xTCxXdex7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=E8JL5KVT; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.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/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.  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 >> >> > 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/Include/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. Ah, great reminder that we have ANALYZER_UNREACHABLE. I've totally forgotten about that; my apologies. ... 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. So this looks very useful to me; can you upstream it? Laszlo > >> 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 PM 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 != 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 >     >>      > >> wrote: >>      > >>      >     On 11/7/23 07:19, Ranbir Singh wrote: >>      >     > From: Ranbir Singh >     > >>      >     > >>      >     > 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 >>     >>      >     >     > >>      >     > >>      >     > 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 = 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 (#111249): https://edk2.groups.io/g/devel/message/111249 Mute This Topic: https://groups.io/mt/102438320/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-