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 0E2F5941CC6 for ; Tue, 14 Nov 2023 16:34:22 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=WYZwrdqE3x5fj267GjpAncR4WEEj89R2tEOvt6kI+6E=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1699979661; v=1; b=NvNSM/tMwB5x0YzrNhVSpHDYOyHH2whgI0RMrH1fNee5xIa647hGaeFwTVUs05eca1yql1PY jdEMnhm5HjPGnPn+Js5aAiBxJaTrmp1PYdYwNcnMBSs3ofZ02RpSUipLgu7uHMC0QQfW+flYcN2 n/IZE8AWZ7dqa120VKeBvU/s= X-Received: by 127.0.0.2 with SMTP id KwFDYY7687511xYiHhkzFg4z; Tue, 14 Nov 2023 08:34:21 -0800 X-Received: from mail-pj1-f52.google.com (mail-pj1-f52.google.com [209.85.216.52]) by mx.groups.io with SMTP id smtpd.web10.17168.1699979661024299097 for ; Tue, 14 Nov 2023 08:34:21 -0800 X-Received: by mail-pj1-f52.google.com with SMTP id 98e67ed59e1d1-28023eadc70so5030286a91.2 for ; Tue, 14 Nov 2023 08:34:20 -0800 (PST) X-Gm-Message-State: aBjjUaujjY0qIblJmd3lLyXTx7686176AA= X-Google-Smtp-Source: AGHT+IF9wgnVwfD9cLBeYUyjIL4UgqDD1+CkX5Q/nnB8gdjbHmLldTz6qjIClbYLlj9DglK3ra/1oGj9innAazVAEdI= X-Received: by 2002:a17:90a:d515:b0:280:23ef:b7fd with SMTP id t21-20020a17090ad51500b0028023efb7fdmr7826932pju.19.1699979660403; Tue, 14 Nov 2023 08:34:20 -0800 (PST) MIME-Version: 1.0 References: <20231109173908.364630-1-rsingh@ventanamicro.com> <20231109173908.364630-2-rsingh@ventanamicro.com> <88f1fa80-8639-3abb-8030-ec4f02b80358@redhat.com> In-Reply-To: <88f1fa80-8639-3abb-8030-ec4f02b80358@redhat.com> From: "Ranbir Singh" Date: Tue, 14 Nov 2023 22:04:09 +0530 Message-ID: Subject: Re: [edk2-devel] [PATCH v3 1/2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues To: Laszlo Ersek Cc: devel@edk2.groups.io, "Kinney, Michael D" , "Ni, Ray" , Veeresh Sangolli 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,rsingh@ventanamicro.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: multipart/alternative; boundary="000000000000c8bfb7060a1f5bd4" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b="NvNSM/tM"; dmarc=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 --000000000000c8bfb7060a1f5bd4 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, Nov 13, 2023 at 9:42=E2=80=AFPM Laszlo Ersek wr= ote: > On 11/10/23 05:07, Ranbir Singh wrote: > > Options before us till now - > > > > 1. Add array overrun check and Debug statement before CpuDeadLoop withi= n > > This would be useless. > > Your current patch implements the following pattern: > > ASSERT (X); > if (!X) { > CpuDeadLoop (); > } > > Option#1 would mean > > ASSERT (X); > if (!X) { > DEBUG ((DEBUG_ERROR, "%a: condition X failed\n", __func__)); > CpuDeadLoop (); > } > > Now compare the behavior of both code snippets. > > - In DEBUG and NOOPT builds, if X evaluated to FALSE, then the ASSERT() > would fire. A DEBUG_ERROR message would be logged, including "X", the > file name, and the line number. ASSERT() would then enter CpuDeadLoop() > internally, or invoke CpuBreakpoint() -- dependent on platform PCD > configuration. This applies to both snippets. In particular, the > explicit DEBUG and CpuDeadLoop() would not be reached, in the second > snippet. In other words, in DEBUG and NOOPT builds, the difference is > unobservable. > > - In RELEASE builds, the ASSERT() is compiled out of both snippets. > Furthermore, the explicit DEBUG is compiled out of the second snippet. > That is, both code snippets would silently enter CpuDeadLoop(). In other > words, the behavior of both snippets is undistinguishable in RELEASE > builds too. > > In my opinion, the current patch is right. > > Reviewed-by: Laszlo Ersek > > > To elaborate on that more generally: > > Core edk2 code has so consistently and so broadly *abused* and *misused* > ASSERT() for error checking, that now we fail to recognize an *actual > valid use* of ASSERT() for what it is. For illustration, compare the > following two code snippets: > > (a) > > UINTN Val; > > Val =3D 1 + 2; > ASSERT (Val =3D=3D 3); > > (b) > > VOID *Ptr; > > Ptr =3D AllocatePool (1024); > ASSERT (Ptr !=3D NULL); > > Snippet (a) is a *valid* use of an assert. It encapsulates a logical > predicate about the program's state, based on algorithmic and > mathematical reasoning. If the predicate turns out not to hold in > practice, at runtime,that means the programmer has retroactively caught > an issue in their own thinking, the algorithm is incorrectly designed or > implemented, and the program's state is effectively unknown at this > point (the internal invariant in question is broken), and so we must > stop immediately, because we don't know what the program is going to do > next. Given that what we thought about the program *thus far* has now > turned out to be false. > > Snippet (b) is an abuse of assert. AllocatePool()'s result depends on > the environment. Available memory, input data seen from the user or the > disk or the network thus far, and a bunch of other things not under our > control. Therefore, we must explicitly deal with AllocatePool() failing. > Claiming that AllocatePool() will succeed is wrong because we generally > cannot even *attempt* to prove it. > > In case (a) however, we can actually make a plausible attempt at > *proving* the predicate. The assert is there just in case our proof is > wrong. > > There's an immense difference between situations (a) and (b). I cannot > emphasize that enough. Case (a) is a provable statement about the > behavior of an algorithm. Case (b) is a *guess* at input data and > environment. > > The problem with edk2 core is that it has so consistently abused > ASSERT() for case (b) that now, when we occasionally see a *valid > instance* of (a), we mistake it for (b). > > That's the case here. The ASSERT() seen in this patch is case (a). It's > just that Coverity cannot prove it itself, because the predicate we > assert is much more complicated than (1 + 2 =3D=3D 3). But that doesn't > change the fact that we have a proof for the invariant in question. > > Therefore, the solution is not to try to handle an impossible error > gracefully. The solution is two-fold: > > - Tell coverity that we have a proof, in terms that it understands, to > shut it up. The terminology for this is CpuDeadLoop(). We're willing to > hang at runtime even in RELEASE builds, if the condition fails. > > - If, at runtime, our proof turns out to be wrong indeed, then we must > stop immediately (because if 1+2 stops equalling 3, then we can't trust > *anything else* that our program would do). > > (The more generic corollary of my last point, by the way, is that > ASSERT()s should NEVER be compiled out of programs. And if we actually > did that, like many other projects do, then this Coverity warning > wouldn't even exist, in the first place, because Coverity would *always* > see -- with the ASSERT() being there in all builds -- a range check on > Index. > > Put differently, having to add a CpuDeadLoop() explicitly is just > "damage control" for the self-inflicted damage of compiling out ASSERTs.) > > Laszlo > > Thanks Laszlo for being immensely generous in writing in that much detail. This must be beneficial to many. Though you already gave R-b, the return statement needs to be added to explicitly and completely rule out ARRAY_OVERRUN issue by static analysis tools. ASSERT (Index < TypeMax); + + if (Index =3D=3D TypeMax) { + CpuDeadLoop (); + return EFI_OUT_OF_RESOURCES; + } + > > > 2. Status Quo (not everything can be ideal :-)) > > > > Question before us > > - Is 1 better than 2 ? > > > > > > On Fri, Nov 10, 2023 at 8:41=E2=80=AFAM Ranbir Singh > > wrote: > > > > As far as I know, from a secure coding perspective, it would be > > recommended that array overrun condition check is captured in the > > code even if it is felt that it will never hit. > > > > Generally speaking, I won't be in favour of handling other ASSERT > > conditions updates even if required if they are not related to arra= y > > overrun conditions i.e., the context of the patch. > > > > If someone / PCI maintainers can advise in this patch context what > > should be done in the array overrun condition, I will be happy to > > update, otherwise, sorry to say I won't be able to pursue this > > particular one further and hence would be leaving the related code > > with the status quo here. > > > > On Fri, Nov 10, 2023 at 2:10=E2=80=AFAM Kinney, Michael D > > > > wrote: > > > > Hi Ranbir, > > > > A deadloop without even a debug print is not good behavior. > > > > 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(). Perhap= s > > EFI_OUT_OF_RESOURCES. The other ASSERT() conditions in this AP= I > > should > > likely be updated to do the same. > > > > 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(). > > > > I recommend you get help on the proposed code changes from the > PCI > > subsystem maintainers. > > > > Thanks, > > > > Mike > > > > > > > > > -----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 Index= 2 > > 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 t= o > > 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 > > -=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 (#111208): https://edk2.groups.io/g/devel/message/111208 Mute This Topic: https://groups.io/mt/102490513/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- --000000000000c8bfb7060a1f5bd4 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Mon, Nov 13, 2023 at 9:42=E2=80=AF= PM Laszlo Ersek <lersek@redhat.com<= /a>> wrote:
O= n 11/10/23 05:07, Ranbir Singh wrote:
> Options before us till now -
>
> 1. Add array overrun check and Debug statement before CpuDeadLoop with= in

This would be useless.

Your current patch implements the following pattern:

=C2=A0 ASSERT (X);
=C2=A0 if (!X) {
=C2=A0 =C2=A0 CpuDeadLoop ();
=C2=A0 }

Option#1 would mean

=C2=A0 ASSERT (X);
=C2=A0 if (!X) {
=C2=A0 =C2=A0 DEBUG ((DEBUG_ERROR, "%a: condition X failed\n", __= func__));
=C2=A0 =C2=A0 CpuDeadLoop ();
=C2=A0 }

Now compare the behavior of both code snippets.

- In DEBUG and NOOPT builds, if X evaluated to FALSE, then the ASSERT()
would fire. A DEBUG_ERROR message would be logged, including "X",= the
file name, and the line number. ASSERT() would then enter CpuDeadLoop()
internally, or invoke CpuBreakpoint() -- dependent on platform PCD
configuration. This applies to both snippets. In particular, the
explicit DEBUG and CpuDeadLoop() would not be reached, in the second
snippet. In other words, in DEBUG and NOOPT builds, the difference is
unobservable.

- In RELEASE builds, the ASSERT() is compiled out of both snippets.
Furthermore, the explicit DEBUG is compiled out of the second snippet.
That is, both code snippets would silently enter CpuDeadLoop(). In other words, the behavior of both snippets is undistinguishable in RELEASE
builds too.

In my opinion, the current patch is right.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>


To elaborate on that more generally:

Core edk2 code has so consistently and so broadly *abused* and *misused* ASSERT() for error checking, that now we fail to recognize an *actual
valid use* of ASSERT() for what it is. For illustration, compare the
following two code snippets:

(a)

=C2=A0 UINTN Val;

=C2=A0 Val =3D 1 + 2;
=C2=A0 ASSERT (Val =3D=3D 3);

(b)

=C2=A0 VOID *Ptr;

=C2=A0 Ptr =3D AllocatePool (1024);
=C2=A0 ASSERT (Ptr !=3D NULL);

Snippet (a) is a *valid* use of an assert. It encapsulates a logical
predicate about the program's state, based on algorithmic and
mathematical reasoning. If the predicate turns out not to hold in
practice, at runtime,that means the programmer has retroactively caught
an issue in their own thinking, the algorithm is incorrectly designed or implemented, and the program's state is effectively unknown at this
point (the internal invariant in question is broken), and so we must
stop immediately, because we don't know what the program is going to do=
next. Given that what we thought about the program *thus far* has now
turned out to be false.

Snippet (b) is an abuse of assert. AllocatePool()'s result depends on the environment. Available memory, input data seen from the user or the
disk or the network thus far, and a bunch of other things not under our
control. Therefore, we must explicitly deal with AllocatePool() failing. Claiming that AllocatePool() will succeed is wrong because we generally
cannot even *attempt* to prove it.

In case (a) however, we can actually make a plausible attempt at
*proving* the predicate. The assert is there just in case our proof is
wrong.

There's an immense difference between situations (a) and (b). I cannot<= br> emphasize that enough. Case (a) is a provable statement about the
behavior of an algorithm. Case (b) is a *guess* at input data and
environment.

The problem with edk2 core is that it has so consistently abused
ASSERT() for case (b) that now, when we occasionally see a *valid
instance* of (a), we mistake it for (b).

That's the case here. The ASSERT() seen in this patch is case (a). It&#= 39;s
just that Coverity cannot prove it itself, because the predicate we
assert is much more complicated than (1 + 2 =3D=3D 3). But that doesn't=
change the fact that we have a proof for the invariant in question.

Therefore, the solution is not to try to handle an impossible error
gracefully. The solution is two-fold:

- Tell coverity that we have a proof, in terms that it understands, to
shut it up. The terminology for this is CpuDeadLoop(). We're willing to=
hang at runtime even in RELEASE builds, if the condition fails.

- If, at runtime, our proof turns out to be wrong indeed, then we must
stop immediately (because if 1+2 stops equalling 3, then we can't trust=
*anything else* that our program would do).

(The more generic corollary of my last point, by the way, is that
ASSERT()s should NEVER be compiled out of programs. And if we actually
did that, like many other projects do, then this Coverity warning
wouldn't even exist, in the first place, because Coverity would *always= *
see -- with the ASSERT() being there in all builds -- a range check on
Index.

Put differently, having to add a CpuDeadLoop() explicitly is just
"damage control" for the self-inflicted damage of compiling out A= SSERTs.)

Laszlo


Thanks Laszlo for being immensely gene= rous in writing in that much detail. This must be beneficial to many.
=

Though you already gave R-b, the return statement needs= to be added to explicitly and completely rule out ARRAY_OVERRUN issue by s= tatic analysis tools.

=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0ASSERT (Index < TypeMax);
+
+=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 if (Index =3D=3D TypeMax) {
+=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 CpuDeadLoop ();
+ =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0return EFI_OUT_OF_RESOURCES;
+=C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
+



> 2. Status Quo (not everything=C2=A0can be ideal :-))
>
> Question before us
> =C2=A0 =C2=A0 =C2=A0- Is 1 better than 2 ?
>
>
> On Fri, Nov 10, 2023 at 8:41=E2=80=AFAM Ranbir Singh <rsingh@ventanamicro.com=
> <mailto:rsingh@ventanamicro.com>> wrote:
>
>=C2=A0 =C2=A0 =C2=A0As far as I know, from a secure coding perspective,= it would be
>=C2=A0 =C2=A0 =C2=A0recommended that array overrun condition check is c= aptured in the
>=C2=A0 =C2=A0 =C2=A0code even if it=C2=A0is felt that it will never hit= .
>
>=C2=A0 =C2=A0 =C2=A0Generally speaking, I won't be in favour of han= dling other=C2=A0ASSERT
>=C2=A0 =C2=A0 =C2=A0conditions updates=C2=A0even if required if=C2=A0th= ey are not related=C2=A0to array
>=C2=A0 =C2=A0 =C2=A0overrun conditions i.e., the context of the patch.<= br> >
>=C2=A0 =C2=A0 =C2=A0If someone / PCI maintainers can advise in this pat= ch context what
>=C2=A0 =C2=A0 =C2=A0should be done in the array overrun condition, I wi= ll be happy to
>=C2=A0 =C2=A0 =C2=A0update, otherwise, sorry to say I won't be able= to pursue this
>=C2=A0 =C2=A0 =C2=A0particular one further and hence would be leaving t= he related code
>=C2=A0 =C2=A0 =C2=A0with the status quo here.
>
>=C2=A0 =C2=A0 =C2=A0On Fri, Nov 10, 2023 at 2:10=E2=80=AFAM Kinney, Mic= hael D
>=C2=A0 =C2=A0 =C2=A0<michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com<= /a>>> wrote:
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Hi Ranbir,
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0A deadloop without even a debug print= is not good behavior.
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0If this condition really represents a= condition where it is not
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0possible
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0to complete the PCI resource allocati= on/assignment, then an
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0error status
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0code should be returned to the caller= of NotifyPhase().=C2=A0 Perhaps
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0EFI_OUT_OF_RESOURCES.=C2=A0 The other= ASSERT() conditions in this API
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0should
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0likely be updated to do the same.
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0This may also require the caller of t= his service, the PCI Bus
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Driver,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0to be reviewed to make sure it handle= s error conditions from
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0NotifyPhase().
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0I recommend you get help on the propo= sed code changes from the PCI
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0subsystem maintainers.
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Thanks,
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Mike
>
>
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> -----Original Message-----
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> From: devel@edk2.groups.io <mailto:devel@edk2.groups.io= >
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0<devel@edk2.groups.io <mailto:devel@edk2.groups.io>>= ; On Behalf
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Of Ranbir
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> Singh
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> Sent: Thursday, November 9, 2023= 9:39 AM
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> To: devel@edk2.groups.io <mailto:devel@edk2.groups.io= >;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0rsingh@ventanamicro.com <mailto:rsingh@ventanamicro.com>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> Cc: Ni, Ray <ray.ni@intel.com <mailto:ray.ni@intel.com>&g= t;;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Veeresh Sangolli
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> <veeresh.sangolli@dellteam.com >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0<mailto:veeresh.sangolli@dellteam.com&= gt;>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> Subject: [edk2-devel] [PATCH v3 = 1/2]
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> MdeModulePkg/Bus/Pci/PciHostBrid= geDxe: Fix OVERRUN Coverity issues
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> From: Ranbir Singh <Ranbir.Si= ngh3@Dell.com>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> The function NotifyPhase has a c= heck
>=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=A0ASSERT (Index= < TypeMax);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> but this comes into play only in= DEBUG mode. In Release mode,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0there is
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> no handling if the Index value i= s within array limits or not.
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0If for
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> whatever reasons, the Index does= not get re-assigned to Index2
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0at line
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> 937, then it remains at TypeMax = as assigned earlier at line
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0929. This
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> poses array overrun risk at line= s 942 and 943. It is better to
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0deploy
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> a safety check on Index limit be= fore accessing array elements.
>=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=3D4212
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0<https= ://bugzilla.tianocore.org/show_bug.cgi?id=3D4212>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> Cc: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com>>= ;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> Co-authored-by: Veeresh Sangolli=
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0<veeresh.sangolli@dellteam.com
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0<mailto:veeresh.sangolli@dellteam.com&= gt;>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> Signed-off-by: Ranbir Singh <= Ranbir.Singh3@Dell.com>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> Signed-off-by: Ranbir Singh <= rsingh@ventana= micro.com
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0<mailto:rsingh@ventanamicro.com>>
>=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/PciHo= stBridgeDxe/PciHostBridge.c | 5 +++++
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0>=C2=A0 1 file changed, 5 insertio= ns(+)
>=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/Pc= i/PciHostBridgeDxe/PciHostBridge.c
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> b/MdeModulePkg/Bus/Pci/PciHostBr= idgeDxe/PciHostBridge.c
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> index d573e532bac8..c2c143068cd2= 100644
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> --- a/MdeModulePkg/Bus/Pci/PciHo= stBridgeDxe/PciHostBridge.c
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> +++ b/MdeModulePkg/Bus/Pci/PciHo= stBridgeDxe/PciHostBridge.c
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> @@ -939,6 +939,11 @@ NotifyPhase= (
>=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 =C2=A0 =C2=A0 =C2=A0 =C2=A0>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 ASSERT (Index < TypeMax);
>=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 if (Index =3D=3D TypeMax) {
>=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 CpuDeadLoop ();
>=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 =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 ResNodeHandled[Index] =3D TRUE;
>=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 Alignment=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =3D RootBridge-
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> >ResAllocNode[Index].Alignmen= t;
>=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 BitsOfAlignment=C2=A0 =C2=A0 =C2=A0 =C2=A0=3D LowBitSet64= (Alignment + 1);
>=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> 2.34.1
>=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> -=3D-=3D-=3D-=3D-=3D-=3D
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> Groups.io Links: You receive all= messages sent to this group.
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> View/Reply Online (#110993):
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> https://edk2= .groups.io/g/devel/message/110993
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0<https://edk2.= groups.io/g/devel/message/110993>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> Mute This Topic: ht= tps://groups.io/mt/102490513/1643496
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0<https://groups.io/mt= /102490513/1643496>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> Group Owner: devel+owner@edk2.groups.io
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0<mailto:devel%2Bowner@edk2.groups.io&= gt;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> Unsubscribe: https://= edk2.groups.io/g/devel/unsub
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0<https://edk2.groups.io= /g/devel/unsub>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> [michael.d.kinney@intel.com <mailto:= michael.d.k= inney@intel.com>]
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0> -=3D-=3D-=3D-=3D-=3D-=3D
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0>
>=20
_._,_._,_
Groups.io Links:

=20 You receive all messages sent to this group. =20 =20

View/Reply Online (#111208) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--000000000000c8bfb7060a1f5bd4--