Thanks for explanations.
So looks like the intention is to support X64 build scenarios, I think
TopOfTemporaryRam should be defined as UINTN from beginning so it can hold
64bits 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 <chasel.c...@intel.com>; devel@edk2.groups.io
> Cc: Kubacki, Michael A <michael.a.kuba...@intel.com>; Desimone, Nathaniel
> L <nathaniel.l.desim...@intel.com>; Gao, Liming <liming....@intel.com>
> Subject: RE: [edk2-devel] [PATCH]
> MinPlatformPkg/SecFspWrapperPlatformSecLib: Change
> TopOfTemporaryRam type
>
> Hi chasel,
>
> > -----Original Message-----
> > From: Chiu, Chasel
> > Sent: Wednesday, September 4, 2019 8:13 PM
> > To: devel@edk2.groups.io; Zhang, Shenglei <shenglei.zh...@intel.com>
> > Cc: Kubacki, Michael A <michael.a.kuba...@intel.com>; Desimone,
> > Nathaniel L <nathaniel.l.desim...@intel.com>; Gao, Liming
> > <liming....@intel.com>
> > 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?
>
> 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 type.
> So I update the code to cast the variable to UINT before it is arithmetically
> operated.
>
> 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 64bits
> OS will affect this code.
>
> Thanks,
> Shenglei
>
> >
> > Thanks!
> > Chasel
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <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 <michael.a.kuba...@intel.com>; Chiu, Chasel
> > > <chasel.c...@intel.com>; Desimone, Nathaniel L
> > > <nathaniel.l.desim...@intel.com>; Gao, Liming <liming....@intel.com>
> > > 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 <michael.a.kuba...@intel.com>
> > > Cc: Chasel Chiu <chasel.c...@intel.com>
> > > Cc: Nate DeSimone <nathaniel.l.desim...@intel.com>
> > > Cc: Liming Gao <liming....@intel.com>
> > > Signed-off-by: Shenglei Zhang <shenglei.zh...@intel.com>
> > > ---
> > > .../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 = (UINT32)(UINTN)TopOfTemporaryRamPpi -
> > > sizeof(UINT32);
> > > TopOfTemporaryRam -= sizeof(UINT32) * 2;
> > > - Count = *(UINT32 *) (UINTN) (TopOfTemporaryRam -
> sizeof
> > > (UINT32));
> > > + Count = *(UINT32 *)((UINTN)TopOfTemporaryRam -
> sizeof
> > > (UINT32));
> > > Size = Count * sizeof (UINT32);
> > >
> > > Ticker = *(UINT64 *) (UINTN) (TopOfTemporaryRam - sizeof (UINT32)
> > > -
> > 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 = (UINT32)(UINTN)TopOfTemporaryRamPpi -
> sizeof
> > > (UINT32);
> > > TopOfTemporaryRam -= sizeof(UINT32) * 2;
> > > - Count = *((UINT32 *)(UINTN) (TopOfTemporaryRam -
> sizeof
> > > (UINT32)));
> > > + Count = *((UINT32 *)((UINTN)TopOfTemporaryRam -
> sizeof
> > > (UINT32)));
> > > Size = Count * sizeof (IA32_HANDOFF_STATUS);
> > >
> > > if ((*StructureSize) < (UINT64) Size) {
> > > --
> > > 2.18.0.windows.1
> > >
> > >
> > >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#46860): https://edk2.groups.io/g/devel/message/46860
Mute This Topic: https://groups.io/mt/33110619/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
