From: Laszlo Ersek <lersek@redhat.com>
To: "Zeng, Star" <star.zeng@intel.com>,
edk2-devel-01 <edk2-devel@lists.01.org>
Cc: "Dong, Eric" <eric.dong@intel.com>, "Ni, Ruiyu" <ruiyu.ni@intel.com>
Subject: Re: [PATCH 1/4] MdeModulePkg/Variable/RuntimeDxe: introduce PcdMaxVolatileVariableSize
Date: Thu, 29 Mar 2018 14:19:19 +0200 [thread overview]
Message-ID: <3cebafb4-e6ec-cadf-6cf3-62fc60b8fa42@redhat.com> (raw)
In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103BA7437D@shsmsx102.ccr.corp.intel.com>
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 <edk2-devel@lists.01.org>
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
> 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 <eric.dong@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 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.<BR><BR>
> # In IA32/X64 platforms, this value should be larger than 1KB.<BR>
> # In IA64 platforms, this value should be larger than 128KB.<BR> 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.<BR><BR>\n"
> + "The default value is 0 for compatibility: in that case, the maximum "
> + "non-authenticated volatile variable size remains specified by "
> + "PcdMaxVariableSize.<BR>\n"
> + "Only the MdeModulePkg/Universal/Variable/RuntimeDxe driver supports this PCD.<BR>"
> +
> #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.<BR><BR>\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
>
>
next prev parent reply other threads:[~2018-03-29 12:12 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-28 20:26 [PATCH 0/4] MdeModulePkg, OvmfPkg: support large CA cert list for HTTPS boot Laszlo Ersek
2018-03-28 20:26 ` [PATCH 1/4] MdeModulePkg/Variable/RuntimeDxe: introduce PcdMaxVolatileVariableSize Laszlo Ersek
2018-03-29 1:34 ` Zeng, Star
2018-03-29 12:19 ` Laszlo Ersek [this message]
2018-03-30 0:54 ` Zeng, Star
2018-03-28 20:26 ` [PATCH 2/4] OvmfPkg/EmuVariableFvbRuntimeDxe: stop using PcdVariableStoreSize Laszlo Ersek
2018-03-30 10:57 ` Ard Biesheuvel
2018-03-28 20:26 ` [PATCH 3/4] OvmfPkg: annotate "PcdVariableStoreSize := PcdFlashNvStorageVariableSize" Laszlo Ersek
2018-03-30 10:58 ` Ard Biesheuvel
2018-03-28 20:26 ` [PATCH 4/4] OvmfPkg/TlsAuthConfigLib: configure trusted CA certs for HTTPS boot Laszlo Ersek
2018-03-30 11:00 ` Ard Biesheuvel
2018-03-29 4:56 ` [PATCH 0/4] MdeModulePkg, OvmfPkg: support large CA cert list " Palmer, Thomas
2018-03-29 11:57 ` Laszlo Ersek
2018-03-29 18:17 ` Palmer, Thomas
2018-03-30 4:39 ` Gary Lin
2018-03-30 19:43 ` Laszlo Ersek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3cebafb4-e6ec-cadf-6cf3-62fc60b8fa42@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox