Re: [edk2-devel] [PATCH] MinPlatformPkg/SecFspWrapperPlatformSecLib: Change TopOfTemporaryRam type

["Zhang, Shenglei" <shenglei.zhang@intel.com>]

Hi chasel,

> -----Original Message-----
> From: Chiu, Chasel
> Sent: Wednesday, September 4, 2019 8:13 PM
> To: devel@edk2.groups.io; Zhang, Shenglei <shenglei.zhang@intel.com>
> Cc: Kubacki, Michael A <michael.a.kubacki@intel.com>; Desimone, Nathaniel
> L <nathaniel.l.desimone@intel.com>; Gao, Liming <liming.gao@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.kubacki@intel.com>; Chiu, Chasel
> > <chasel.chiu@intel.com>; Desimone, Nathaniel L
> > <nathaniel.l.desimone@intel.com>; Gao, Liming <liming.gao@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.kubacki@intel.com>
> > Cc: Chasel Chiu <chasel.chiu@intel.com>
> > Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Signed-off-by: Shenglei Zhang <shenglei.zhang@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
> >
> >
> >