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 869DB2114B122 for ; Wed, 26 Sep 2018 01:30:10 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.25]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 2518B9E61F; Wed, 26 Sep 2018 08:30:10 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-54.rdu2.redhat.com [10.10.120.54]) by smtp.corp.redhat.com (Postfix) with ESMTP id 27BBE2010DBE; Wed, 26 Sep 2018 08:30:07 +0000 (UTC) To: "Wang, Jian J" , "Justen, Jordan L" , "edk2-devel@lists.01.org" Cc: "Wu, Hao A" , "Bi, Dandan" , "Dong, Eric" , "Kinney, Michael D" References: <20180918090448.7324-1-jian.j.wang@intel.com> <153729377116.1021.13677753557698494356@jljusten-skl> <47392a62-419a-45e2-6a09-5cec8b35cd55@redhat.com> From: Laszlo Ersek Message-ID: Date: Wed, 26 Sep 2018 10:30:07 +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: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.25 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Wed, 26 Sep 2018 08:30:10 +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: Wed, 26 Sep 2018 08:30:11 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 ; Wang, Jian J >> ; edk2-devel@lists.01.org >> Cc: Wu, Hao A ; Bi, Dandan ; >> Dong, Eric ; Kinney, Michael D >> >> 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 >>> . >>> >>> 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 >>>> 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); >>>> -- >>>> 2.16.2.windows.1 >>>> >>>> _______________________________________________ >>>> edk2-devel mailing list >>>> edk2-devel@lists.01.org >>>> https://lists.01.org/mailman/listinfo/edk2-devel >