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

[Laszlo Ersek <lersek@redhat.com>]

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