* [PATCH] UefiCpuPkg/CpuMpPei: fix unsafe way to get stack pointer @ 2018-09-18 9:04 Jian J Wang 2018-09-18 16:41 ` Laszlo Ersek 2018-09-18 18:02 ` Jordan Justen 0 siblings, 2 replies; 10+ messages in thread From: Jian J Wang @ 2018-09-18 9:04 UTC (permalink / raw) To: edk2-devel; +Cc: Dandan Bi, Hao A Wu, Eric Dong, Laszlo Ersek 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] UefiCpuPkg/CpuMpPei: fix unsafe way to get stack pointer 2018-09-18 9:04 [PATCH] UefiCpuPkg/CpuMpPei: fix unsafe way to get stack pointer Jian J Wang @ 2018-09-18 16:41 ` Laszlo Ersek 2018-09-19 0:46 ` Wang, Jian J 2018-09-18 18:02 ` Jordan Justen 1 sibling, 1 reply; 10+ messages in thread From: Laszlo Ersek @ 2018-09-18 16:41 UTC (permalink / raw) To: Jian J Wang, edk2-devel Cc: Dandan Bi, Hao A Wu, Eric Dong, Jordan Justen (Intel address), Ard Biesheuvel, Gao, Liming, Michael Kinney 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] UefiCpuPkg/CpuMpPei: fix unsafe way to get stack pointer 2018-09-18 16:41 ` Laszlo Ersek @ 2018-09-19 0:46 ` Wang, Jian J 0 siblings, 0 replies; 10+ messages in thread From: Wang, Jian J @ 2018-09-19 0:46 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel@lists.01.org Cc: Bi, Dandan, Wu, Hao A, Dong, Eric, Justen, Jordan L, Ard Biesheuvel, Gao, Liming, Kinney, Michael D 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] UefiCpuPkg/CpuMpPei: fix unsafe way to get stack pointer 2018-09-18 9:04 [PATCH] UefiCpuPkg/CpuMpPei: fix unsafe way to get stack pointer Jian J Wang 2018-09-18 16:41 ` Laszlo Ersek @ 2018-09-18 18:02 ` Jordan Justen 2018-09-19 1:12 ` Wang, Jian J 2018-09-19 11:17 ` Laszlo Ersek 1 sibling, 2 replies; 10+ messages in thread From: Jordan Justen @ 2018-09-18 18:02 UTC (permalink / raw) To: Jian J Wang, edk2-devel Cc: Hao A Wu, Dandan Bi, Laszlo Ersek, Eric Dong, Michael Kinney 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? 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] UefiCpuPkg/CpuMpPei: fix unsafe way to get stack pointer 2018-09-18 18:02 ` Jordan Justen @ 2018-09-19 1:12 ` Wang, Jian J 2018-09-19 1:21 ` Wu, Hao A 2018-09-19 11:17 ` Laszlo Ersek 1 sibling, 1 reply; 10+ messages in thread From: Wang, Jian J @ 2018-09-19 1:12 UTC (permalink / raw) To: Justen, Jordan L, edk2-devel@lists.01.org Cc: Wu, Hao A, Bi, Dandan, Laszlo Ersek, Dong, Eric, Kinney, Michael D Jordan, No, it didn't help. But thanks for the help before. And I saw there's another guy having the same issue as mine. I tend to believe it's outlook (client/server) issue now. I think you're right about the "unsafe". The question is it's almost impossible for the static code checker to know how you use it. So it tend to give warning anyway. Hao, do you have comment on adding the exception? Regards, Jian > -----Original Message----- > From: Justen, Jordan L > Sent: Wednesday, September 19, 2018 2:03 AM > To: 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>; > Laszlo Ersek <lersek@redhat.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 > > 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? > > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] UefiCpuPkg/CpuMpPei: fix unsafe way to get stack pointer 2018-09-19 1:12 ` Wang, Jian J @ 2018-09-19 1:21 ` Wu, Hao A 0 siblings, 0 replies; 10+ messages in thread From: Wu, Hao A @ 2018-09-19 1:21 UTC (permalink / raw) To: Wang, Jian J, Justen, Jordan L, edk2-devel@lists.01.org Cc: Bi, Dandan, Laszlo Ersek, Dong, Eric, Kinney, Michael D > -----Original Message----- > From: Wang, Jian J > Sent: Wednesday, September 19, 2018 9:13 AM > To: Justen, Jordan L; edk2-devel@lists.01.org > Cc: Wu, Hao A; Bi, Dandan; Laszlo Ersek; Dong, Eric; Kinney, Michael D > Subject: RE: [edk2] [PATCH] UefiCpuPkg/CpuMpPei: fix unsafe way to get stack > pointer > > Jordan, > > No, it didn't help. But thanks for the help before. And I saw there's another guy > having the same issue as mine. I tend to believe it's outlook (client/server) issue > now. > > I think you're right about the "unsafe". The question is it's almost impossible for > the static code checker to know how you use it. So it tend to give warning > anyway. > > Hao, do you have comment on adding the exception? Hi, It is doable to tell the checker to ignore this issue. I think it will be better to also hear Mike's comment on this one. Best Regards, Hao Wu > > Regards, > Jian > > > > -----Original Message----- > > From: Justen, Jordan L > > Sent: Wednesday, September 19, 2018 2:03 AM > > To: 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>; > > Laszlo Ersek <lersek@redhat.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 > > > > 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? > > > > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] UefiCpuPkg/CpuMpPei: fix unsafe way to get stack pointer 2018-09-18 18:02 ` Jordan Justen 2018-09-19 1:12 ` Wang, Jian J @ 2018-09-19 11:17 ` Laszlo Ersek 2018-09-26 2:18 ` Wang, Jian J 1 sibling, 1 reply; 10+ messages in thread From: Laszlo Ersek @ 2018-09-19 11:17 UTC (permalink / raw) To: Jordan Justen, Jian J Wang, edk2-devel Cc: Hao A Wu, Dandan Bi, Eric Dong, Michael Kinney 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] UefiCpuPkg/CpuMpPei: fix unsafe way to get stack pointer 2018-09-19 11:17 ` Laszlo Ersek @ 2018-09-26 2:18 ` Wang, Jian J 2018-09-26 8:30 ` Laszlo Ersek 0 siblings, 1 reply; 10+ messages in thread From: Wang, Jian J @ 2018-09-26 2:18 UTC (permalink / raw) To: Laszlo Ersek, Justen, Jordan L, edk2-devel@lists.01.org Cc: Wu, Hao A, Bi, Dandan, Dong, Eric, Kinney, Michael D 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. Regards, Jian > -----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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] UefiCpuPkg/CpuMpPei: fix unsafe way to get stack pointer 2018-09-26 2:18 ` Wang, Jian J @ 2018-09-26 8:30 ` Laszlo Ersek 2018-09-26 8:54 ` Wang, Jian J 0 siblings, 1 reply; 10+ messages in thread From: Laszlo Ersek @ 2018-09-26 8:30 UTC (permalink / raw) To: Wang, Jian J, Justen, Jordan L, edk2-devel@lists.01.org Cc: Wu, Hao A, Bi, Dandan, Dong, Eric, Kinney, Michael D 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] UefiCpuPkg/CpuMpPei: fix unsafe way to get stack pointer 2018-09-26 8:30 ` Laszlo Ersek @ 2018-09-26 8:54 ` Wang, Jian J 0 siblings, 0 replies; 10+ messages in thread From: Wang, Jian J @ 2018-09-26 8:54 UTC (permalink / raw) To: Laszlo Ersek, Justen, Jordan L, edk2-devel@lists.01.org, Kinney, Michael D, Gao, Liming Cc: Wu, Hao A, Bi, Dandan, Dong, Eric Laszlo, You mean to add a specific interface to get stack pointer, right? I think it work for me too. But I guess there might be discussions before on such thing. Since it's not there, there might be some reasons not to do it. Mike and Liming, do you have any comments on this? Regards, Jian > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Wednesday, September 26, 2018 4:30 PM > To: Wang, Jian J <jian.j.wang@intel.com>; Justen, Jordan L > <jordan.l.justen@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/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 > > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-09-26 8:55 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-09-18 9:04 [PATCH] UefiCpuPkg/CpuMpPei: fix unsafe way to get stack pointer Jian J Wang 2018-09-18 16:41 ` Laszlo Ersek 2018-09-19 0:46 ` Wang, Jian J 2018-09-18 18:02 ` Jordan Justen 2018-09-19 1:12 ` Wang, Jian J 2018-09-19 1:21 ` Wu, Hao A 2018-09-19 11:17 ` Laszlo Ersek 2018-09-26 2:18 ` Wang, Jian J 2018-09-26 8:30 ` Laszlo Ersek 2018-09-26 8:54 ` Wang, Jian J
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox