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 E1D5F9418D7 for ; Tue, 7 Nov 2023 15:47:36 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=u5wOH+gYwUSTEgXWLcBfJVrNW8ZGX9gvVPhJASg0Nac=; 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=1699372055; v=1; b=SN2ANdLliBGZ9o8g1gYImmxL+JICOETI7YbZfKa6aKoTErzQAURtP73RLuCqoLmYQjPTjqD3 82sVE/GK6JbHbA8TIfrac0rFL0HkSLpCrv2ksHwKUHnBpTXPMN4eL9wTEjsRB2eL/+ZZG+WJGCA +8YUZj3w+35zK08mat3VBEt0= X-Received: by 127.0.0.2 with SMTP id 1x8dYY7687511xFbsiKbiR3P; Tue, 07 Nov 2023 07:47:35 -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.14317.1699372054771240851 for ; Tue, 07 Nov 2023 07:47:34 -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-160--baE5wWsNsqT44p9W6_ubQ-1; Tue, 07 Nov 2023 10:47:28 -0500 X-MC-Unique: -baE5wWsNsqT44p9W6_ubQ-1 X-Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (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 79C3A82BA86; Tue, 7 Nov 2023 15:47:28 +0000 (UTC) X-Received: from [10.39.193.64] (unknown [10.39.193.64]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 9CEB940C6EBC; Tue, 7 Nov 2023 15:47:27 +0000 (UTC) Message-ID: <025f01a8-af6c-4d58-ae74-3f3865eec3c5@redhat.com> Date: Tue, 7 Nov 2023 16:47:26 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v2 1/2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues To: devel@edk2.groups.io, rsingh@ventanamicro.com Cc: Ray Ni , Veeresh Sangolli References: <20231107050647.59613-1-rsingh@ventanamicro.com> <20231107050647.59613-2-rsingh@ventanamicro.com> From: "Laszlo Ersek" In-Reply-To: <20231107050647.59613-2-rsingh@ventanamicro.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.2 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: gVGZKAQbfE5ZdxXYyb7qwW3Ox7686176AA= 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=SN2ANdLl; 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 Hi Ranbir, On 11/7/23 06:06, Ranbir Singh wrote: > From: Ranbir Singh >=20 > The function NotifyPhase has a check >=20 > ASSERT (Index < TypeMax); >=20 > 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 > 137, then it remains at TypeMax as assigned earlier at line 929. This 137 should be 937 > poses array overrun risk at lines 942 and 943. It is better to deploy > a safety check on Index limit before accessing array elements. >=20 > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D4212 >=20 > 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(+) >=20 > diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c b/MdeM= odulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c > index d573e532bac8..519e1369f85e 100644 > --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c > +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c > @@ -939,6 +939,11 @@ NotifyPhase ( > } > =20 > ASSERT (Index < TypeMax); > + > + if (Index >=3D TypeMax) { > + continue; > + } > + > ResNodeHandled[Index] =3D TRUE; > Alignment =3D RootBridge->ResAllocNode[Index].Al= ignment; > BitsOfAlignment =3D LowBitSet64 (Alignment + 1); The ASSERT() will never fire. But I agree that it is hard to see. I propose that we should add if (Index =3D=3D TypeMax) { CpuDeadLoop (); } instead of "continue". Here's why the ASSERT() will never fire. - The outer loop (using Index1) will run five times exactly. - In each execution of the outer loop, we have two branches. Each branch fl= ips *at most* one element in ResNodeHandled from FALSE to TRUE. While each branch writes to exactly one ResNodeHandled element (storing TRU= E), the original ResNodeHandled value may be FALSE, or may be TRUE. (TRUE a= s original value is not easy to see, but consider that the first branch of = the outer loop body may notice ResNone for a particular resource type *afte= r* the second branch of the outer loop body has assigned a resource to that= type. That *is* a bug, but a *different* one!) The point is that the FALSE->TRUE *transition* may happen for at most one r= esource type per outer loop iteration. This means that in the Nth iteration= of the outer loop (Index1=3D0, 1, ... 4 inclusive), there are initially *a= t least* (5 - Index1) FALSE elements in ResNodeHandled. In the last iterati= on of the outer loop (Index1=3D4), there is at least 5 - 4 =3D 1 FALSE elem= ent in ResNodeHandled. - This means that the Index2-based inner loop will *always find* an Index2 = where ResNodeHandled is FALSE. - For the first such Index2 in the inner loop body, Index will be assigned,= because MaxAlignment starts with 0, and the Alignment field has type UINT6= 4. Therefore the ASSERT will never fire -- it is a correct assertion. Basically the assert states that we have a resource type to assign at that = point -- and that claim is correct. So, unfortunately, Coverity is wrong he= re. We should add a CpuDeadLoop() therefore, to tell coverity that we're wi= lling to hang there even in RELEASE builds. "continue" is not useful in any case, because if there are no more resource= types to assign, then continuing the outer loop makes no sense. That is, "= break" would make more sense. (But again, that too would never be reached.) ... For making the code easier to understand, I'd perhaps propose (this is = displayed with "git diff -b"): diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c b/MdeMod= ulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c index d573e532bac8..87c85e9df771 100644 --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c @@ -833,11 +833,12 @@ NotifyPhase ( EFI_STATUS Status; EFI_STATUS ReturnStatus; PCI_RESOURCE_TYPE Index; - PCI_RESOURCE_TYPE Index1; PCI_RESOURCE_TYPE Index2; BOOLEAN ResNodeHandled[TypeMax]; UINT64 MaxAlignment; UINT64 Translation; + UINTN ToAssign; + UINTN Assigned; =20 HostBridge =3D PCI_HOST_BRIDGE_FROM_THIS (This); =20 @@ -911,17 +912,20 @@ NotifyPhase ( ; Link =3D GetNextNode (&HostBridge->RootBridges, Link) ) { + ToAssign =3D 0; for (Index =3D TypeIo; Index < TypeBus; Index++) { + if (RootBridge->ResAllocNode[Index].Status =3D=3D ResNone) { + ResNodeHandled[Index] =3D TRUE; + } else { ResNodeHandled[Index] =3D FALSE; + ToAssign++; + } } =20 RootBridge =3D ROOT_BRIDGE_FROM_LINK (Link); DEBUG ((DEBUG_INFO, " RootBridge: %s\n", RootBridge->DevicePathStr= )); =20 - for (Index1 =3D TypeIo; Index1 < TypeBus; Index1++) { - if (RootBridge->ResAllocNode[Index1].Status =3D=3D ResNone) { - ResNodeHandled[Index1] =3D TRUE; - } else { + for (Assigned =3D 0; Assigned < ToAssign; Assigned++) { // // Allocate the resource node with max alignment at first // @@ -1091,7 +1095,6 @@ NotifyPhase ( } } } - } =20 if (ReturnStatus =3D=3D EFI_OUT_OF_RESOURCES) { ResourceConflict (HostBridge); Thanks Laszlo -=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 (#110856): https://edk2.groups.io/g/devel/message/110856 Mute This Topic: https://groups.io/mt/102437647/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-