Re: [PATCH 1/4] MdeModulePkg/Variable/RuntimeDxe: introduce PcdMaxVolatileVariableSize

["Zeng, Star" <star.zeng@intel.com>]

No V2 needed for this case, Reviewed-by: Star Zeng <star.zeng@intel.com> with my comment handled.


Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Thursday, March 29, 2018 8:19 PM
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

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|0x30
> 000009
>  
> +  ## 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|UINT3
> + 2|
> + 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.in
> +++ f
> @@ -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 - 
> + mVariableModuleGlobal->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
> 
>