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 CF877211799D6 for ; Wed, 17 Oct 2018 10:57:15 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 2E67B308123E; Wed, 17 Oct 2018 17:57:15 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-50.rdu2.redhat.com [10.10.120.50]) by smtp.corp.redhat.com (Postfix) with ESMTP id E0BEC60C69; Wed, 17 Oct 2018 17:57:13 +0000 (UTC) To: Jian J Wang , edk2-devel@lists.01.org Cc: Eric Dong , Ruiyu Ni References: <20181017083448.3436-1-jian.j.wang@intel.com> From: Laszlo Ersek Message-ID: Date: Wed, 17 Oct 2018 19:57:12 +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: <20181017083448.3436-1-jian.j.wang@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.49]); Wed, 17 Oct 2018 17:57:15 +0000 (UTC) Subject: Re: [PATCH] UefiCpuPkg/CpuExceptionHandlerLib: always clear descriptor data in advance 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, 17 Oct 2018 17:57:16 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Jian, On 10/17/18 10:34, Jian J Wang wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1237 > > Sometimes the memory will be contaminated by random data left in last > boot (warm reset). The code should not assume the allocated memory is > always filled with zero. This patch add code to clear data structure > used for stack switch to prevent such problem from happening. > > Cc: Eric Dong > Cc: Laszlo Ersek > Cc: Ruiyu Ni > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang > --- > UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c | 3 +++ > UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c | 3 +++ > 2 files changed, 6 insertions(+) > > diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c > index 031d0d35fa..eebd27a25d 100644 > --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c > +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c > @@ -214,6 +214,7 @@ ArchSetupExcpetionStack ( > // > TssBase = (UINTN)Tss; > > + TssDesc->Uint64 = 0; > TssDesc->Bits.LimitLow = sizeof(IA32_TASK_STATE_SEGMENT) - 1; > TssDesc->Bits.BaseLow = (UINT16)TssBase; > TssDesc->Bits.BaseMid = (UINT8)(TssBase >> 16); > @@ -238,6 +239,7 @@ ArchSetupExcpetionStack ( > // > TssBase = (UINTN)Tss; > > + TssDesc->Uint64 = 0; > TssDesc->Bits.LimitLow = sizeof(IA32_TASK_STATE_SEGMENT) - 1; > TssDesc->Bits.BaseLow = (UINT16)TssBase; > TssDesc->Bits.BaseMid = (UINT8)(TssBase >> 16); > @@ -255,6 +257,7 @@ ArchSetupExcpetionStack ( > continue; > } > > + SetMem (Tss, sizeof (IA32_TASK_STATE_SEGMENT), 0); > Tss->EIP = (UINT32)(TemplateMap.ExceptionStart > + Vector * TemplateMap.ExceptionStubHeaderSize); > Tss->EFLAGS = 0x2; > diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c > index 93ecf5ae5a..6745bc77c0 100644 > --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c > +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c > @@ -219,6 +219,8 @@ ArchSetupExcpetionStack ( > // > TssBase = (UINTN)Tss; > > + TssDesc->Uint128.Uint64 = 0; > + TssDesc->Uint128.Uint64_1= 0; > TssDesc->Bits.LimitLow = sizeof(IA32_TASK_STATE_SEGMENT) - 1; > TssDesc->Bits.BaseLow = (UINT16)TssBase; > TssDesc->Bits.BaseMidl = (UINT8)(TssBase >> 16); > @@ -231,6 +233,7 @@ ArchSetupExcpetionStack ( > // > // Fixup exception task descriptor and task-state segment > // > + SetMem (Tss, sizeof (IA32_TASK_STATE_SEGMENT), 0); > StackTop = StackSwitchData->X64.KnownGoodStackTop - CPU_STACK_ALIGNMENT; > StackTop = (UINTN)ALIGN_POINTER (StackTop, CPU_STACK_ALIGNMENT); > IdtTable = StackSwitchData->X64.IdtTable; > it can be checked whether this patch is complete (i.e. whether it covers all such places) and whether it is sound (i.e. what it does is correct). I can only offer to check the 2nd question. The patch seems correct, yes. However, I would like to suggest two style improvements: (1) Rather than SetMem (..., 0), I suggest ZeroMem(). (2) In general, I find ZeroMem (Tss, sizeof *Tss); easier to read than ZeroMem (Tss, sizeof (IA32_TASK_STATE_SEGMENT)); If you agree, feel free to update the code before pushing. (Do await feedback from Eric however.) With or without the updates: Reviewed-by: Laszlo Ersek Thanks Laszlo