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

[Laszlo Ersek <lersek@redhat.com>]

On 09/26/18 04:18, Wang, Jian J wrote:
> Hi,
> 
> Since the patch will introduce "#if defined(...)" macro in code, which violates
> edk2 coding style, it's suggested to add exception to static checker.
> 
> I'll wait for one or two days in case there's other suggestions. If no objection
> then, I'll withdraw this patch and close BZ#1186 as not-fix.

If we can *selectively* suppress this one warning from the static
checker (saying that "yeah we know what we are doing"), then I agree
WONTFIX is acceptable for the BZ. It's not optimal IMO (I think there
would be value in a generic facility for getting the stack pointer --
several places in edk2 want to know the stack pointer), but it is
acceptable. (As far as I'm concerned anyway.)

Thanks
Laszlo

>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Wednesday, September 19, 2018 7:17 PM
>> To: Justen, Jordan L <jordan.l.justen@intel.com>; Wang, Jian J
>> <jian.j.wang@intel.com>; edk2-devel@lists.01.org
>> Cc: Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>;
>> Dong, Eric <eric.dong@intel.com>; Kinney, Michael D
>> <michael.d.kinney@intel.com>
>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/CpuMpPei: fix unsafe way to get stack
>> pointer
>>
>> On 09/18/18 20:02, Jordan Justen wrote:
>>> I guess the git config sendemail.from setting did not help your
>>> patches. ?? It still is coming through with a From field of
>>> <edk2-devel-bounces@lists.01.org>.
>>>
>>> Regarding this patch, I suppose it is worth asking if &StackBase in
>>> the old code could possibly be an address not on the stack. I don't
>>> think it is possible, and I'm guessing the C specification would
>>> probably back that up.
>>>
>>> It can be unsafe to get an address of something on the stack and then
>>> refer to that address after the variable is no longer in scope. I
>>> suspect this is what the static checker is noticing. By calling
>>> SetJump, aren't we just doing the same thing, but hiding what we are
>>> doing from the static checker?
>>
>> Yep, we're totally doing that.
>>
>> Thanks,
>> Laszlo
>>
>>>
>>> So, can't we just tell the static checker to ignore the error because
>>> we know what we are doing?
>>>
>>> -Jordan
>>>
>>> On 2018-09-18 02:04:48,  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);
>>>> --
>>>> 2.16.2.windows.1
>>>>
>>>> _______________________________________________
>>>> edk2-devel mailing list
>>>> edk2-devel@lists.01.org
>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>