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 99F5B74004D for ; Mon, 13 Nov 2023 11:28:14 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=XsAPycd+3eAJDTFj8sSCL7dr4Kq/88lCG/gw6b2dA2k=; 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=1699874893; v=1; b=OsCGjnjS7kkNW0wQcgWWt5R+xmP1kEREDU5jeERU7rSwqM8XDi+H4XMMH0JXWPWlJHYmpGA+ yYlvNvpZ4/s8Ismu6iBnnSnAsn9lfgUc277H6bkfIWmh6l46/KWlRtdNW14hXMkACwUs4zD+jN4 wZ2IR87+BjrSFYYV5t4d4dcI= X-Received: by 127.0.0.2 with SMTP id 8jEYYY7687511xWVrVdXRBnK; Mon, 13 Nov 2023 03:28:13 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web10.34725.1699874892741606460 for ; Mon, 13 Nov 2023 03:28:12 -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-618-eH_fzPR1PrmAY-2tkIQ2EA-1; Mon, 13 Nov 2023 06:28:08 -0500 X-MC-Unique: eH_fzPR1PrmAY-2tkIQ2EA-1 X-Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.rdu2.redhat.com [10.11.54.7]) (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 B9244185A780; Mon, 13 Nov 2023 11:28:07 +0000 (UTC) X-Received: from [10.39.192.220] (unknown [10.39.192.220]) by smtp.corp.redhat.com (Postfix) with ESMTPS id F136D1C060AE; Mon, 13 Nov 2023 11:28:06 +0000 (UTC) Message-ID: <5a7f3c56-d18f-9e4c-0e70-9113923ee36d@redhat.com> Date: Mon, 13 Nov 2023 12:28:05 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue To: Ranbir Singh Cc: devel@edk2.groups.io, Ray Ni , Veeresh Sangolli References: <20231107061959.113213-1-rsingh@ventanamicro.com> <20231107061959.113213-5-rsingh@ventanamicro.com> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.7 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: Jc64OmlbYeBT19zcinHxbpf5x7686176AA= 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=OsCGjnjS; 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/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 > > 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 (#111143): https://edk2.groups.io/g/devel/message/111143 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] -=-=-=-=-=-=-=-=-=-=-=-