On 07/28/15 08:05, Fan, Jeff wrote: > Ersek, > > I have one comment for PCD PcdCpuSyncMtrrToAcpiNvs. > > I knew OvmfPkg implemented LockBox based on ACPI NVS.
That is true for OVMF at the moment, but this series is exactly the one changing it. Going forward, if you build OVMF with -D SMM_REQUIRE, then the custom LockBox implementation will not be used; the edk2-standard SMRAM-based lockbox will be used instead. (Where SMRAM protection will be enforced by (virtual) hardware.) > Saving MTRR > setting in AcpiNVS is OK for OvmfPkg. > But other platform may want to use more safe solution to save MTRR > based on in SMM. MTRR settings are not saved in AcpiNVS for the long term (ie. AcpiNVS is not trusted to persist intact from normal boot to S3 suspend). These patches only ensure that the CPU driver maintains an up-to-date copy of the MTRR settings in AcpiNVS until SMM-ready-to-lock is installed. Please look at the diagram in: [edk2] [PATCH 33/58] OvmfPkg: add skeleton QuarkPort/CpuS3DataDxe http://thread.gmane.org/gmane.comp.bios.edk2.devel/329/focus=357 and also the commit message in: [edk2] [PATCH 42/58] OvmfPkg: QuarkPort/CpuS3DataDxe: fill in ACPI_CPU_DATA.MtrrTable http://thread.gmane.org/gmane.comp.bios.edk2.devel/329/focus=365 This is the process: - UefiCpuPkg/CpuDxe maintains an up-to-date copy of the MTRR settings in AcpiNVS, and stores the address in PcdCpuMtrrTableAddress. - OvmfPkg/QuarkPort/CpuS3DataDxe reads PcdCpuMtrrTableAddress, and passes the address to OvmfPkg/QuarkPort/PiSmmCpuDxeSmm via another structure. - When SMM-ready-to-lock is installed during normal boot, the OvmfPkg/QuarkPort/PiSmmCpuDxeSmm driver fetches the then-most-recent MTRR settings from the AcpiNVS block (which has been kept up-to-date by UefiCpuPkg/CpuDxe), and saves the contents in SMRAM. - The OS is booted. The AcpiNVS block can be overwritten (corrupted, tampered-with, etc) by the runtime OS. - S3 suspend and resume occurs. The S3Resume PEIM transfers control to OvmfPkg/QuarkPort/PiSmmCpuDxeSmm via direct stack switching. The latter programs the MTRRs directly from the data residing in SMRAM. - etc. > I think that, for long term, saving MTRR setting by LockBox instead > of using ACPI NVS memory directly. Then, different platforms could > provide the different LockBox solutions. That could be possible, yes, but in this instance I followed the Quark package as closely as possible (I extracted / ported the absolutely necessary parts of the drivers). The communication channels between the separate drivers are usually the most vulnerable points from a security POV -- indeed that's where I found, and reported, a security issue to Intel --, so I definitely did not want to redesign those channels; I just verified them as much as I could, and ported them as intact as I could. And, at the moment, the Quark package follows the above pattern; it saves the MTRR settings in a dedicated SMRAM allocation, not under a LockBox GUID. > For short term, still saving MTRR setting in ACPI NVS in CpuDxe, and > removing this PCD. That means we could CpuDxe implementation to use > the long term solution in the future and needn't to remove one > un-used PCD more. As I said, the AcpiNVS block carries live (= authoritative) data only during normal boot, *before* unprivileged / untrusted code is launched: from UefiPkg/CpuDxe to OvmfPkg/QuarkPort/PiSmmCpuDxeSmm. The AcpiNVS block ceases being sensitive when SMM-ready-to-lock is installed. So, we have two PCDs here: - PcdCpuMtrrTableAddress: this is dynamic, and it is necessary for the communication channel I described above. It carries the address of the AcpiNVS block. - PcdCpuSyncMtrrToAcpiNvs: this is a feature PCD. Given that at the moment, OvmfPkg is the only platform in the open source edk2 tree that puts this new UefiCpuPkg functionality to use, I thought it prudent to allow other platforms to compile *out* the relevant code of CpuDxe. Or, at least, to enable them to opt out of the AcpiNVS memory allocation (which would be pure waste for them). Therefore I intend these UefiCpuPkg patches as long-term. Thanks! Laszlo > > Thanks! > Jeff > > -----Original Message----- > From: Laszlo Ersek [mailto:[email protected]] > Sent: Saturday, July 25, 2015 7:01 AM > To: [email protected] > Cc: Fan, Jeff; Chen Fan; Justen, Jordan L > Subject: [PATCH 38/58] UefiCpuPkg: CpuDxe: optionally save MTRR settings to > AcpiNVS memory block > > The > > Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/CpuArchDxe > > driver provides the following capability in its implementation of > EFI_CPU_ARCH_PROTOCOL: whenever the SetMemoryAttributes() member is used > (directly, or indirectly via gDS->SetMemorySpaceAttributes()) to change MTRR > settings, the complete set of settings is immediately reflected to an AcpiNVS > memory block. > > (The address of said block is published via a dynamic PCD.) > > This feature is necessary for supporting SMM-capable S3 resume, hence port it > to UefiCpuPkg. In IA32FamilyCpuBasePkg the feature is always built in; here > we make it controllable with a feature PCD. > > Cc: Jeff Fan <[email protected]> > Cc: Chen Fan <[email protected]> > Cc: Jordan Justen <[email protected]> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek <[email protected]> > --- > UefiCpuPkg/CpuDxe/CpuDxe.inf | 6 + > UefiCpuPkg/CpuDxe/MtrrSync.h | 86 ++++++++++++++ > UefiCpuPkg/CpuDxe/CpuDxe.c | 17 +++ > UefiCpuPkg/CpuDxe/MtrrSync.c | 118 ++++++++++++++++++++ > UefiCpuPkg/UefiCpuPkg.dec | 11 ++ > 5 files changed, 238 insertions(+) > > diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.inf b/UefiCpuPkg/CpuDxe/CpuDxe.inf > index a251922..2450a20 100644 > --- a/UefiCpuPkg/CpuDxe/CpuDxe.inf > +++ b/UefiCpuPkg/CpuDxe/CpuDxe.inf > @@ -54,6 +54,8 @@ [Sources] > CpuGdt.h > CpuMp.c > CpuMp.h > + MtrrSync.c > + MtrrSync.h > > [Sources.IA32] > Ia32/CpuAsm.asm | MSFT > @@ -86,6 +88,10 @@ [Ppis] > [Pcd] > gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ## CONSUMES > gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize ## CONSUMES > + gUefiCpuPkgTokenSpaceGuid.PcdCpuMtrrTableAddress ## > SOMETIMES_PRODUCES > + > +[FeaturePcd] > + gUefiCpuPkgTokenSpaceGuid.PcdCpuSyncMtrrToAcpiNvs > > [Depex] > TRUE > diff --git a/UefiCpuPkg/CpuDxe/MtrrSync.h b/UefiCpuPkg/CpuDxe/MtrrSync.h new > file mode 100644 index 0000000..1c20a32 > --- /dev/null > +++ b/UefiCpuPkg/CpuDxe/MtrrSync.h > @@ -0,0 +1,86 @@ > +/** @file > + > + Include file for MTRR synchronzation. > + > + Copyright (C) 2015, Red Hat, Inc. > + Copyright (c) 2013-2015 Intel Corporation. > + > + Redistribution and use in source and binary forms, with or without > + modification, are permitted provided that the following conditions > + are met: > + > + * Redistributions of source code must retain the above copyright > + notice, this list of conditions and the following disclaimer. > + * Redistributions in binary form must reproduce the above copyright > + notice, this list of conditions and the following disclaimer in the > + documentation and/or other materials provided with the distribution. > + * Neither the name of Intel Corporation nor the names of its > + contributors may be used to endorse or promote products derived from > + this software without specific prior written permission. > + > + THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS > + "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT > + LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR > + A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT > + OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, > + SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT > + LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, > + DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY > + THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > + (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE > + OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > + > + Module Name: MtrrSync.h > + > +**/ > + > +#ifndef _EFI_MTRR_SYNC_H_ > +#define _EFI_MTRR_SYNC_H_ > + > +#include <Library/MtrrLib.h> > + > +extern MTRR_SETTINGS *mMtrrTable; > + > +/** > + Initialize memory region for MTRR data. > + > + This function allocates ACPI NVS memory for MTRR data, and fills the > + region with current MTRR data. Each time MTRRs are written, this > + memory region will be updated accordingly. > + > +**/ > +VOID > +InitializeMtrrData ( > + VOID > + ); > + > +/** > + Synchronzies up the MTRR values with BSP for calling processor. > + > + This function synchronzies up the MTRR values with BSP for calling > processor. > + > + @param Buffer Mtrr table address. > + > +**/ > +VOID > +EFIAPI > +LoadMtrrData ( > + VOID *Buffer > + ); > + > +/** > + Allocate EfiACPIMemoryNVS below 4G memory address. > + > + This function allocates EfiACPIMemoryNVS below 4G memory address. > + > + @param Size Size of memory to allocate. > + > + @return Allocated address for output. > + > +**/ > +VOID* > +AllocateAcpiNvsMemoryBelow4G ( > + IN UINTN Size > + ); > +#endif > diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c index > c9df4e1..3d11807 100644 > --- a/UefiCpuPkg/CpuDxe/CpuDxe.c > +++ b/UefiCpuPkg/CpuDxe/CpuDxe.c > @@ -14,6 +14,7 @@ > > #include "CpuDxe.h" > #include "CpuMp.h" > +#include "MtrrSync.h" > > // > // Global Variables > @@ -405,6 +406,14 @@ CpuSetMemoryAttributes ( > CacheType > ); > > + if (!RETURN_ERROR (Status)) { > + if (FeaturePcdGet (PcdCpuSyncMtrrToAcpiNvs)) { > + // > + // Sync saved MTRR settings > + // > + MtrrGetAllMtrrs (mMtrrTable); > + } > + } > return (EFI_STATUS) Status; > } > > @@ -870,6 +879,14 @@ InitializeCpu ( > // > ProgramVirtualWireMode (); > > + if (FeaturePcdGet (PcdCpuSyncMtrrToAcpiNvs)) { > + // > + // Allocates ACPI NVS memory for MTRR data. > + // > + InitializeMtrrData (); > + MtrrGetAllMtrrs (mMtrrTable); > + } > + > // > // Install CPU Architectural Protocol > // > diff --git a/UefiCpuPkg/CpuDxe/MtrrSync.c b/UefiCpuPkg/CpuDxe/MtrrSync.c new > file mode 100644 index 0000000..8538014 > --- /dev/null > +++ b/UefiCpuPkg/CpuDxe/MtrrSync.c > @@ -0,0 +1,118 @@ > +/** @file > + > + Code for MTRR synchronzation. > + > + Copyright (C) 2015, Red Hat, Inc. > + Copyright (c) 2013-2015 Intel Corporation. > + > + Redistribution and use in source and binary forms, with or without > + modification, are permitted provided that the following conditions > + are met: > + > + * Redistributions of source code must retain the above copyright > + notice, this list of conditions and the following disclaimer. > + * Redistributions in binary form must reproduce the above copyright > + notice, this list of conditions and the following disclaimer in the > + documentation and/or other materials provided with the distribution. > + * Neither the name of Intel Corporation nor the names of its > + contributors may be used to endorse or promote products derived from > + this software without specific prior written permission. > + > + THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS > + "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT > + LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR > + A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT > + OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, > + SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT > + LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, > + DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY > + THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > + (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE > + OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > + > + Module Name: MtrrSync.c > + > +**/ > + > +#include <Library/UefiBootServicesTableLib.h> > +#include <Library/DebugLib.h> > +#include <Library/BaseMemoryLib.h> > + > +#include "MtrrSync.h" > + > +MTRR_SETTINGS *mMtrrTable; > + > +/** > + Initialize memory region for MTRR data. > + > + This function allocates ACPI NVS memory for MTRR data. > + Each time MTRRs are written, this memory region will be updated > accordingly. > + > +**/ > +VOID > +InitializeMtrrData ( > + VOID > + ) > +{ > + // > + // Allocate memory for fixed MTRRs, variable MTRRs and MTRR_DEF_TYPE > + // > + mMtrrTable = AllocateAcpiNvsMemoryBelow4G (sizeof (MTRR_SETTINGS)); > + > + PcdSet64 (PcdCpuMtrrTableAddress, (UINT64) (UINTN) mMtrrTable); } > + > +/** > + Synchronzies up the MTRR values with BSP for calling processor. > + > + This function synchronzies up the MTRR values with BSP for calling > processor. > + > + @param Buffer Mtrr table address. > + > +**/ > +VOID > +EFIAPI > +LoadMtrrData ( > + VOID *Buffer > + ) > +{ > + MtrrSetAllMtrrs (Buffer); > +} > + > +/** > + Allocate EfiACPIMemoryNVS below 4G memory address. > + > + This function allocates EfiACPIMemoryNVS below 4G memory address. > + > + @param Size Size of memory to allocate. > + > + @return Allocated address for output. > + > +**/ > +VOID* > +AllocateAcpiNvsMemoryBelow4G ( > + IN UINTN Size > + ) > +{ > + UINTN Pages; > + EFI_PHYSICAL_ADDRESS Address; > + EFI_STATUS Status; > + VOID* Buffer; > + > + Pages = EFI_SIZE_TO_PAGES (Size); > + Address = 0xffffffff; > + > + Status = gBS->AllocatePages ( > + AllocateMaxAddress, > + EfiACPIMemoryNVS, > + Pages, > + &Address > + ); > + ASSERT_EFI_ERROR (Status); > + > + Buffer = (VOID *) (UINTN) Address; > + ZeroMem (Buffer, Size); > + > + return Buffer; > +} > diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec index > 202e719..9c5da3c 100644 > --- a/UefiCpuPkg/UefiCpuPkg.dec > +++ b/UefiCpuPkg/UefiCpuPkg.dec > @@ -73,5 +73,16 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, > PcdsDynamicEx] > # @Prompt Microcode Region size. > > gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize|0x0|UINT64|0x00000006 > > +[PcdsFeatureFlag] > + ## This flag controls whether the EFI_CPU_ARCH_PROTOCOL > +implementation > + # reflects the updated MTRR settings to a persistent (AcpiNVS) memory > block. > + # @Prompt Keep MTRR settings reflected to an AcpiNVS memory block > + > +gUefiCpuPkgTokenSpaceGuid.PcdCpuSyncMtrrToAcpiNvs|FALSE|BOOLEAN|0x00000 > +007 > + > +[PcdsDynamic, PcdsDynamicEx] > + ## If PcdCpuSyncMtrrToAcpiNvs is TRUE, the PCD below will be set to > +the > + # address of the AcpiNVS memory block in question. > + > +gUefiCpuPkgTokenSpaceGuid.PcdCpuMtrrTableAddress|0x0|UINT64|0x00000008 > + > [UserExtensions.TianoCore."ExtraFiles"] > UefiCpuPkgExtra.uni > -- > 1.8.3.1 > > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
