From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 6889C21140820 for ; Tue, 18 Sep 2018 09:41:39 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D00AB5F74A; Tue, 18 Sep 2018 16:41:38 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-209.rdu2.redhat.com [10.10.120.209]) by smtp.corp.redhat.com (Postfix) with ESMTP id B3E8C176AF; Tue, 18 Sep 2018 16:41:36 +0000 (UTC) To: Jian J Wang , edk2-devel@lists.01.org Cc: Dandan Bi , Hao A Wu , Eric Dong , "Jordan Justen (Intel address)" , Ard Biesheuvel , "Gao, Liming" , Michael Kinney References: <20180918090448.7324-1-jian.j.wang@intel.com> From: Laszlo Ersek Message-ID: Date: Tue, 18 Sep 2018 18:41:35 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20180918090448.7324-1-jian.j.wang@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Tue, 18 Sep 2018 16:41:38 +0000 (UTC) Subject: Re: [PATCH] UefiCpuPkg/CpuMpPei: fix unsafe way to get stack pointer X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 18 Sep 2018 16:41:40 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Hao A Wu > Cc: Eric Dong > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang > --- > 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 . 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.] 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