Re: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Fix volatile variable RT cache update logic

["Wang, Jian J" <jian.j.wang@intel.com>]

I reproduced this issue in secure boot. The patch fixed the problem.

One minor issue in the patch: you changed the code from if...else... to ...if...,
which confused me a little bit. I thought you wanted to use 
'VolatileCacheInstance' in the 'if' expression but you didn't. So I suggest to
keep original style, i.e. put the first fetch of 'VolatileCacheInstance' in the 'else'
statement (it'd be better to go to the if statement to keep the same order as
original code, if it won't cause too complex if expression). From the reviewer
perspective, this way make it clearer to know you just want to update the
condition to determine the non-volatile variable update.

With this addressed (no v2 needed),

   Reviewed-by: Jian J Wang <jian.j.wang@intel.com>

Regards,
Jian

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Kubacki,
> Michael A
> Sent: Monday, November 11, 2019 1:16 PM
> To: devel@edk2.groups.io
> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao
> A <hao.a.wu@intel.com>
> Subject: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Fix volatile
> variable RT cache update logic
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2333
> 
> During a SetVariable () invocation, UpdateVariable () is called.
> UpdateVariable () contains logic to determine whether a volatile or
> non-volatile UEFI variable was set so the corresponding runtime
> cache can be updated to reflect the change. The current logic simply
> evaluates Variable->Volatile to determine which runtime cache should
> be updated.
> 
> The problem is Variable->Volatile does not always reflect whether a
> volatile variable is being set. Variable->Volatile is set to TRUE
> only in the case a pre-existing variable is found in the volatile
> variable store. Therefore, the value is FALSE when a new volatile
> variable is written.
> 
> This change updates the logic to take this into account. If a new
> variable is written successfully, the Attributes will accurately
> reflect whether the variable is non-volatile. If a pre-existing
> variable is modified, the Volatile field will reflect the type of
> variable (Attributes are not reliable; e.g. 0x0 indicates deletion).
> 
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Signed-off-by: Michael Kubacki <michael.a.kubacki@intel.com>
> ---
>  MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> index 29d6aca993..75d33ff724 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> @@ -2296,9 +2296,8 @@ UpdateVariable (
> 
>  Done:
>    if (!EFI_ERROR (Status)) {
> -    if (Variable->Volatile) {
> -      VolatileCacheInstance = &(mVariableModuleGlobal-
> >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCache);
> -    } else {
> +    VolatileCacheInstance = &(mVariableModuleGlobal-
> >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCache);
> +    if ((Variable->CurrPtr != NULL && !Variable->Volatile) || (Attributes &
> EFI_VARIABLE_NON_VOLATILE) != 0) {
>        VolatileCacheInstance = &(mVariableModuleGlobal-
> >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache);
>      }
> 
> --
> 2.16.2.windows.1
> 
> 
>