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

[Laszlo Ersek <lersek@redhat.com>]

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