From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.151, mailfrom: chasel.chiu@intel.com) Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by groups.io with SMTP; Wed, 04 Sep 2019 19:59:04 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 04 Sep 2019 19:59:04 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,469,1559545200"; d="scan'208";a="199176581" Received: from kmsmsx151.gar.corp.intel.com ([172.21.73.86]) by fmsmga001.fm.intel.com with ESMTP; 04 Sep 2019 19:59:01 -0700 Received: from pgsmsx111.gar.corp.intel.com ([169.254.2.22]) by KMSMSX151.gar.corp.intel.com ([169.254.10.91]) with mapi id 14.03.0439.000; Thu, 5 Sep 2019 10:53:58 +0800 From: "Chiu, Chasel" To: "Zhang, Shenglei" , "devel@edk2.groups.io" CC: "Kubacki, Michael A" , "Desimone, Nathaniel L" , "Gao, Liming" Subject: Re: [edk2-devel] [PATCH] MinPlatformPkg/SecFspWrapperPlatformSecLib: Change TopOfTemporaryRam type Thread-Topic: [edk2-devel] [PATCH] MinPlatformPkg/SecFspWrapperPlatformSecLib: Change TopOfTemporaryRam type Thread-Index: AQHVYYrWcH+ZB150L0CdngW7WLvkd6cbb9XggABlUACAAI8QoA== Date: Thu, 5 Sep 2019 02:53:57 +0000 Message-ID: <3C3EFB470A303B4AB093197B6777CCEC50481D6C@PGSMSX111.gar.corp.intel.com> References: <20190902123452.27716-1-shenglei.zhang@intel.com> <3C3EFB470A303B4AB093197B6777CCEC5048145B@PGSMSX111.gar.corp.intel.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMzZmNjY5YmYtMGI3OC00NzlhLTg1NWMtYTAxOWY2NzNlMTdlIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiaWFiV3J6OTJSWkhOU1QwbXNuMFNhb3NyVkxuV1dcL3NTa25HYUxLRDl1dG9URGxBSUxMQUVLanV2NjRZTVwvcTdcLyJ9 x-ctpclassification: CTP_NT x-originating-ip: [172.30.20.206] MIME-Version: 1.0 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Thanks for explanations. So looks like the intention is to support X64 build scenarios, I think Top= OfTemporaryRam should be defined as UINTN from beginning so it can hold 64b= its address in X64 build. Please evaluate if we could make these changes. Thanks! Chasel > -----Original Message----- > From: Zhang, Shenglei > Sent: Thursday, September 5, 2019 10:11 AM > To: Chiu, Chasel ; devel@edk2.groups.io > Cc: Kubacki, Michael A ; Desimone, Nathanie= l > L ; Gao, Liming > Subject: RE: [edk2-devel] [PATCH] > MinPlatformPkg/SecFspWrapperPlatformSecLib: Change > TopOfTemporaryRam type >=20 > Hi chasel, >=20 > > -----Original Message----- > > From: Chiu, Chasel > > Sent: Wednesday, September 4, 2019 8:13 PM > > To: devel@edk2.groups.io; Zhang, Shenglei > > Cc: Kubacki, Michael A ; Desimone, > > Nathaniel L ; Gao, Liming > > > > Subject: RE: [edk2-devel] [PATCH] > > MinPlatformPkg/SecFspWrapperPlatformSecLib: Change > TopOfTemporaryRam > > type > > > > > > Hi Shenglei, > > > > Would you please elaborate a little on how casting to UINTN can > > resolve the overflow scenario and why 64bits OS will affect this code? >=20 > Actually casting to UINTN can't resolve the overflow. > What I mean is the result of (TopOfTemporaryRam - sizeof (UINT32)) may > overflow. > While it's meaningless to cast an already overflowed result to another t= ype. > So I update the code to cast the variable to UINT before it is arithmeti= cally > operated. >=20 > Under 64bits OS, the size of UINTN is 64-bit and the original > "(TopOfTemporaryRam - sizeof (UINT32)) " > is 32-bit. So the operation for casting will be performed. That's why 64= bits > OS will affect this code. >=20 > Thanks, > Shenglei >=20 > > > > Thanks! > > Chasel > > > > > -----Original Message----- > > > From: devel@edk2.groups.io On Behalf Of > > > Zhang, Shenglei > > > Sent: Monday, September 2, 2019 8:35 PM > > > To: devel@edk2.groups.io > > > Cc: Kubacki, Michael A ; Chiu, Chasel > > > ; Desimone, Nathaniel L > > > ; Gao, Liming > > > Subject: [edk2-devel] [PATCH] > > MinPlatformPkg/SecFspWrapperPlatformSecLib: > > > Change TopOfTemporaryRam type > > > > > > Cast TopOfTemporaryRam's from UINT32 to UINTN in the expression. > > > The original code (TopOfTemporaryRam - sizeof (UINT32)) may cause > > > overflow. As a result the operation under 64-bit OS environment, > > (UINT)(...), > > > may cast a overflowed 4-byte result to 8-byte one. > > > > > > Cc: Michael Kubacki > > > Cc: Chasel Chiu > > > Cc: Nate DeSimone > > > Cc: Liming Gao > > > Signed-off-by: Shenglei Zhang > > > --- > > > .../Library/SecFspWrapperPlatformSecLib/SecGetPerformance.c | 2 > +- > > > .../SecFspWrapperPlatformSecLib/SecPlatformInformation.c | 2 > +- > > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git > > > > > a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf > > or > > > mSecLib/SecGetPerformance.c > > > > > b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf > > or > > > mSecLib/SecGetPerformance.c > > > index c4eeb2b1..0cc42f96 100644 > > > --- > > > > > a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf > > or > > > mSecLib/SecGetPerformance.c > > > +++ > > > > b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlat > > > +++ formSecLib/SecGetPerformance.c > > > @@ -79,7 +79,7 @@ SecGetPerformance ( > > > // > > > TopOfTemporaryRam =3D (UINT32)(UINTN)TopOfTemporaryRamPpi - > > > sizeof(UINT32); > > > TopOfTemporaryRam -=3D sizeof(UINT32) * 2; > > > - Count =3D *(UINT32 *) (UINTN) (TopOfTemporaryRam - > sizeof > > > (UINT32)); > > > + Count =3D *(UINT32 *)((UINTN)TopOfTemporaryRam - > sizeof > > > (UINT32)); > > > Size =3D Count * sizeof (UINT32); > > > > > > Ticker =3D *(UINT64 *) (UINTN) (TopOfTemporaryRam - sizeof (UINT3= 2) > > > - > > Size > > > - sizeof (UINT32) * 2); diff --git > > > > > a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf > > or > > > mSecLib/SecPlatformInformation.c > > > > > b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf > > or > > > mSecLib/SecPlatformInformation.c > > > index 5b94ed2b..1bcee5f4 100644 > > > --- > > > > > a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf > > or > > > mSecLib/SecPlatformInformation.c > > > +++ > > > > b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlat > > > +++ formSecLib/SecPlatformInformation.c > > > @@ -61,7 +61,7 @@ SecPlatformInformation ( > > > // > > > TopOfTemporaryRam =3D (UINT32)(UINTN)TopOfTemporaryRamPpi - > sizeof > > > (UINT32); > > > TopOfTemporaryRam -=3D sizeof(UINT32) * 2; > > > - Count =3D *((UINT32 *)(UINTN) (TopOfTemporaryRam - > sizeof > > > (UINT32))); > > > + Count =3D *((UINT32 *)((UINTN)TopOfTemporaryRam - > sizeof > > > (UINT32))); > > > Size =3D Count * sizeof (IA32_HANDOFF_STATUS); > > > > > > if ((*StructureSize) < (UINT64) Size) { > > > -- > > > 2.18.0.windows.1 > > > > > > > > >=20