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 153132034BBC8 for ; Wed, 8 Nov 2017 17:32:02 -0800 (PST) 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 DAFFC13A5F; Thu, 9 Nov 2017 01:36:03 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-142.rdu2.redhat.com [10.10.120.142]) by smtp.corp.redhat.com (Postfix) with ESMTP id 410D35D6AE; Thu, 9 Nov 2017 01:36:02 +0000 (UTC) To: Ruiyu Ni Cc: edk2-devel@lists.01.org, Michael D Kinney , Eric Dong , Ard Biesheuvel , "Jordan Justen (Intel address)" , Jiewen Yao References: <20171012084810.148196-1-ruiyu.ni@intel.com> <20171012084810.148196-4-ruiyu.ni@intel.com> From: Laszlo Ersek Message-ID: <03e369bb-77c4-0134-258f-bdae62cbc8c5@redhat.com> Date: Thu, 9 Nov 2017 02:36:01 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171012084810.148196-4-ruiyu.ni@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.29]); Thu, 09 Nov 2017 01:36:03 +0000 (UTC) Subject: Re: [PATCH 3/4] UefiCpuPkg/MtrrLib: Update algorithm to calculate optimal settings X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 09 Nov 2017 01:32:03 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Ray, On 10/12/17 10:48, Ruiyu Ni wrote: > The new algorithm converts the problem calculating optimal > MTRR settings (using least MTRR registers) to the problem finding > the shortest path in a graph. > The memory required in extreme but rare case can be up to 256KB, > so using local stack buffer is impossible considering current > DxeIpl only allocates 128KB stack. > > The patch changes existing MtrrSetMemoryAttributeInMtrrSettings() and > MtrrSetMemoryAttribute() to use the 4-page stack buffer for > calculation. The two APIs return BUFFER_TOO_SMALL when the buffer > is too small for calculation. [snip] > +#define SCRATCH_BUFFER_SIZE (4 * SIZE_4KB) [snip] > RETURN_STATUS > EFIAPI > -MtrrSetMemoryAttribute ( > +MtrrSetMemoryAttributeInMtrrSettings ( > + IN OUT MTRR_SETTINGS *MtrrSetting, > IN PHYSICAL_ADDRESS BaseAddress, > IN UINT64 Length, > IN MTRR_MEMORY_CACHE_TYPE Attribute > ) > { > RETURN_STATUS Status; > + UINT8 Scratch[SCRATCH_BUFFER_SIZE]; [snip] (This patch is now commit 2bbd7e2fbd4b.) Today I managed to spend time on https://bugzilla.tianocore.org/show_bug.cgi?id=747 (which is in turn based on the earlier mailing list thread [edk2] dynamic PCD impact on temporary PEI memory https://lists.01.org/pipermail/edk2-devel/2017-October/016213.html ). While writing the patches, I found the root cause of BZ#747: "OvmfPkg/PlatformPei" calls MtrrLib APIs, and due to the above 16KB stack allocation, MtrrLib overflow's OVMF's 16KB (total) temp SEC/PEI stack. Because the temp SEC/PEI heap is just below the stack, and the stack grows down, this overflow by to the large Scratch array corrupts the heap (for example, various HOBs). Now, I'm fixing this for OVMF by enlarging its temp SEC/PEI RAM (the patches are mostly ready for posting), but I have a different concern: MtrrLib is MP-safe, meaning that it can be called from APs as well, for setting up MTRRs on APs. (The Intel SDM basically requires all processors to use the same MTRR settings, which must be configured on all APs in parallel.) Hence it seems to me that the above Stack array (16KB in size) must fit into the stack of *each* AP. In particular I'm concerned about the UefiCpuPkg/PiSmmCpuDxeSmm driver. In OVMF we have seen SMM stack overflow before. The following two commits were added back then: - 509f8425b75d ("UefiCpuPkg: change PcdCpuSmmStackGuard default to TRUE", 2016-06-01) - 0d0c245dfb14 ("OvmfPkg: set SMM stack size to 16KB", 2016-06-01) However: the default SMM stack size (in "UefiCpuPkg.dec") remains 8KB (PcdCpuSmmStackSize). Furthermore, the guard page can only catch accesses that are *slightly* below the stack base address. If an overflow is several pages out of bounds, then the wrong access will skip over the guard page. The same worry might apply, via MpInitLib, and the PcdCpuApStackSize PCD, to: - UefiCpuPkg/CpuMpPei/CpuMpPei.inf (produces the MP services PPI), - UefiCpuPkg/CpuDxe/CpuDxe.inf (produces the MP services protocol). (Although the default value for PcdCpuApStackSize is larger: 32KB). Do we need to audit all the AP stacks to see if they can accommodate the Scratch array? Thanks Laszlo