Re: [PATCH] UefiCpuPkg/CpuMpPei: fix unsafe way to get stack pointer

["Wang, Jian J" <jian.j.wang@intel.com>]

I know the rule. But I saw it used in CpuDxe/CpuGdt.h and I thought it could
have exceptions. Anyway, I agree keeping consistency is more important.

Regards,
Jian


> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, September 19, 2018 12:42 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Bi, Dandan <dandan.bi@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Dong, Eric <eric.dong@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Ard Biesheuvel <ard.biesheuvel@linaro.org>; Gao, Liming
> <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: Re: [PATCH] UefiCpuPkg/CpuMpPei: fix unsafe way to get stack pointer
> 
> Adding Jordan, Ard, Liming, Mike; comment at the bottom:
> 
> On 09/18/18 11:04, Jian J Wang wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1186
> >
> > This patch uses SetJump() to get the stack pointer from esp/rsp
> > register to replace local variable way, which was marked by static
> > code checker as an unsafe way.
> >
> > Cc: Dandan Bi <dandan.bi@intel.com>
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> > ---
> >  UefiCpuPkg/CpuMpPei/CpuMpPei.h  | 8 ++++++++
> >  UefiCpuPkg/CpuMpPei/CpuPaging.c | 9 +++++++--
> >  2 files changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.h
> b/UefiCpuPkg/CpuMpPei/CpuMpPei.h
> > index d097a66aa8..fe61f5e3bc 100644
> > --- a/UefiCpuPkg/CpuMpPei/CpuMpPei.h
> > +++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.h
> > @@ -35,6 +35,14 @@
> >
> >  extern EFI_PEI_PPI_DESCRIPTOR   mPeiCpuMpPpiDesc;
> >
> > +#if   defined (MDE_CPU_IA32)
> > +#define CPU_STACK_POINTER(Context)  ((Context).Esp)
> > +#elif defined (MDE_CPU_X64)
> > +#define CPU_STACK_POINTER(Context)  ((Context).Rsp)
> > +#else
> > +#error CPU type not supported!
> > +#endif
> > +
> >  /**
> >    This service retrieves the number of logical processor in the platform
> >    and the number of those logical processors that are enabled on this boot.
> > diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c
> b/UefiCpuPkg/CpuMpPei/CpuPaging.c
> > index c7e0822452..997c20c26e 100644
> > --- a/UefiCpuPkg/CpuMpPei/CpuPaging.c
> > +++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c
> > @@ -517,9 +517,14 @@ GetStackBase (
> >    IN OUT VOID *Buffer
> >    )
> >  {
> > -  EFI_PHYSICAL_ADDRESS    StackBase;
> > +  EFI_PHYSICAL_ADDRESS      StackBase;
> > +  BASE_LIBRARY_JUMP_BUFFER  Context;
> >
> > -  StackBase = (EFI_PHYSICAL_ADDRESS)(UINTN)&StackBase;
> > +  //
> > +  // Retrieve stack pointer from current processor context.
> > +  //
> > +  SetJump (&Context);
> > +  StackBase = (EFI_PHYSICAL_ADDRESS)CPU_STACK_POINTER (Context);
> >    StackBase += BASE_4KB;
> >    StackBase &= ~((EFI_PHYSICAL_ADDRESS)BASE_4KB - 1);
> >    StackBase -= PcdGet32(PcdCpuApStackSize);
> >
> 
> I think using SetJump() for this purpose, in contexts where library
> constructors have run, is a good idea.
> 
> What I like less is that we are open-coding this trick here, in
> CpuMpPei. Getting the stack pointer in C code is frequently necessary,
> and I would prefer an API addition to MdePkg's BaseLib, implemented for
> as many architectures as possible. One discussion that I recall about
> this is the sub-thread at
> <https://www.mail-archive.com/edk2-devel@lists.01.org/msg32216.html>.
> 
> If the MdePkg maintainers disagree with the BaseLib API addition, then
> the patch should still be improved, if possible. Mike said earlier that
> in C files we like to avoid MDE_CPU_* dependent-code, instead we extract
> the affected function(s) to architecture-dependent subdirectories, and
> use [Sources.<ARCH>] sections in the INF files. That suggests files like:
> 
> - UefiCpuPkg/CpuMpPei/Ia32/GetStackBase.c
> - UefiCpuPkg/CpuMpPei/X64/GetStackBase.c
> 
> here.
> 
> Possibly overkill, yes, but we should be consistent.
> 
> Thanks
> Laszlo