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 8857B78003C for ; Fri, 10 Nov 2023 04:08:00 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=zBl3dXcT7FzVvciZ2NaTheIkI6APg3TstlM2y10UNU4=; 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=1699589278; v=1; b=wbgNhJoHruC1faYDAxJJtRyibc9nfBeeXLtTTdrtCtqfvQ8VltGDuKl76BZ++0lYQ1PDC92B 0f6ZTUDNacNwykEgcFv9gQbrHkiqil7+qSJ8NrYm7sxAxESmBzSdNXHNgZ/zdcnk/OVXq/O8dGL BCVm33VLIdgDu6d6exQCaC5w= X-Received: by 127.0.0.2 with SMTP id 7AGnYY7687511x9Aft6JOVDf; Thu, 09 Nov 2023 20:07:58 -0800 X-Received: from mail-pg1-f177.google.com (mail-pg1-f177.google.com [209.85.215.177]) by mx.groups.io with SMTP id smtpd.web10.21093.1699589277710031912 for ; Thu, 09 Nov 2023 20:07:57 -0800 X-Received: by mail-pg1-f177.google.com with SMTP id 41be03b00d2f7-5bd33a450fdso1345113a12.0 for ; Thu, 09 Nov 2023 20:07:57 -0800 (PST) X-Gm-Message-State: 5e2gqVJEsjX3vaF0CmHWDJFQx7686176AA= X-Google-Smtp-Source: AGHT+IEp7X7XJsSaw4JO3wu0+Zf1yUPN512J9OYbhgBvsAafwsQSHIMWtEtg8FVgYFbtF7qA/NEoLlc4Al867LVdPeo= X-Received: by 2002:a05:6300:8005:b0:17a:de5d:1d7e with SMTP id an5-20020a056300800500b0017ade5d1d7emr7620448pzc.55.1699589276982; Thu, 09 Nov 2023 20:07:56 -0800 (PST) MIME-Version: 1.0 References: <20231109173908.364630-1-rsingh@ventanamicro.com> <20231109173908.364630-2-rsingh@ventanamicro.com> In-Reply-To: From: "Ranbir Singh" Date: Fri, 10 Nov 2023 09:37:46 +0530 Message-ID: Subject: Re: [edk2-devel] [PATCH v3 1/2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues To: "Kinney, Michael D" Cc: "devel@edk2.groups.io" , "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="0000000000001e70790609c47720" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=wbgNhJoH; 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 --0000000000001e70790609c47720 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Options before us till now - 1. Add array overrun check and Debug statement before CpuDeadLoop within 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 ev= en > 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 array > 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 < > michael.d.kinney@intel.com> 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(). Perhaps >> EFI_OUT_OF_RESOURCES. The other ASSERT() conditions in this API 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 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 >> > >> >> -=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 (#111009): https://edk2.groups.io/g/devel/message/111009 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- --0000000000001e70790609c47720 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Options before us till now -

1. Add arr= ay overrun check and Debug statement before CpuDeadLoop within
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@ventana= micro.com> wrote:
As far as I know, from a secure coding per= spective, it would be recommended that array overrun condition check is cap= tured in the code even if it=C2=A0is felt that it will never hit.
=

Generally speaking, I won't be in favour of ha= ndling other=C2=A0ASSERT conditions updates=C2=A0even if required if=C2=A0t= hey are not related=C2=A0to array overrun conditions i.e., the context of t= he patch.

If someone / PCI mainta= iners can advise in this patch context what should be done in the array ove= rrun condition, I will be happy to update, otherwise, sorry to say I won= 9;t be able to pursue this particular one further and hence would be leavin= g the related code with the status quo here.

On Fri, Nov 10, 2023 at 2= :10=E2=80=AFAM Kinney, Michael D <michael.d.kinney@intel.com> 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().=C2=A0 Perhaps
EFI_OUT_OF_RESOURCES.=C2=A0 The other ASSERT() conditions in this API shoul= d
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 <devel@edk2.groups.io> On Behalf Of Ranbir
> Singh
> Sent: Thursday, November 9, 2023 9:39 AM
> To: devel@ed= k2.groups.io; rsingh@ventanamicro.com
> Cc: Ni, Ray <= ray.ni@intel.com>; Veeresh Sangolli
> <veeresh.sangolli@dellteam.com>
> Subject: [edk2-devel] [PATCH v3 1/2]
> MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues
>
> From: Ranbir Singh <Ranbir.Singh3@Dell.com>
>
> The function NotifyPhase has a check
>
>=C2=A0 =C2=A0 =C2=A0ASSERT (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<= br> > poses array overrun risk at lines 942 and 943. It is better to deploy<= br> > a safety check on Index limit before accessing array elements.
>
> REF: https://bugzilla.tianocore.org/show_b= ug.cgi?id=3D4212
>
> Cc: Ray Ni <r= ay.ni@intel.com>
> Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
> Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> ---
>=C2=A0 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 5 +++++<= br> >=C2=A0 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 (
>=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 < Typ= eMax);
>
> +
>
> +=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=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 Alignment=C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=3D RootBridge-
> >ResAllocNode[Index].Alignment;
>
>=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);
>
> --
> 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<= br> > Mute This Topic: https://groups.io/mt/102490513/1643496
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub
> [micha= el.d.kinney@intel.com]
> -=3D-=3D-=3D-=3D-=3D-=3D
>

_._,_._,_
Groups.io Links:

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

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

_._,_._,_
--0000000000001e70790609c47720--