From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pj1-f41.google.com (mail-pj1-f41.google.com [209.85.216.41]) by mx.groups.io with SMTP id smtpd.web11.2095.1685417763167363228 for ; Mon, 29 May 2023 20:36:03 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="signature has expired" header.i=@ventanamicro.com header.s=google header.b=GvhT+I9Z; spf=pass (domain: ventanamicro.com, ip: 209.85.216.41, mailfrom: rsingh@ventanamicro.com) Received: by mail-pj1-f41.google.com with SMTP id 98e67ed59e1d1-256712e2be3so1726884a91.2 for ; Mon, 29 May 2023 20:36:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ventanamicro.com; s=google; t=1685417762; x=1688009762; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=gncX/ZgD4z4MA2/zeBtgmXiQD23uPhagppCuADKxCHk=; b=GvhT+I9ZQWNPnRF4lhLUfmWSTLI9u4odiih/hBgJd7nDfxfqbYgORqDCJKAKE1JRvK ZxZLjN4VNCLH/BKtDqInZqizLXLw2Cd3QWOuxXFuJLpmkkTNCPPAwZmmPSmVxBj4YFC4 eJGps5zmFGEkhu4yUR0JBDXQxG2c3PPvrlpY5iLGCGnKph1noGyHgm9LtSG9xXI0lWjv +FwkGxWtefPW1T+qZLT3gccBPJJCWKOcWBnq1fF0ZPe97CsgFJS/2rSYWTKX5uOZbyIN eMu0X+V17DHKaiXXlTwNSJrHdw1Vnm3FngQSuD0lDYk5mVsJVAHzNsXf7f/N5Pj7qhMw Ld7w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685417762; x=1688009762; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=gncX/ZgD4z4MA2/zeBtgmXiQD23uPhagppCuADKxCHk=; b=RR1Qo3iLQUCSE/QG+NXRKWdO9h2RyXRsxeWHkS3LyMmT4/UD5LUQjlirz97yQ79/DS /Fc5fR+Dquy76GPYj1xg5+RCd5s2Ob6ooekwH8bbTSYikppq+zE0U03mTsJTW8K/mUc/ MgGbjyZZgd/mQNqIsAuy/vPyTowhcEpB7yNJJCy0/rHDJt+6bHyo5lb9Wx425VxUTsae 8Vcf87plpEIayXxXjJhYtGkRE75Dlnh7+V44T/NCnPwIVe+G9TIVDOl1d5GRVfDjCA+0 Z/dRvrSkwViaUHZe/OIfMOMYTOOqaO8S+01zP7N3m7xO5cUZjK2AMyHXUEcfvwqXJTcE CvjA== X-Gm-Message-State: AC+VfDxJRsZ/yPF8jL6W7qj4vRBNG5cUw2aewGoTPrMDuyivrVju7mgd zQv9K5tLpM4+wAHEXZRZmhqnQSmGNQynCUlAk7gGgQ== X-Google-Smtp-Source: ACHHUZ6vGPRKRdbTVXFhHRQOi1WhSpSCnWfjGALGc0hX/rHbdFeL4gkQw2JNkApUesEAoBn//m2Lm/mhK12rpuWAy9o= X-Received: by 2002:a17:902:b498:b0:1a6:6fe3:df8d with SMTP id y24-20020a170902b49800b001a66fe3df8dmr947890plr.8.1685417762543; Mon, 29 May 2023 20:36:02 -0700 (PDT) MIME-Version: 1.0 References: <20230518062851.184724-1-rsingh@ventanamicro.com> In-Reply-To: From: Ranbir Singh Date: Tue, 30 May 2023 09:05:52 +0530 Message-ID: Subject: Re: [edk2-devel] [PATCH 1/1] IntelFsp2Pkg/Library/BaseFspCommonLib: Fix OVERRUN Coverity issue To: "Chiu, Chasel" Cc: "devel@edk2.groups.io" , "pedro.falcato@gmail.com" , "Desimone, Nathaniel L" , "Zeng, Star" , Ranbir Singh Content-Type: multipart/alternative; boundary="00000000000008f56905fce0e771" --00000000000008f56905fce0e771 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Yes Chasel - please do so while merging, thanks! On Tue, May 30, 2023 at 8:58=E2=80=AFAM Chiu, Chasel wrote: > > That=E2=80=99s good suggestion Pedro! > Ranbir, would you like me to modify your patch to "return 0" during > merging? > > Thanks, > Chasel > > > > -----Original Message----- > > From: devel@edk2.groups.io On Behalf Of Pedro > > Falcato > > Sent: Friday, May 19, 2023 5:29 AM > > To: devel@edk2.groups.io; rsingh@ventanamicro.com > > Cc: Chiu, Chasel ; Desimone, Nathaniel L > > ; Zeng, Star ; > Ranbir > > Singh > > Subject: Re: [edk2-devel] [PATCH 1/1] > IntelFsp2Pkg/Library/BaseFspCommonLib: > > Fix OVERRUN Coverity issue > > > > On Thu, May 18, 2023 at 4:16=E2=80=AFPM Ranbir Singh > > wrote: > > > > > > FspData->PerfIdx is getting increased for every call unconditionally > > > in the function SetFspMeasurePoint and hence memory access can happen > > > for out of bound FspData->PerfData[] array entries also. > > > > > > Example - > > > FspData->PerfData is an array of 32 UINT64 entries. Assume a call > > > is made to SetFspMeasurePoint function when the FspData->PerfIdx > > > last value is 31. It gets incremented to 32 at line 400. > > > Any subsequent call to SetFspMeasurePoint functions leads to > > > FspData->PerfData[32] getting accessed which is out of the PerfDat= a > > > array as well as the FSP_GLOBAL_DATA structure boundary. > > > > > > Hence keep array access and index increment inside if block only and > > > return invalid performance timestamp when PerfIdx is invalid. > > > > > > Cc: Chasel Chiu > > > Cc: Nate DeSimone > > > Cc: Star Zeng > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D4200 > > > Signed-off-by: Ranbir Singh > > > Signed-off-by: Ranbir Singh > > > --- > > > IntelFsp2Pkg/Library/BaseFspCommonLib/FspCommonLib.c | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/IntelFsp2Pkg/Library/BaseFspCommonLib/FspCommonLib.c > > > b/IntelFsp2Pkg/Library/BaseFspCommonLib/FspCommonLib.c > > > index a22b0e7825ad..cda2a7b2478e 100644 > > > --- a/IntelFsp2Pkg/Library/BaseFspCommonLib/FspCommonLib.c > > > +++ b/IntelFsp2Pkg/Library/BaseFspCommonLib/FspCommonLib.c > > > @@ -377,7 +377,8 @@ GetFspSiliconInitUpdDataPointer ( > > > > > > @param[in] Id Measurement point ID. > > > > > > - @return performance timestamp. > > > + @return performance timestamp if current PerfIdx is valid, > > > + else return 0 as invalid performance timestamp > > > **/ > > > UINT64 > > > EFIAPI > > > @@ -395,9 +396,10 @@ SetFspMeasurePoint ( > > > if (FspData->PerfIdx < sizeof (FspData->PerfData) / sizeof (FspDat= a- > > >PerfData[0])) { > > > FspData->PerfData[FspData->PerfIdx] =3D AsmRead= Tsc > (); > > > ((UINT8 *)(&FspData->PerfData[FspData->PerfIdx]))[7] =3D Id; > > > + return FspData->PerfData[(FspData->PerfIdx)++]; > > > } > > > > > > - return FspData->PerfData[(FspData->PerfIdx)++]; > > > + return (UINT64)0x0000000000000000; > > > > return 0; > > > > Works just as well. You also don't need a cast. > > > > https://godbolt.org/z/e5vvGcWWo > > > > -- > > Pedro > > > > > >=20 > > > > --00000000000008f56905fce0e771 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Yes Chasel - please do so while merging, thanks!=C2=A0
=

= On Tue, May 30, 2023 at 8:58=E2=80=AFAM Chiu, Chasel <chasel.chiu@intel.com> wrote:

That=E2=80=99s good suggestion Pedro!
Ranbir, would you like me to modify your patch to "return 0" duri= ng merging?

Thanks,
Chasel


> -----Original Message-----
> From: devel@= edk2.groups.io <devel@edk2.groups.io> On Behalf Of Pedro
> Falcato
> Sent: Friday, May 19, 2023 5:29 AM
> To: devel@ed= k2.groups.io; rsingh@ventanamicro.com
> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Zeng, Star <star.zeng@intel.com>; Ranbir<= br> > Singh <= Ranbir.Singh3@dell.com>
> Subject: Re: [edk2-devel] [PATCH 1/1] IntelFsp2Pkg/Library/BaseFspComm= onLib:
> Fix OVERRUN Coverity issue
>
> On Thu, May 18, 2023 at 4:16=E2=80=AFPM Ranbir Singh <rsingh@ventanamicro.com= >
> wrote:
> >
> > FspData->PerfIdx is getting increased for every call unconditi= onally
> > in the function SetFspMeasurePoint and hence memory access can ha= ppen
> > for out of bound FspData->PerfData[] array entries also.
> >
> > Example -
> >=C2=A0 =C2=A0 FspData->PerfData is an array of 32 UINT64 entrie= s. Assume a call
> >=C2=A0 =C2=A0 is made to SetFspMeasurePoint function when the FspD= ata->PerfIdx
> >=C2=A0 =C2=A0 last value is 31. It gets incremented to 32 at line = 400.
> >=C2=A0 =C2=A0 Any subsequent call to SetFspMeasurePoint functions = leads to
> >=C2=A0 =C2=A0 FspData->PerfData[32] getting accessed which is o= ut of the PerfData
> >=C2=A0 =C2=A0 array as well as the FSP_GLOBAL_DATA structure bound= ary.
> >
> > Hence keep array access and index increment inside if block only = and
> > return invalid performance timestamp when PerfIdx is invalid.
> >
> > Cc: Chasel Chiu <chasel.chiu@intel.com>
> > Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > REF: https://bugzilla.tianocore.org/s= how_bug.cgi?id=3D4200
> > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> > ---
> >=C2=A0 IntelFsp2Pkg/Library/BaseFspCommonLib/FspCommonLib.c | 6 ++= ++--
> >=C2=A0 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/IntelFsp2Pkg/Library/BaseFspCommonLib/FspCommonLib.c=
> > b/IntelFsp2Pkg/Library/BaseFspCommonLib/FspCommonLib.c
> > index a22b0e7825ad..cda2a7b2478e 100644
> > --- a/IntelFsp2Pkg/Library/BaseFspCommonLib/FspCommonLib.c
> > +++ b/IntelFsp2Pkg/Library/BaseFspCommonLib/FspCommonLib.c
> > @@ -377,7 +377,8 @@ GetFspSiliconInitUpdDataPointer (
> >
> >=C2=A0 =C2=A0 @param[in] Id=C2=A0 =C2=A0 =C2=A0 =C2=A0Measurement = point ID.
> >
> > -=C2=A0 @return performance timestamp.
> > +=C2=A0 @return performance timestamp if current PerfIdx is valid= ,
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 else return 0 as invalid perf= ormance timestamp
> >=C2=A0 **/
> >=C2=A0 UINT64
> >=C2=A0 EFIAPI
> > @@ -395,9 +396,10 @@ SetFspMeasurePoint (
> >=C2=A0 =C2=A0 if (FspData->PerfIdx < sizeof (FspData->Per= fData) / sizeof (FspData-
> >PerfData[0])) {
> >=C2=A0 =C2=A0 =C2=A0 FspData->PerfData[FspData->PerfIdx]=C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =3D AsmReadTsc = ();
> >=C2=A0 =C2=A0 =C2=A0 ((UINT8 *)(&FspData->PerfData[FspData-= >PerfIdx]))[7] =3D Id;
> > +=C2=A0 =C2=A0 return FspData->PerfData[(FspData->PerfIdx)+= +];
> >=C2=A0 =C2=A0 }
> >
> > -=C2=A0 return FspData->PerfData[(FspData->PerfIdx)++];
> > +=C2=A0 return (UINT64)0x0000000000000000;
>
> return 0;
>
> Works just as well. You also don't need a cast.
>
> https://godbolt.org/z/e5vvGcWWo
>
> --
> Pedro
>
>
>
>

--00000000000008f56905fce0e771--