From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f68.google.com (mail-wm1-f68.google.com [209.85.128.68]) by mx.groups.io with SMTP id smtpd.web12.15767.1583955410977419288 for ; Wed, 11 Mar 2020 12:36:51 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=ax4bhfTe; spf=pass (domain: nuviainc.com, ip: 209.85.128.68, mailfrom: leif@nuviainc.com) Received: by mail-wm1-f68.google.com with SMTP id a141so3470963wme.2 for ; Wed, 11 Mar 2020 12:36:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=ivFVmiloA1odPHjbVxHxXlFA5mbCozEb8rSK4uQJvTc=; b=ax4bhfTe4hjSxtNLKV88G9yktcab8xDKuG2gwZH9bOCUulrS6UX/mXY8wiQcHsAQgB HtJP4tyww1Y+bneTlQ3dimA0qdde+JJj114vQNNF62+zX7q3kRn4XWtV6a1pPMaWKOYN Y0+/EJWAPvgTMHoKQYUf3NLsuabyOMSGD+gq7Jsn6DKoExdl2MLTJqSQejXMI3aXz5Rh pWeERDf54DVf6em/5dcPZJ0B5QR/btwiB3MNZ5wmi5ienyBeMvZv8lG/cdgyTvGoOokU 2luzvOE70h+ry20rUtXbIP6Y9wn21mYQOVZ2Wk3RnOiT5egA46weqRRp1z9/PQu5Li9x hpfg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=ivFVmiloA1odPHjbVxHxXlFA5mbCozEb8rSK4uQJvTc=; b=Jweyc/jBt+G1XBHmEIWYHBs7gqvAFWHjn1nsq7lSO00X/K/m3wWRa2Y/jKB0FhNAzm r5y6EP5VsZmCH+pYpWnU+fyyvbB6iZHWGmqz67mCqLliN4ZTSKF53g1WK/X2p4lD2wVM O0WeIwcCzadus3U79XwjGfnzklyxbRo6S11OKU2ZvPYYPz/BYH6fQgY810grZvoiV3fn KWwbNh0sdL7uYPOe3O6A/FOugF6LaVFooW4eaA+g6V62VGY6Sd51ahJOVcDxQzYlbyaM Gk9JNv8RhJt1iLGtXKj5xhLkyw3NwSxSa01C9Duz6znLwZW/Os3d1myTFHb86XPTB1zD 3K3A== X-Gm-Message-State: ANhLgQ2kfYs+Oxubvbqo4tfl4XD9ke/1USuJyWx8YQ9iR6Wqgo1uKuZb VxHZPWDbRSa1NJQUzWaqgTSSDg== X-Google-Smtp-Source: ADFU+vuoZzl2ncM54zLYMm89bd7KFKlEgKcMVTz/ULmqn//RDZ24gsgqIFcJVYn9LpMsB3PSnZEltg== X-Received: by 2002:a1c:cc03:: with SMTP id h3mr344121wmb.20.1583955409341; Wed, 11 Mar 2020 12:36:49 -0700 (PDT) Return-Path: Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id z19sm9816376wma.41.2020.03.11.12.36.48 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 11 Mar 2020 12:36:48 -0700 (PDT) Date: Wed, 11 Mar 2020 19:36:46 +0000 From: "Leif Lindholm" To: Laszlo Ersek Cc: devel@edk2.groups.io, Ard Biesheuvel , Jordan Justen , Philippe =?iso-8859-1?Q?Mathieu-Daud=E9?= Subject: Re: [edk2-devel] [PATCH 5/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation Message-ID: <20200311193646.GY23627@bivouac.eciton.net> References: <20200310222739.26717-1-lersek@redhat.com> <20200310222739.26717-6-lersek@redhat.com> <20200311160040.GS23627@bivouac.eciton.net> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Wed, Mar 11, 2020 at 17:22:47 +0100, Laszlo Ersek wrote: > >> +STATIC EFI_MEMORY_TYPE_INFORMATION mDefaultMemoryTypeInformation[] = { > >> + { EfiACPIMemoryNVS, 0x004 }, > >> + { EfiACPIReclaimMemory, 0x008 }, > >> + { EfiReservedMemoryType, 0x004 }, > >> + { EfiRuntimeServicesData, 0x024 }, > >> + { EfiRuntimeServicesCode, 0x030 }, > >> + { EfiBootServicesCode, 0x180 }, > >> + { EfiBootServicesData, 0xF00 }, > >> + { EfiMaxMemoryType, 0x000 } > >> +}; > > > > Could you add a comment as to where these page counts come from? > > Oh, right, they're just moved across from OvmfPkg/PlatformPei/Platform.c. > > > > It *would* be nice if that could be cleaned up at the same time... > > Sorry, I don't understand -- what kind of cleanup do you have in mind > precisely? The table is not copied, but moved from the original place, > so I'm unsure what's left in "Platform.c" to clean up. Not left to clean up there, but something to consider addressing when moving it here. Yes, it's just a move, so we could argue it doesn't need fixing - but it's a struct with a bunch of live-coded numerical values completely without explanation. "I'd rather not" is an acceptable answer, but I figured I should point it out. / Leif > Regarding the origin of the table, it's ancient: > > - 49ba9447c92d ("Add initial version of Open Virtual Machine Firmware > (OVMF) platform.", 2009-05-27) > - 991d95636264 ("Update OVMF Platform PEI to declare IO and MMIO > resources. Update default memory type information to > reduce EFI Memory Map fragmentation.", 2010-07-16) > - 55cdb67acb3d ("OVMF: Support greater than 2GB of memory", 2010-10-13) > > (BTW I have a patch as a separate work item in the queue for this -- I'm > going to remove the EfiBootServicesCode / EfiBootServicesData entries > from this table, per Jiewen's recommendation / explanation in the "WSMT > bits" thread: > > https://edk2.groups.io/g/devel/message/55712 > http://mid.mail-archive.com/a5e71131-65dc-8b85-481a-d35011512987@redhat.com > > But that's a separate patch, for later.) > > Thank you! > Laszlo > > > > > / > > Leif > > > >> + > >> +STATIC > >> +VOID > >> +BuildMemTypeInfoHob ( > >> + VOID > >> + ) > >> +{ > >> + BuildGuidDataHob ( > >> + &gEfiMemoryTypeInformationGuid, > >> + mDefaultMemoryTypeInformation, > >> + sizeof mDefaultMemoryTypeInformation > >> + ); > >> + DEBUG ((DEBUG_INFO, "%a: default memory type information HOB built\n", > >> + __FUNCTION__)); > >> +} > >> + > >> +/** > >> + Notification function called when EFI_PEI_READ_ONLY_VARIABLE2_PPI becomes > >> + available. > >> + > >> + @param[in] PeiServices Indirect reference to the PEI Services Table. > >> + @param[in] NotifyDescriptor Address of the notification descriptor data > >> + structure. > >> + @param[in] Ppi Address of the PPI that was installed. > >> + > >> + @return Status of the notification. The status code returned from this > >> + function is ignored. > >> +**/ > >> +STATIC > >> +EFI_STATUS > >> +EFIAPI > >> +OnReadOnlyVariable2Available ( > >> + IN EFI_PEI_SERVICES **PeiServices, > >> + IN EFI_PEI_NOTIFY_DESCRIPTOR *NotifyDescriptor, > >> + IN VOID *Ppi > >> + ) > >> +{ > >> + EFI_PEI_READ_ONLY_VARIABLE2_PPI *ReadOnlyVariable2; > >> + UINTN DataSize; > >> + EFI_STATUS Status; > >> + > >> + DEBUG ((DEBUG_VERBOSE, "%a\n", __FUNCTION__)); > >> + > >> + // > >> + // Check if the "MemoryTypeInformation" variable exists, in the > >> + // gEfiMemoryTypeInformationGuid namespace. > >> + // > >> + ReadOnlyVariable2 = Ppi; > >> + DataSize = 0; > >> + Status = ReadOnlyVariable2->GetVariable ( > >> + ReadOnlyVariable2, > >> + EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME, > >> + &gEfiMemoryTypeInformationGuid, > >> + NULL, > >> + &DataSize, > >> + NULL > >> + ); > >> + switch (Status) { > >> + case EFI_BUFFER_TOO_SMALL: > >> + // > >> + // The variable exists; the DXE IPL PEIM will build the HOB from it. > >> + // > >> + break; > >> + case EFI_NOT_FOUND: > >> + // > >> + // The variable does not exist; install the default memory type information > >> + // HOB. > >> + // > >> + BuildMemTypeInfoHob (); > >> + break; > >> + default: > >> + DEBUG ((DEBUG_ERROR, "%a: unexpected: GetVariable(): %r\n", __FUNCTION__, > >> + Status)); > >> + ASSERT (FALSE); > >> + CpuDeadLoop (); > >> + break; > >> + } > >> + > >> + return EFI_SUCCESS; > >> +} > >> + > >> +// > >> +// Notification object for registering the callback, for when > >> +// EFI_PEI_READ_ONLY_VARIABLE2_PPI becomes available. > >> +// > >> +STATIC CONST EFI_PEI_NOTIFY_DESCRIPTOR mReadOnlyVariable2Notify = { > >> + (EFI_PEI_PPI_DESCRIPTOR_NOTIFY_DISPATCH | > >> + EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST), // Flags > >> + &gEfiPeiReadOnlyVariable2PpiGuid, // Guid > >> + OnReadOnlyVariable2Available // Notify > >> +}; > >> + > >> +VOID > >> +MemTypeInfoInitialization ( > >> + VOID > >> + ) > >> +{ > >> + EFI_STATUS Status; > >> + > >> + if (!FeaturePcdGet (PcdSmmSmramRequire)) { > >> + // > >> + // EFI_PEI_READ_ONLY_VARIABLE2_PPI will never be available; install > >> + // the default memory type information HOB right away. > >> + // > >> + BuildMemTypeInfoHob (); > >> + return; > >> + } > >> + > >> + Status = PeiServicesNotifyPpi (&mReadOnlyVariable2Notify); > >> + if (EFI_ERROR (Status)) { > >> + DEBUG ((DEBUG_ERROR, "%a: failed to set up R/O Variable 2 callback: %r\n", > >> + __FUNCTION__, Status)); > >> + ASSERT (FALSE); > >> + CpuDeadLoop (); > >> + } > >> +} > >> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c > >> index 473af102783a..587ca68fc210 100644 > >> --- a/OvmfPkg/PlatformPei/Platform.c > >> +++ b/OvmfPkg/PlatformPei/Platform.c > >> @@ -28,7 +28,6 @@ > >> #include > >> #include > >> #include > >> -#include > >> #include > >> #include > >> #include > >> @@ -39,18 +38,6 @@ > >> #include "Platform.h" > >> #include "Cmos.h" > >> > >> -EFI_MEMORY_TYPE_INFORMATION mDefaultMemoryTypeInformation[] = { > >> - { EfiACPIMemoryNVS, 0x004 }, > >> - { EfiACPIReclaimMemory, 0x008 }, > >> - { EfiReservedMemoryType, 0x004 }, > >> - { EfiRuntimeServicesData, 0x024 }, > >> - { EfiRuntimeServicesCode, 0x030 }, > >> - { EfiBootServicesCode, 0x180 }, > >> - { EfiBootServicesData, 0xF00 }, > >> - { EfiMaxMemoryType, 0x000 } > >> -}; > >> - > >> - > >> EFI_PEI_PPI_DESCRIPTOR mPpiBootMode[] = { > >> { > >> EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST, > >> @@ -162,15 +149,6 @@ MemMapInitialization ( > >> PciIoBase = 0xC000; > >> PciIoSize = 0x4000; > >> > >> - // > >> - // Create Memory Type Information HOB > >> - // > >> - BuildGuidDataHob ( > >> - &gEfiMemoryTypeInformationGuid, > >> - mDefaultMemoryTypeInformation, > >> - sizeof(mDefaultMemoryTypeInformation) > >> - ); > >> - > >> // > >> // Video memory + Legacy BIOS region > >> // > >> @@ -811,6 +789,7 @@ InitializePlatform ( > >> ReserveEmuVariableNvStore (); > >> } > >> PeiFvInitialization (); > >> + MemTypeInfoInitialization (); > >> MemMapInitialization (); > >> NoexecDxeInitialization (); > >> } > >> -- > >> 2.19.1.3.g30247aa5d201 > >> > >> > >> > >> > > >