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 24A98941B34 for ; Mon, 13 Nov 2023 15:48:14 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=dOKte7SLBUFCLr6olOJ0odGhqUaNbGGWqrl/Msy98xI=; 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=1699890493; v=1; b=bRd9RN1fTu6fw4inENpxpLJnYBb6QX35UVQS7INJeuErZsKMxiEsKOzYMQYaKA+o660aokKc fnDLDtLjNJX+aBDiQh8B9U4NdOp5MCkPnIUFAlJBCmKuQ8zfyJ36e+i9iI3phTf3pUXZe8X1xjZ OqcF9tmiWJ59FAZXIjaFms7M= X-Received: by 127.0.0.2 with SMTP id 0ydGYY7687511xSPxnJVabhr; Mon, 13 Nov 2023 07:48: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.web11.40212.1699890493170416926 for ; Mon, 13 Nov 2023 07:48:13 -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-76-QZlYqz_wP2-kP0LkT6xqiw-1; Mon, 13 Nov 2023 10:48:09 -0500 X-MC-Unique: QZlYqz_wP2-kP0LkT6xqiw-1 X-Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) (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 B83B484AEE2; Mon, 13 Nov 2023 15:48:08 +0000 (UTC) X-Received: from [10.39.192.220] (unknown [10.39.192.220]) by smtp.corp.redhat.com (Postfix) with ESMTPS id A9DF7143; Mon, 13 Nov 2023 15:48:07 +0000 (UTC) Message-ID: <817ccb26-6197-ecda-29cb-246bd9ecbbe0@redhat.com> Date: Mon, 13 Nov 2023 16:48:03 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v3 1/2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues To: devel@edk2.groups.io, michael.d.kinney@intel.com, "rsingh@ventanamicro.com" Cc: "Ni, Ray" , Veeresh Sangolli References: <20231109173908.364630-1-rsingh@ventanamicro.com> <20231109173908.364630-2-rsingh@ventanamicro.com> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.1 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: 299RLVhtetOdRJwQbfJasVlDx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 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=bRd9RN1f; 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/9/23 21:40, Michael D Kinney wrote: > Hi Ranbir, >=20 > A deadloop without even a debug print is not good behavior. In DEBUG and NOOPT builds, the ASSERT() would fire, hence providing a debug message. In RELEASE builds, even if there were a separate DEBUG message, the DEBUG would be compiled out. >=20 > If this condition really represents a condition where it is not possible > to complete the PCI resource allocation/assignment, then an error status > code should be returned to the caller of NotifyPhase(). That's not the case here. This ASSERT() really cannot fire: https://edk2.groups.io/g/devel/message/110856 In other words, it is a *true* genuine assertion. It's only that Coverity cannot see that. Thus the idea here is to tell Coverity that we're willing to hang even in RELEASE builds. That hang will never ever occur (it can't), but by adding the explicit CpuDeadLoop(), we can placate Coverity (hopefully). Put differently: if we had an ASSERT() variant that could not be compiled out of RELEASE builds, then we'd use that variant here. Laszlo > Perhaps > EFI_OUT_OF_RESOURCES. The other ASSERT() conditions in this API should > likely be updated to do the same. >=20 > This may also require the caller of this service, the PCI Bus Driver, > to be reviewed to make sure it handles error conditions from NotifyPhase(= ). >=20 > I recommend you get help on the proposed code changes from the PCI > subsystem maintainers. >=20 > Thanks, >=20 > Mike >=20 >=20 >=20 >> -----Original Message----- >> From: devel@edk2.groups.io On Behalf Of Ranbir >> Singh >> Sent: Thursday, November 9, 2023 9:39 AM >> To: devel@edk2.groups.io; rsingh@ventanamicro.com >> Cc: Ni, Ray ; Veeresh Sangolli >> >> Subject: [edk2-devel] [PATCH v3 1/2] >> MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues >> >> From: Ranbir Singh >> >> The function NotifyPhase has a check >> >> ASSERT (Index < TypeMax); >> >> but this comes into play only in DEBUG mode. In Release mode, there is >> no handling if the Index value is within array limits or not. If for >> whatever reasons, the Index does not get re-assigned to Index2 at line >> 937, then it remains at TypeMax as assigned earlier at line 929. This >> poses array overrun risk at lines 942 and 943. It is better to deploy >> a safety check on Index limit before accessing array elements. >> >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D4212 >> >> Cc: Ray Ni >> Co-authored-by: Veeresh Sangolli >> Signed-off-by: Ranbir Singh >> Signed-off-by: Ranbir Singh >> --- >> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c >> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c >> index d573e532bac8..c2c143068cd2 100644 >> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c >> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c >> @@ -939,6 +939,11 @@ NotifyPhase ( >> } >> >> >> >> ASSERT (Index < TypeMax); >> >> + >> >> + if (Index =3D=3D TypeMax) { >> >> + CpuDeadLoop (); >> >> + } >> >> + >> >> ResNodeHandled[Index] =3D TRUE; >> >> Alignment =3D RootBridge- >>> ResAllocNode[Index].Alignment; >> >> BitsOfAlignment =3D LowBitSet64 (Alignment + 1); >> >> -- >> 2.34.1 >> >> >> >> -=3D-=3D-=3D-=3D-=3D-=3D >> Groups.io Links: You receive all messages sent to this group. >> View/Reply Online (#110993): >> https://edk2.groups.io/g/devel/message/110993 >> Mute This Topic: https://groups.io/mt/102490513/1643496 >> Group Owner: devel+owner@edk2.groups.io >> Unsubscribe: https://edk2.groups.io/g/devel/unsub >> [michael.d.kinney@intel.com] >> -=3D-=3D-=3D-=3D-=3D-=3D >> >=20 >=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 (#111163): https://edk2.groups.io/g/devel/message/111163 Mute This Topic: https://groups.io/mt/102490513/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-