From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (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 B0A9921F0DB28 for ; Thu, 29 Mar 2018 05:12:42 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 7F4A1BD9F; Thu, 29 Mar 2018 12:19:21 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-223.rdu2.redhat.com [10.10.120.223]) by smtp.corp.redhat.com (Postfix) with ESMTP id 70300100F2EE; Thu, 29 Mar 2018 12:19:20 +0000 (UTC) To: "Zeng, Star" , edk2-devel-01 Cc: "Dong, Eric" , "Ni, Ruiyu" References: <20180328202651.1478-1-lersek@redhat.com> <20180328202651.1478-2-lersek@redhat.com> <0C09AFA07DD0434D9E2A0C6AEB0483103BA7437D@shsmsx102.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: <3cebafb4-e6ec-cadf-6cf3-62fc60b8fa42@redhat.com> Date: Thu, 29 Mar 2018 14:19:19 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103BA7437D@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.3 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Thu, 29 Mar 2018 12:19:21 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Thu, 29 Mar 2018 12:19:21 +0000 (UTC) for IP:'10.11.54.3' DOMAIN:'int-mx03.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH 1/4] MdeModulePkg/Variable/RuntimeDxe: introduce PcdMaxVolatileVariableSize X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 29 Mar 2018 12:12:43 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 03/29/18 03:34, Zeng, Star wrote: > Laszlo, > > Good patch and I assume you have done some tests with the patch. :) Yes, I have. For example I re-ran the Secure Boot Logo Test (the automated one) from Microsoft HCK, using a "controller" VM (Windows Server 2008R2) and a "client" VM (Windows Server 2016). This test massages variables quite heavily. I've never used the UEFI SCT; if someone could help with that, that would be great. > One very minor comment. > > Do you think the MaxVolatileVariableSize field assignment is better to be in VariableCommonInitialize(), but not InitNonVolatileVariableStore()? > > mVariableModuleGlobal->MaxVolatileVariableSize = ((PcdGet32 (PcdMaxVolatileVariableSize) != 0) ? > PcdGet32 (PcdMaxVolatileVariableSize) : > mVariableModuleGlobal->MaxVariableSize > ); That's a good point! InitNonVolatileVariableStore() is not the right place, judged simply by the function name. I originally put it there for keeping the "size" PCDs close together, but there's likely a better spot. > For example, have it right before the comments in VariableCommonInitialize(). > > // > // Allocate memory for volatile variable store, note that there is a scratch space to store scratch data. > // OK, I'll do that in v2. Thanks! Laszlo > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Thursday, March 29, 2018 4:27 AM > To: edk2-devel-01 > Cc: Dong, Eric ; Ni, Ruiyu ; Zeng, Star > Subject: [PATCH 1/4] MdeModulePkg/Variable/RuntimeDxe: introduce PcdMaxVolatileVariableSize > > The variable driver doesn't distinguish "non-volatile non-authenticated" > variables from "volatile non-authenticated" variables, when checking individual variable sizes against the permitted maximum. > PcdMaxVariableSize covers both kinds. > > This prevents volatile non-authenticated variables from carrying large data between UEFI drivers, despite having no flash impact. One example is EFI_TLS_CA_CERTIFICATE_VARIABLE, which platforms might want to create as volatile on every boot: the certificate list can be several hundred KB in size. > > Introduce PcdMaxVolatileVariableSize to represent the limit on individual volatile non-authenticated variables. The default value is zero, which makes Variable/RuntimeDxe fall back to PcdMaxVariableSize (i.e. the current behavior). This is similar to the PcdMaxAuthVariableSize fallback. > > Whenever the size limit is enforced, consult MaxVolatileVariableSize as the last option, after checking > - MaxAuthVariableSize for VARIABLE_ATTRIBUTE_AT_AW, > - and MaxVariableSize for EFI_VARIABLE_NON_VOLATILE. > > EFI_VARIABLE_HARDWARE_ERROR_RECORD is always handled separately; it always takes priority over the three cases listed above. > > Introduce the GetMaxVariableSize() helper to consider PcdMaxVolatileVariableSize, in addition to GetNonVolatileMaxVariableSize(). GetNonVolatileMaxVariableSize() is currently called at three sites, and two of those need to start using > GetMaxVariableSize() instead: > - VariableServiceInitialize() [VariableSmm.c]: the SMM comms buffer must > accommodate all kinds of variables, > - VariableCommonInitialize() [Variable.c]: the preallocated scratch space > must also accommodate all kinds of variables, > - InitNonVolatileVariableStore() [Variable.c] can continue using > GetNonVolatileMaxVariableSize(). > > Don't modify the ReclaimForOS() function as it is specific to non-volatile variables and should ignore PcdMaxVolatileVariableSize. > > Cc: Eric Dong > Cc: Ruiyu Ni > Cc: Star Zeng > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Laszlo Ersek > --- > MdeModulePkg/MdeModulePkg.dec | 8 ++++ > MdeModulePkg/MdeModulePkg.uni | 8 ++++ > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf | 1 + > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf | 1 + > MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h | 12 +++++ > MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 50 +++++++++++++++++--- > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c | 2 +- > 7 files changed, 74 insertions(+), 8 deletions(-) > > diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec index 5d561ff48495..cc397185f7b9 100644 > --- a/MdeModulePkg/MdeModulePkg.dec > +++ b/MdeModulePkg/MdeModulePkg.dec > @@ -1043,6 +1043,14 @@ [PcdsFixedAtBuild, PcdsPatchableInModule] > # @Prompt Maximum authenticated variable size. > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x00|UINT32|0x30000009 > > + ## The maximum size of a single non-authenticated volatile variable. > + # The default value is 0 for compatibility: in that case, the maximum > + # non-authenticated volatile variable size remains specified by # > + PcdMaxVariableSize. Only the > + MdeModulePkg/Universal/Variable/RuntimeDxe > + # driver supports this PCD. > + # @Prompt Maximum non-authenticated volatile variable size. > + > + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize|0x00|UINT32| > + 0x3000000a > + > ## The maximum size of single hardware error record variable.

> # In IA32/X64 platforms, this value should be larger than 1KB.
> # In IA64 platforms, this value should be larger than 128KB.
diff --git a/MdeModulePkg/MdeModulePkg.uni b/MdeModulePkg/MdeModulePkg.uni index f3fa616438b0..080b8a62c045 100644 > --- a/MdeModulePkg/MdeModulePkg.uni > +++ b/MdeModulePkg/MdeModulePkg.uni > @@ -94,6 +94,14 @@ > #string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdMaxAuthVariableSize_HELP #language en-US "The maximum size of a single authenticated variable." > "The value is 0 as default for compatibility that maximum authenticated variable size is specified by PcdMaxVariableSize." > > +#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdMaxVolatileVariableSize_PROMPT #language en-US "The maximum size of a single non-authenticated volatile variable." > + > +#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdMaxVolatileVariableSize_HELP #language en-US "The maximum size of a single non-authenticated volatile variable.

\n" > + "The default value is 0 for compatibility: in that case, the maximum " > + "non-authenticated volatile variable size remains specified by " > + "PcdMaxVariableSize.
\n" > + "Only the MdeModulePkg/Universal/Variable/RuntimeDxe driver supports this PCD.
" > + > #string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdMaxHardwareErrorVariableSize_PROMPT #language en-US "Maximum HwErr variable size" > > #string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdMaxHardwareErrorVariableSize_HELP #language en-US "The maximum size of single hardware error record variable.

\n" > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf > index e840fc9bff40..2d0a172ece35 100644 > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf > @@ -123,6 +123,7 @@ [Pcd] > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 ## CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize ## CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize ## CONSUMES > + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize ## CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxHardwareErrorVariableSize ## CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize ## CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdHwErrStorageSize ## CONSUMES > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf > index 69966f0d37ee..dbb0674a46ad 100644 > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf > @@ -125,6 +125,7 @@ [Pcd] > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 ## CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize ## CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize ## CONSUMES > + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize ## CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxHardwareErrorVariableSize ## CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize ## CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdHwErrStorageSize ## CONSUMES > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h > index b35e8ab91273..938eb5de61fa 100644 > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h > @@ -101,6 +101,7 @@ typedef struct { > UINTN HwErrVariableTotalSize; > UINTN MaxVariableSize; > UINTN MaxAuthVariableSize; > + UINTN MaxVolatileVariableSize; > UINTN ScratchBufferSize; > CHAR8 *PlatformLangCodes; > CHAR8 *LangCodes; > @@ -460,6 +461,17 @@ GetNonVolatileMaxVariableSize ( > VOID > ); > > +/** > + Get maximum variable size, covering both non-volatile and volatile variables. > + > + @return Maximum variable size. > + > +**/ > +UINTN > +GetMaxVariableSize ( > + VOID > + ); > + > /** > Initializes variable write service after FVB was ready. > > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c > index c11842b5c3ba..5a9051648004 100644 > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c > @@ -2345,12 +2345,14 @@ UpdateVariable ( > CopyMem (BufferForMerge, (UINT8 *) ((UINTN) CacheVariable->CurrPtr + DataOffset), DataSizeOfVariable (CacheVariable->CurrPtr)); > > // > - // Set Max Common/Auth Variable Data Size as default MaxDataSize. > + // Set Max Auth/Non-Volatile/Volatile Variable Data Size as default MaxDataSize. > // > if ((Attributes & VARIABLE_ATTRIBUTE_AT_AW) != 0) { > MaxDataSize = mVariableModuleGlobal->MaxAuthVariableSize - DataOffset; > - } else { > + } else if ((Attributes & EFI_VARIABLE_NON_VOLATILE) != 0) { > MaxDataSize = mVariableModuleGlobal->MaxVariableSize - DataOffset; > + } else { > + MaxDataSize = mVariableModuleGlobal->MaxVolatileVariableSize > + - DataOffset; > } > > // > @@ -3218,16 +3220,20 @@ VariableServiceSetVariable ( > } else { > // > // The size of the VariableName, including the Unicode Null in bytes plus > - // the DataSize is limited to maximum size of Max(Auth)VariableSize bytes. > + // the DataSize is limited to maximum size of Max(Auth|Volatile)VariableSize bytes. > // > if ((Attributes & VARIABLE_ATTRIBUTE_AT_AW) != 0) { > if (StrSize (VariableName) + PayloadSize > mVariableModuleGlobal->MaxAuthVariableSize - GetVariableHeaderSize ()) { > return EFI_INVALID_PARAMETER; > } > - } else { > + } else if ((Attributes & EFI_VARIABLE_NON_VOLATILE) != 0) { > if (StrSize (VariableName) + PayloadSize > mVariableModuleGlobal->MaxVariableSize - GetVariableHeaderSize ()) { > return EFI_INVALID_PARAMETER; > } > + } else { > + if (StrSize (VariableName) + PayloadSize > mVariableModuleGlobal->MaxVolatileVariableSize - GetVariableHeaderSize ()) { > + return EFI_INVALID_PARAMETER; > + } > } > } > > @@ -3399,12 +3405,14 @@ VariableServiceQueryVariableInfoInternal ( > } > > // > - // Let *MaximumVariableSize be Max(Auth)VariableSize with the exception of the variable header size. > + // Let *MaximumVariableSize be Max(Auth|Volatile)VariableSize with the exception of the variable header size. > // > if ((Attributes & VARIABLE_ATTRIBUTE_AT_AW) != 0) { > *MaximumVariableSize = mVariableModuleGlobal->MaxAuthVariableSize - GetVariableHeaderSize (); > - } else { > + } else if ((Attributes & EFI_VARIABLE_NON_VOLATILE) != 0) { > *MaximumVariableSize = mVariableModuleGlobal->MaxVariableSize - GetVariableHeaderSize (); > + } else { > + *MaximumVariableSize = > + mVariableModuleGlobal->MaxVolatileVariableSize - GetVariableHeaderSize > + (); > } > } > > @@ -3657,6 +3665,30 @@ GetNonVolatileMaxVariableSize ( > } > } > > +/** > + Get maximum variable size, covering both non-volatile and volatile variables. > + > + @return Maximum variable size. > + > +**/ > +UINTN > +GetMaxVariableSize ( > + VOID > + ) > +{ > + UINTN MaxVariableSize; > + > + MaxVariableSize = GetNonVolatileMaxVariableSize(); > + // > + // The condition below fails implicitly if PcdMaxVolatileVariableSize > +equals > + // the default zero value. > + // > + if (MaxVariableSize < PcdGet32 (PcdMaxVolatileVariableSize)) { > + MaxVariableSize = PcdGet32 (PcdMaxVolatileVariableSize); > + } > + return MaxVariableSize; > +} > + > /** > Init non-volatile variable store. > > @@ -3810,6 +3842,10 @@ InitNonVolatileVariableStore ( > > mVariableModuleGlobal->MaxVariableSize = PcdGet32 (PcdMaxVariableSize); > mVariableModuleGlobal->MaxAuthVariableSize = ((PcdGet32 (PcdMaxAuthVariableSize) != 0) ? PcdGet32 (PcdMaxAuthVariableSize) : mVariableModuleGlobal->MaxVariableSize); > + mVariableModuleGlobal->MaxVolatileVariableSize = ((PcdGet32 (PcdMaxVolatileVariableSize) != 0) ? > + PcdGet32 (PcdMaxVolatileVariableSize) : > + mVariableModuleGlobal->MaxVariableSize > + ); > > // > // Parse non-volatile variable data and get last variable offset. > @@ -4228,7 +4264,7 @@ VariableCommonInitialize ( > // > // Allocate memory for volatile variable store, note that there is a scratch space to store scratch data. > // > - ScratchSize = GetNonVolatileMaxVariableSize (); > + ScratchSize = GetMaxVariableSize (); > mVariableModuleGlobal->ScratchBufferSize = ScratchSize; > VolatileVariableStore = AllocateRuntimePool (PcdGet32 (PcdVariableStoreSize) + ScratchSize); > if (VolatileVariableStore == NULL) { > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c > index 8d73b6edee51..e495d971a08b 100644 > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c > @@ -955,7 +955,7 @@ VariableServiceInitialize ( > ); > ASSERT_EFI_ERROR (Status); > > - mVariableBufferPayloadSize = GetNonVolatileMaxVariableSize () + > + mVariableBufferPayloadSize = GetMaxVariableSize () + > OFFSET_OF (SMM_VARIABLE_COMMUNICATE_VAR_CHECK_VARIABLE_PROPERTY, Name) - GetVariableHeaderSize (); > > Status = gSmst->SmmAllocatePool ( > -- > 2.14.1.3.gb7cf6e02401b > >