* [PATCH V1 1/1] MdeModulePkg/Variable: Fix VarErrorFlag RT cache offset calculation
@ 2020-01-13 23:19 Kubacki, Michael A
2020-01-14 6:42 ` [edk2-devel] " Wang, Jian J
0 siblings, 1 reply; 7+ messages in thread
From: Kubacki, Michael A @ 2020-01-13 23:19 UTC (permalink / raw)
To: devel; +Cc: Liming Gao, Michael D Kinney, Michael Turner, Jian J Wang,
Hao A Wu
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2457
This commit fixes an offset calculation that is used to write the
VarErrorFlag UEFI variable to the UEFI variable runtime cache.
Currently a physical address is used instead of an offset. This
commit changes the offset to zero with a length of the entire
non-volatile variable store so the entire non-volatile variable
store buffer in SMRAM (with the variable update modification) is
copied to the runtime variable cache. This follows the same pattern
used in other SynchronizeRuntimeVariableCache () calls for
consistency.
* Observable symptom: An exception in SMM will most likely occur
due to the invalid memory reference when the VarErrorFlag variable
is written. The variable is most commonly written when the UEFI
variable store is full.
* The issue only occurs when the variable runtime cache is enabled
by the following PCD being set to TRUE:
gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache
Fixes: aab3b9b9a1e5e1f3fa966fb1667fc3e6c47e7706
Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Michael Turner <michael.turner@microsoft.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 | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
index b0ee5e50d0..d23aea4bc7 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
@@ -16,7 +16,7 @@
VariableServiceSetVariable() should also check authenticate data to avoid buffer overflow,
integer overflow. It should also check attribute to avoid authentication bypass.
-Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2006 - 2020, Intel Corporation. All rights reserved.<BR>
(C) Copyright 2015-2018 Hewlett Packard Enterprise Development LP<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -335,8 +335,8 @@ RecordVarErrorFlag (
*VarErrFlag = TempFlag;
Status = SynchronizeRuntimeVariableCache (
&mVariableModuleGlobal->VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache,
- (UINTN) VarErrFlag - (UINTN) mNvVariableCache + (UINTN) mVariableModuleGlobal->VariableGlobal.NonVolatileVariableBase,
- sizeof (TempFlag)
+ 0,
+ mNvVariableCache->Size
);
ASSERT_EFI_ERROR (Status);
}
--
2.16.2.windows.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Fix VarErrorFlag RT cache offset calculation
2020-01-13 23:19 [PATCH V1 1/1] MdeModulePkg/Variable: Fix VarErrorFlag RT cache offset calculation Kubacki, Michael A
@ 2020-01-14 6:42 ` Wang, Jian J
2020-01-15 3:52 ` Kubacki, Michael A
0 siblings, 1 reply; 7+ messages in thread
From: Wang, Jian J @ 2020-01-14 6:42 UTC (permalink / raw)
To: devel@edk2.groups.io, Kubacki, Michael A
Cc: Gao, Liming, Kinney, Michael D, Michael Turner, Wu, Hao A
Michael,
I'm not sure sync-ing whole variable cache memory is an efficient operation.
What about using mVariableModuleGlobal->NonVolatileLastVariableOffset
as Length parameter?
Status = SynchronizeRuntimeVariableCache (
&mVariableModuleGlobal->VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache,
0,
mVariableModuleGlobal->NonVolatileLastVariableOffset
);
Regards,
Jian
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Kubacki,
> Michael A
> Sent: Tuesday, January 14, 2020 7:19 AM
> To: devel@edk2.groups.io
> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Michael Turner
> <michael.turner@microsoft.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 VarErrorFlag
> RT cache offset calculation
>
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2457
>
> This commit fixes an offset calculation that is used to write the
> VarErrorFlag UEFI variable to the UEFI variable runtime cache.
>
> Currently a physical address is used instead of an offset. This
> commit changes the offset to zero with a length of the entire
> non-volatile variable store so the entire non-volatile variable
> store buffer in SMRAM (with the variable update modification) is
> copied to the runtime variable cache. This follows the same pattern
> used in other SynchronizeRuntimeVariableCache () calls for
> consistency.
>
> * Observable symptom: An exception in SMM will most likely occur
> due to the invalid memory reference when the VarErrorFlag variable
> is written. The variable is most commonly written when the UEFI
> variable store is full.
>
> * The issue only occurs when the variable runtime cache is enabled
> by the following PCD being set to TRUE:
> gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache
>
> Fixes: aab3b9b9a1e5e1f3fa966fb1667fc3e6c47e7706
>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Michael Turner <michael.turner@microsoft.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 | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> index b0ee5e50d0..d23aea4bc7 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> @@ -16,7 +16,7 @@
> VariableServiceSetVariable() should also check authenticate data to avoid
> buffer overflow,
> integer overflow. It should also check attribute to avoid authentication bypass.
>
> -Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2006 - 2020, Intel Corporation. All rights reserved.<BR>
> (C) Copyright 2015-2018 Hewlett Packard Enterprise Development LP<BR>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
> @@ -335,8 +335,8 @@ RecordVarErrorFlag (
> *VarErrFlag = TempFlag;
> Status = SynchronizeRuntimeVariableCache (
> &mVariableModuleGlobal-
> >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache,
> - (UINTN) VarErrFlag - (UINTN) mNvVariableCache + (UINTN)
> mVariableModuleGlobal->VariableGlobal.NonVolatileVariableBase,
> - sizeof (TempFlag)
> + 0,
> + mNvVariableCache->Size
> );
> ASSERT_EFI_ERROR (Status);
> }
> --
> 2.16.2.windows.1
>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Fix VarErrorFlag RT cache offset calculation
2020-01-14 6:42 ` [edk2-devel] " Wang, Jian J
@ 2020-01-15 3:52 ` Kubacki, Michael A
2020-01-15 4:09 ` Wang, Jian J
0 siblings, 1 reply; 7+ messages in thread
From: Kubacki, Michael A @ 2020-01-15 3:52 UTC (permalink / raw)
To: Wang, Jian J, devel@edk2.groups.io
Cc: Gao, Liming, Kinney, Michael D, Michael Turner, Wu, Hao A
Hi Jian,
I considered that but these are the reasons I settled on the approach in patch V1.
1. With the variable store filled, the length of mVariableModuleGlobal->NonVolatileLastVariableOffset will only marginally be a smaller value than mNvVariableCache->Size (since variable writes grow the store for SPI flash wear leveling). In this case, it will be ~CommonRuntimeVariableSpace which is usually a major portion of the variable store size anyway.
2. Since mVariableModuleGlobal->NonVolatileLastVariableOffset is a global moving value that is more frequently manipulated than the fixed variable store size, depending upon it increases the likelihood it will be set to an invalid value somewhere else.
3. This is a relatively rare case (an error condition) and the memory copy is within DRAM for variable stores that are typically ~128KB - ~512KB.
To reduce the copy size, the Offset parameter can be "(UINTN) VarErrFlag - (UINTN) mNvVariableCache" (just remove the unnecessary addition of (UINTN) mVariableModuleGlobal->VariableGlobal.NonVolatileVariableBase) with size "sizeof (TempFlag)". How about this in a V2?
Thanks,
Michael
> -----Original Message-----
> From: Wang, Jian J <jian.j.wang@intel.com>
> Sent: Monday, January 13, 2020 10:43 PM
> To: devel@edk2.groups.io; Kubacki, Michael A
> <michael.a.kubacki@intel.com>
> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Michael Turner
> <michael.turner@microsoft.com>; Wu, Hao A <hao.a.wu@intel.com>
> Subject: RE: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Fix
> VarErrorFlag RT cache offset calculation
>
> Michael,
>
> I'm not sure sync-ing whole variable cache memory is an efficient operation.
> What about using mVariableModuleGlobal->NonVolatileLastVariableOffset
> as Length parameter?
>
> Status = SynchronizeRuntimeVariableCache (
> &mVariableModuleGlobal-
> >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache,
> 0,
> mVariableModuleGlobal->NonVolatileLastVariableOffset
> );
>
> Regards,
> Jian
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > Kubacki, Michael A
> > Sent: Tuesday, January 14, 2020 7:19 AM
> > To: devel@edk2.groups.io
> > Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Michael Turner
> > <michael.turner@microsoft.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
> > VarErrorFlag RT cache offset calculation
> >
> > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2457
> >
> > This commit fixes an offset calculation that is used to write the
> > VarErrorFlag UEFI variable to the UEFI variable runtime cache.
> >
> > Currently a physical address is used instead of an offset. This commit
> > changes the offset to zero with a length of the entire non-volatile
> > variable store so the entire non-volatile variable store buffer in
> > SMRAM (with the variable update modification) is copied to the runtime
> > variable cache. This follows the same pattern used in other
> > SynchronizeRuntimeVariableCache () calls for consistency.
> >
> > * Observable symptom: An exception in SMM will most likely occur
> > due to the invalid memory reference when the VarErrorFlag variable
> > is written. The variable is most commonly written when the UEFI
> > variable store is full.
> >
> > * The issue only occurs when the variable runtime cache is enabled
> > by the following PCD being set to TRUE:
> > gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache
> >
> > Fixes: aab3b9b9a1e5e1f3fa966fb1667fc3e6c47e7706
> >
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Michael Turner <michael.turner@microsoft.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 | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > index b0ee5e50d0..d23aea4bc7 100644
> > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > @@ -16,7 +16,7 @@
> > VariableServiceSetVariable() should also check authenticate data to
> > avoid buffer overflow,
> > integer overflow. It should also check attribute to avoid authentication
> bypass.
> >
> > -Copyright (c) 2006 - 2019, Intel Corporation. All rights
> > reserved.<BR>
> > +Copyright (c) 2006 - 2020, Intel Corporation. All rights
> > +reserved.<BR>
> > (C) Copyright 2015-2018 Hewlett Packard Enterprise Development LP<BR>
> > SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > @@ -335,8 +335,8 @@ RecordVarErrorFlag (
> > *VarErrFlag = TempFlag;
> > Status = SynchronizeRuntimeVariableCache (
> > &mVariableModuleGlobal-
> > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache,
> > - (UINTN) VarErrFlag - (UINTN) mNvVariableCache + (UINTN)
> > mVariableModuleGlobal->VariableGlobal.NonVolatileVariableBase,
> > - sizeof (TempFlag)
> > + 0,
> > + mNvVariableCache->Size
> > );
> > ASSERT_EFI_ERROR (Status);
> > }
> > --
> > 2.16.2.windows.1
> >
> >
> >
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Fix VarErrorFlag RT cache offset calculation
2020-01-15 3:52 ` Kubacki, Michael A
@ 2020-01-15 4:09 ` Wang, Jian J
2020-01-15 4:31 ` Kubacki, Michael A
0 siblings, 1 reply; 7+ messages in thread
From: Wang, Jian J @ 2020-01-15 4:09 UTC (permalink / raw)
To: Kubacki, Michael A, devel@edk2.groups.io
Cc: Gao, Liming, Kinney, Michael D, Michael Turner, Wu, Hao A
Mike,
Thanks for explaining. You're right that the error is rare case and it won't
cause big problem, and NonVolatileLastVariableOffset will be approaching
the whole FV size after some time. I don't have strong opinion. Both work
for me.
Regards,
Jian
> -----Original Message-----
> From: Kubacki, Michael A <michael.a.kubacki@intel.com>
> Sent: Wednesday, January 15, 2020 11:53 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Michael Turner
> <michael.turner@microsoft.com>; Wu, Hao A <hao.a.wu@intel.com>
> Subject: RE: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Fix
> VarErrorFlag RT cache offset calculation
>
> Hi Jian,
>
> I considered that but these are the reasons I settled on the approach in patch V1.
>
> 1. With the variable store filled, the length of mVariableModuleGlobal-
> >NonVolatileLastVariableOffset will only marginally be a smaller value than
> mNvVariableCache->Size (since variable writes grow the store for SPI flash wear
> leveling). In this case, it will be ~CommonRuntimeVariableSpace which is usually
> a major portion of the variable store size anyway.
> 2. Since mVariableModuleGlobal->NonVolatileLastVariableOffset is a global
> moving value that is more frequently manipulated than the fixed variable store
> size, depending upon it increases the likelihood it will be set to an invalid value
> somewhere else.
> 3. This is a relatively rare case (an error condition) and the memory copy is
> within DRAM for variable stores that are typically ~128KB - ~512KB.
>
> To reduce the copy size, the Offset parameter can be "(UINTN) VarErrFlag -
> (UINTN) mNvVariableCache" (just remove the unnecessary addition of (UINTN)
> mVariableModuleGlobal->VariableGlobal.NonVolatileVariableBase) with size
> "sizeof (TempFlag)". How about this in a V2?
>
> Thanks,
> Michael
>
> > -----Original Message-----
> > From: Wang, Jian J <jian.j.wang@intel.com>
> > Sent: Monday, January 13, 2020 10:43 PM
> > To: devel@edk2.groups.io; Kubacki, Michael A
> > <michael.a.kubacki@intel.com>
> > Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Michael Turner
> > <michael.turner@microsoft.com>; Wu, Hao A <hao.a.wu@intel.com>
> > Subject: RE: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Fix
> > VarErrorFlag RT cache offset calculation
> >
> > Michael,
> >
> > I'm not sure sync-ing whole variable cache memory is an efficient operation.
> > What about using mVariableModuleGlobal->NonVolatileLastVariableOffset
> > as Length parameter?
> >
> > Status = SynchronizeRuntimeVariableCache (
> > &mVariableModuleGlobal-
> > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache,
> > 0,
> > mVariableModuleGlobal->NonVolatileLastVariableOffset
> > );
> >
> > Regards,
> > Jian
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > Kubacki, Michael A
> > > Sent: Tuesday, January 14, 2020 7:19 AM
> > > To: devel@edk2.groups.io
> > > Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> > > <michael.d.kinney@intel.com>; Michael Turner
> > > <michael.turner@microsoft.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
> > > VarErrorFlag RT cache offset calculation
> > >
> > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2457
> > >
> > > This commit fixes an offset calculation that is used to write the
> > > VarErrorFlag UEFI variable to the UEFI variable runtime cache.
> > >
> > > Currently a physical address is used instead of an offset. This commit
> > > changes the offset to zero with a length of the entire non-volatile
> > > variable store so the entire non-volatile variable store buffer in
> > > SMRAM (with the variable update modification) is copied to the runtime
> > > variable cache. This follows the same pattern used in other
> > > SynchronizeRuntimeVariableCache () calls for consistency.
> > >
> > > * Observable symptom: An exception in SMM will most likely occur
> > > due to the invalid memory reference when the VarErrorFlag variable
> > > is written. The variable is most commonly written when the UEFI
> > > variable store is full.
> > >
> > > * The issue only occurs when the variable runtime cache is enabled
> > > by the following PCD being set to TRUE:
> > > gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache
> > >
> > > Fixes: aab3b9b9a1e5e1f3fa966fb1667fc3e6c47e7706
> > >
> > > Cc: Liming Gao <liming.gao@intel.com>
> > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > Cc: Michael Turner <michael.turner@microsoft.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 | 6 +++---
> > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > > b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > > index b0ee5e50d0..d23aea4bc7 100644
> > > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > > @@ -16,7 +16,7 @@
> > > VariableServiceSetVariable() should also check authenticate data to
> > > avoid buffer overflow,
> > > integer overflow. It should also check attribute to avoid authentication
> > bypass.
> > >
> > > -Copyright (c) 2006 - 2019, Intel Corporation. All rights
> > > reserved.<BR>
> > > +Copyright (c) 2006 - 2020, Intel Corporation. All rights
> > > +reserved.<BR>
> > > (C) Copyright 2015-2018 Hewlett Packard Enterprise Development LP<BR>
> > > SPDX-License-Identifier: BSD-2-Clause-Patent
> > >
> > > @@ -335,8 +335,8 @@ RecordVarErrorFlag (
> > > *VarErrFlag = TempFlag;
> > > Status = SynchronizeRuntimeVariableCache (
> > > &mVariableModuleGlobal-
> > > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache,
> > > - (UINTN) VarErrFlag - (UINTN) mNvVariableCache + (UINTN)
> > > mVariableModuleGlobal->VariableGlobal.NonVolatileVariableBase,
> > > - sizeof (TempFlag)
> > > + 0,
> > > + mNvVariableCache->Size
> > > );
> > > ASSERT_EFI_ERROR (Status);
> > > }
> > > --
> > > 2.16.2.windows.1
> > >
> > >
> > >
> >
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Fix VarErrorFlag RT cache offset calculation
2020-01-15 4:09 ` Wang, Jian J
@ 2020-01-15 4:31 ` Kubacki, Michael A
2020-01-16 2:28 ` Wang, Jian J
0 siblings, 1 reply; 7+ messages in thread
From: Kubacki, Michael A @ 2020-01-15 4:31 UTC (permalink / raw)
To: Wang, Jian J, devel@edk2.groups.io
Cc: Gao, Liming, Kinney, Michael D, Michael Turner, Wu, Hao A
Since I don't have a strong opinion either, I won't make any changes to V1 at this time.
Thanks,
Michael
> -----Original Message-----
> From: Wang, Jian J <jian.j.wang@intel.com>
> Sent: Tuesday, January 14, 2020 8:09 PM
> To: Kubacki, Michael A <michael.a.kubacki@intel.com>;
> devel@edk2.groups.io
> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Michael Turner
> <michael.turner@microsoft.com>; Wu, Hao A <hao.a.wu@intel.com>
> Subject: RE: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Fix
> VarErrorFlag RT cache offset calculation
>
> Mike,
>
> Thanks for explaining. You're right that the error is rare case and it won't
> cause big problem, and NonVolatileLastVariableOffset will be approaching
> the whole FV size after some time. I don't have strong opinion. Both work for
> me.
>
> Regards,
> Jian
>
> > -----Original Message-----
> > From: Kubacki, Michael A <michael.a.kubacki@intel.com>
> > Sent: Wednesday, January 15, 2020 11:53 AM
> > To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
> > Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Michael Turner
> > <michael.turner@microsoft.com>; Wu, Hao A <hao.a.wu@intel.com>
> > Subject: RE: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Fix
> > VarErrorFlag RT cache offset calculation
> >
> > Hi Jian,
> >
> > I considered that but these are the reasons I settled on the approach in
> patch V1.
> >
> > 1. With the variable store filled, the length of
> > mVariableModuleGlobal-
> > >NonVolatileLastVariableOffset will only marginally be a smaller value
> > >than
> > mNvVariableCache->Size (since variable writes grow the store for SPI
> > mNvVariableCache->flash wear
> > leveling). In this case, it will be ~CommonRuntimeVariableSpace which
> > is usually a major portion of the variable store size anyway.
> > 2. Since mVariableModuleGlobal->NonVolatileLastVariableOffset is a
> > global moving value that is more frequently manipulated than the fixed
> > variable store size, depending upon it increases the likelihood it
> > will be set to an invalid value somewhere else.
> > 3. This is a relatively rare case (an error condition) and the memory
> > copy is within DRAM for variable stores that are typically ~128KB - ~512KB.
> >
> > To reduce the copy size, the Offset parameter can be "(UINTN)
> > VarErrFlag -
> > (UINTN) mNvVariableCache" (just remove the unnecessary addition of
> > (UINTN)
> > mVariableModuleGlobal->VariableGlobal.NonVolatileVariableBase) with
> > mVariableModuleGlobal->size
> > "sizeof (TempFlag)". How about this in a V2?
> >
> > Thanks,
> > Michael
> >
> > > -----Original Message-----
> > > From: Wang, Jian J <jian.j.wang@intel.com>
> > > Sent: Monday, January 13, 2020 10:43 PM
> > > To: devel@edk2.groups.io; Kubacki, Michael A
> > > <michael.a.kubacki@intel.com>
> > > Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> > > <michael.d.kinney@intel.com>; Michael Turner
> > > <michael.turner@microsoft.com>; Wu, Hao A <hao.a.wu@intel.com>
> > > Subject: RE: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Fix
> > > VarErrorFlag RT cache offset calculation
> > >
> > > Michael,
> > >
> > > I'm not sure sync-ing whole variable cache memory is an efficient
> operation.
> > > What about using
> > > mVariableModuleGlobal->NonVolatileLastVariableOffset
> > > as Length parameter?
> > >
> > > Status = SynchronizeRuntimeVariableCache (
> > > &mVariableModuleGlobal-
> > >
> >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache,
> > > 0,
> > > mVariableModuleGlobal->NonVolatileLastVariableOffset
> > > );
> > >
> > > Regards,
> > > Jian
> > >
> > > > -----Original Message-----
> > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > > Kubacki, Michael A
> > > > Sent: Tuesday, January 14, 2020 7:19 AM
> > > > To: devel@edk2.groups.io
> > > > Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> > > > <michael.d.kinney@intel.com>; Michael Turner
> > > > <michael.turner@microsoft.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
> > > > VarErrorFlag RT cache offset calculation
> > > >
> > > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2457
> > > >
> > > > This commit fixes an offset calculation that is used to write the
> > > > VarErrorFlag UEFI variable to the UEFI variable runtime cache.
> > > >
> > > > Currently a physical address is used instead of an offset. This
> > > > commit changes the offset to zero with a length of the entire
> > > > non-volatile variable store so the entire non-volatile variable
> > > > store buffer in SMRAM (with the variable update modification) is
> > > > copied to the runtime variable cache. This follows the same
> > > > pattern used in other SynchronizeRuntimeVariableCache () calls for
> consistency.
> > > >
> > > > * Observable symptom: An exception in SMM will most likely occur
> > > > due to the invalid memory reference when the VarErrorFlag variable
> > > > is written. The variable is most commonly written when the UEFI
> > > > variable store is full.
> > > >
> > > > * The issue only occurs when the variable runtime cache is enabled
> > > > by the following PCD being set to TRUE:
> > > > gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache
> > > >
> > > > Fixes: aab3b9b9a1e5e1f3fa966fb1667fc3e6c47e7706
> > > >
> > > > Cc: Liming Gao <liming.gao@intel.com>
> > > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > > Cc: Michael Turner <michael.turner@microsoft.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 | 6 +++---
> > > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > > > b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > > > index b0ee5e50d0..d23aea4bc7 100644
> > > > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > > > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > > > @@ -16,7 +16,7 @@
> > > > VariableServiceSetVariable() should also check authenticate
> > > > data to avoid buffer overflow,
> > > > integer overflow. It should also check attribute to avoid
> > > > authentication
> > > bypass.
> > > >
> > > > -Copyright (c) 2006 - 2019, Intel Corporation. All rights
> > > > reserved.<BR>
> > > > +Copyright (c) 2006 - 2020, Intel Corporation. All rights
> > > > +reserved.<BR>
> > > > (C) Copyright 2015-2018 Hewlett Packard Enterprise Development
> > > > LP<BR>
> > > > SPDX-License-Identifier: BSD-2-Clause-Patent
> > > >
> > > > @@ -335,8 +335,8 @@ RecordVarErrorFlag (
> > > > *VarErrFlag = TempFlag;
> > > > Status = SynchronizeRuntimeVariableCache (
> > > > &mVariableModuleGlobal-
> > > >
> >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache
> > > > >,
> > > > - (UINTN) VarErrFlag - (UINTN) mNvVariableCache + (UINTN)
> > > > mVariableModuleGlobal->VariableGlobal.NonVolatileVariableBase,
> > > > - sizeof (TempFlag)
> > > > + 0,
> > > > + mNvVariableCache->Size
> > > > );
> > > > ASSERT_EFI_ERROR (Status);
> > > > }
> > > > --
> > > > 2.16.2.windows.1
> > > >
> > > >
> > > >
> > >
> >
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Fix VarErrorFlag RT cache offset calculation
2020-01-15 4:31 ` Kubacki, Michael A
@ 2020-01-16 2:28 ` Wang, Jian J
2020-01-16 5:56 ` Liming Gao
0 siblings, 1 reply; 7+ messages in thread
From: Wang, Jian J @ 2020-01-16 2:28 UTC (permalink / raw)
To: Kubacki, Michael A, devel@edk2.groups.io
Cc: Gao, Liming, Kinney, Michael D, Michael Turner, Wu, Hao A
Reviewed-by: Jian J Wang <jian.j.wang@intel.com>
Regards,
Jian
> -----Original Message-----
> From: Kubacki, Michael A <michael.a.kubacki@intel.com>
> Sent: Wednesday, January 15, 2020 12:31 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Michael Turner
> <michael.turner@microsoft.com>; Wu, Hao A <hao.a.wu@intel.com>
> Subject: RE: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Fix
> VarErrorFlag RT cache offset calculation
>
> Since I don't have a strong opinion either, I won't make any changes to V1 at this
> time.
>
> Thanks,
> Michael
>
> > -----Original Message-----
> > From: Wang, Jian J <jian.j.wang@intel.com>
> > Sent: Tuesday, January 14, 2020 8:09 PM
> > To: Kubacki, Michael A <michael.a.kubacki@intel.com>;
> > devel@edk2.groups.io
> > Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Michael Turner
> > <michael.turner@microsoft.com>; Wu, Hao A <hao.a.wu@intel.com>
> > Subject: RE: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Fix
> > VarErrorFlag RT cache offset calculation
> >
> > Mike,
> >
> > Thanks for explaining. You're right that the error is rare case and it won't
> > cause big problem, and NonVolatileLastVariableOffset will be approaching
> > the whole FV size after some time. I don't have strong opinion. Both work for
> > me.
> >
> > Regards,
> > Jian
> >
> > > -----Original Message-----
> > > From: Kubacki, Michael A <michael.a.kubacki@intel.com>
> > > Sent: Wednesday, January 15, 2020 11:53 AM
> > > To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
> > > Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> > > <michael.d.kinney@intel.com>; Michael Turner
> > > <michael.turner@microsoft.com>; Wu, Hao A <hao.a.wu@intel.com>
> > > Subject: RE: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Fix
> > > VarErrorFlag RT cache offset calculation
> > >
> > > Hi Jian,
> > >
> > > I considered that but these are the reasons I settled on the approach in
> > patch V1.
> > >
> > > 1. With the variable store filled, the length of
> > > mVariableModuleGlobal-
> > > >NonVolatileLastVariableOffset will only marginally be a smaller value
> > > >than
> > > mNvVariableCache->Size (since variable writes grow the store for SPI
> > > mNvVariableCache->flash wear
> > > leveling). In this case, it will be ~CommonRuntimeVariableSpace which
> > > is usually a major portion of the variable store size anyway.
> > > 2. Since mVariableModuleGlobal->NonVolatileLastVariableOffset is a
> > > global moving value that is more frequently manipulated than the fixed
> > > variable store size, depending upon it increases the likelihood it
> > > will be set to an invalid value somewhere else.
> > > 3. This is a relatively rare case (an error condition) and the memory
> > > copy is within DRAM for variable stores that are typically ~128KB - ~512KB.
> > >
> > > To reduce the copy size, the Offset parameter can be "(UINTN)
> > > VarErrFlag -
> > > (UINTN) mNvVariableCache" (just remove the unnecessary addition of
> > > (UINTN)
> > > mVariableModuleGlobal->VariableGlobal.NonVolatileVariableBase) with
> > > mVariableModuleGlobal->size
> > > "sizeof (TempFlag)". How about this in a V2?
> > >
> > > Thanks,
> > > Michael
> > >
> > > > -----Original Message-----
> > > > From: Wang, Jian J <jian.j.wang@intel.com>
> > > > Sent: Monday, January 13, 2020 10:43 PM
> > > > To: devel@edk2.groups.io; Kubacki, Michael A
> > > > <michael.a.kubacki@intel.com>
> > > > Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> > > > <michael.d.kinney@intel.com>; Michael Turner
> > > > <michael.turner@microsoft.com>; Wu, Hao A <hao.a.wu@intel.com>
> > > > Subject: RE: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Fix
> > > > VarErrorFlag RT cache offset calculation
> > > >
> > > > Michael,
> > > >
> > > > I'm not sure sync-ing whole variable cache memory is an efficient
> > operation.
> > > > What about using
> > > > mVariableModuleGlobal->NonVolatileLastVariableOffset
> > > > as Length parameter?
> > > >
> > > > Status = SynchronizeRuntimeVariableCache (
> > > > &mVariableModuleGlobal-
> > > >
> > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache,
> > > > 0,
> > > > mVariableModuleGlobal->NonVolatileLastVariableOffset
> > > > );
> > > >
> > > > Regards,
> > > > Jian
> > > >
> > > > > -----Original Message-----
> > > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > > > Kubacki, Michael A
> > > > > Sent: Tuesday, January 14, 2020 7:19 AM
> > > > > To: devel@edk2.groups.io
> > > > > Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> > > > > <michael.d.kinney@intel.com>; Michael Turner
> > > > > <michael.turner@microsoft.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
> > > > > VarErrorFlag RT cache offset calculation
> > > > >
> > > > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2457
> > > > >
> > > > > This commit fixes an offset calculation that is used to write the
> > > > > VarErrorFlag UEFI variable to the UEFI variable runtime cache.
> > > > >
> > > > > Currently a physical address is used instead of an offset. This
> > > > > commit changes the offset to zero with a length of the entire
> > > > > non-volatile variable store so the entire non-volatile variable
> > > > > store buffer in SMRAM (with the variable update modification) is
> > > > > copied to the runtime variable cache. This follows the same
> > > > > pattern used in other SynchronizeRuntimeVariableCache () calls for
> > consistency.
> > > > >
> > > > > * Observable symptom: An exception in SMM will most likely occur
> > > > > due to the invalid memory reference when the VarErrorFlag variable
> > > > > is written. The variable is most commonly written when the UEFI
> > > > > variable store is full.
> > > > >
> > > > > * The issue only occurs when the variable runtime cache is enabled
> > > > > by the following PCD being set to TRUE:
> > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache
> > > > >
> > > > > Fixes: aab3b9b9a1e5e1f3fa966fb1667fc3e6c47e7706
> > > > >
> > > > > Cc: Liming Gao <liming.gao@intel.com>
> > > > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > > > Cc: Michael Turner <michael.turner@microsoft.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 | 6 +++---
> > > > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > > > > b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > > > > index b0ee5e50d0..d23aea4bc7 100644
> > > > > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > > > > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > > > > @@ -16,7 +16,7 @@
> > > > > VariableServiceSetVariable() should also check authenticate
> > > > > data to avoid buffer overflow,
> > > > > integer overflow. It should also check attribute to avoid
> > > > > authentication
> > > > bypass.
> > > > >
> > > > > -Copyright (c) 2006 - 2019, Intel Corporation. All rights
> > > > > reserved.<BR>
> > > > > +Copyright (c) 2006 - 2020, Intel Corporation. All rights
> > > > > +reserved.<BR>
> > > > > (C) Copyright 2015-2018 Hewlett Packard Enterprise Development
> > > > > LP<BR>
> > > > > SPDX-License-Identifier: BSD-2-Clause-Patent
> > > > >
> > > > > @@ -335,8 +335,8 @@ RecordVarErrorFlag (
> > > > > *VarErrFlag = TempFlag;
> > > > > Status = SynchronizeRuntimeVariableCache (
> > > > > &mVariableModuleGlobal-
> > > > >
> > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache
> > > > > >,
> > > > > - (UINTN) VarErrFlag - (UINTN) mNvVariableCache + (UINTN)
> > > > > mVariableModuleGlobal->VariableGlobal.NonVolatileVariableBase,
> > > > > - sizeof (TempFlag)
> > > > > + 0,
> > > > > + mNvVariableCache->Size
> > > > > );
> > > > > ASSERT_EFI_ERROR (Status);
> > > > > }
> > > > > --
> > > > > 2.16.2.windows.1
> > > > >
> > > > >
> > > > >
> > > >
> > >
> >
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Fix VarErrorFlag RT cache offset calculation
2020-01-16 2:28 ` Wang, Jian J
@ 2020-01-16 5:56 ` Liming Gao
0 siblings, 0 replies; 7+ messages in thread
From: Liming Gao @ 2020-01-16 5:56 UTC (permalink / raw)
To: Wang, Jian J, Kubacki, Michael A, devel@edk2.groups.io
Cc: Kinney, Michael D, Michael Turner, Wu, Hao A
Reviewed-by: Liming Gao <liming.gao@intel.com>
> -----Original Message-----
> From: Wang, Jian J <jian.j.wang@intel.com>
> Sent: Thursday, January 16, 2020 10:29 AM
> To: Kubacki, Michael A <michael.a.kubacki@intel.com>; devel@edk2.groups.io
> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Michael Turner
> <michael.turner@microsoft.com>; Wu, Hao A <hao.a.wu@intel.com>
> Subject: RE: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Fix VarErrorFlag RT cache offset calculation
>
>
> Reviewed-by: Jian J Wang <jian.j.wang@intel.com>
>
> Regards,
> Jian
>
> > -----Original Message-----
> > From: Kubacki, Michael A <michael.a.kubacki@intel.com>
> > Sent: Wednesday, January 15, 2020 12:31 PM
> > To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
> > Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Michael Turner
> > <michael.turner@microsoft.com>; Wu, Hao A <hao.a.wu@intel.com>
> > Subject: RE: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Fix
> > VarErrorFlag RT cache offset calculation
> >
> > Since I don't have a strong opinion either, I won't make any changes to V1 at this
> > time.
> >
> > Thanks,
> > Michael
> >
> > > -----Original Message-----
> > > From: Wang, Jian J <jian.j.wang@intel.com>
> > > Sent: Tuesday, January 14, 2020 8:09 PM
> > > To: Kubacki, Michael A <michael.a.kubacki@intel.com>;
> > > devel@edk2.groups.io
> > > Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> > > <michael.d.kinney@intel.com>; Michael Turner
> > > <michael.turner@microsoft.com>; Wu, Hao A <hao.a.wu@intel.com>
> > > Subject: RE: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Fix
> > > VarErrorFlag RT cache offset calculation
> > >
> > > Mike,
> > >
> > > Thanks for explaining. You're right that the error is rare case and it won't
> > > cause big problem, and NonVolatileLastVariableOffset will be approaching
> > > the whole FV size after some time. I don't have strong opinion. Both work for
> > > me.
> > >
> > > Regards,
> > > Jian
> > >
> > > > -----Original Message-----
> > > > From: Kubacki, Michael A <michael.a.kubacki@intel.com>
> > > > Sent: Wednesday, January 15, 2020 11:53 AM
> > > > To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
> > > > Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> > > > <michael.d.kinney@intel.com>; Michael Turner
> > > > <michael.turner@microsoft.com>; Wu, Hao A <hao.a.wu@intel.com>
> > > > Subject: RE: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Fix
> > > > VarErrorFlag RT cache offset calculation
> > > >
> > > > Hi Jian,
> > > >
> > > > I considered that but these are the reasons I settled on the approach in
> > > patch V1.
> > > >
> > > > 1. With the variable store filled, the length of
> > > > mVariableModuleGlobal-
> > > > >NonVolatileLastVariableOffset will only marginally be a smaller value
> > > > >than
> > > > mNvVariableCache->Size (since variable writes grow the store for SPI
> > > > mNvVariableCache->flash wear
> > > > leveling). In this case, it will be ~CommonRuntimeVariableSpace which
> > > > is usually a major portion of the variable store size anyway.
> > > > 2. Since mVariableModuleGlobal->NonVolatileLastVariableOffset is a
> > > > global moving value that is more frequently manipulated than the fixed
> > > > variable store size, depending upon it increases the likelihood it
> > > > will be set to an invalid value somewhere else.
> > > > 3. This is a relatively rare case (an error condition) and the memory
> > > > copy is within DRAM for variable stores that are typically ~128KB - ~512KB.
> > > >
> > > > To reduce the copy size, the Offset parameter can be "(UINTN)
> > > > VarErrFlag -
> > > > (UINTN) mNvVariableCache" (just remove the unnecessary addition of
> > > > (UINTN)
> > > > mVariableModuleGlobal->VariableGlobal.NonVolatileVariableBase) with
> > > > mVariableModuleGlobal->size
> > > > "sizeof (TempFlag)". How about this in a V2?
> > > >
> > > > Thanks,
> > > > Michael
> > > >
> > > > > -----Original Message-----
> > > > > From: Wang, Jian J <jian.j.wang@intel.com>
> > > > > Sent: Monday, January 13, 2020 10:43 PM
> > > > > To: devel@edk2.groups.io; Kubacki, Michael A
> > > > > <michael.a.kubacki@intel.com>
> > > > > Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> > > > > <michael.d.kinney@intel.com>; Michael Turner
> > > > > <michael.turner@microsoft.com>; Wu, Hao A <hao.a.wu@intel.com>
> > > > > Subject: RE: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Fix
> > > > > VarErrorFlag RT cache offset calculation
> > > > >
> > > > > Michael,
> > > > >
> > > > > I'm not sure sync-ing whole variable cache memory is an efficient
> > > operation.
> > > > > What about using
> > > > > mVariableModuleGlobal->NonVolatileLastVariableOffset
> > > > > as Length parameter?
> > > > >
> > > > > Status = SynchronizeRuntimeVariableCache (
> > > > > &mVariableModuleGlobal-
> > > > >
> > > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache,
> > > > > 0,
> > > > > mVariableModuleGlobal->NonVolatileLastVariableOffset
> > > > > );
> > > > >
> > > > > Regards,
> > > > > Jian
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > > > > Kubacki, Michael A
> > > > > > Sent: Tuesday, January 14, 2020 7:19 AM
> > > > > > To: devel@edk2.groups.io
> > > > > > Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> > > > > > <michael.d.kinney@intel.com>; Michael Turner
> > > > > > <michael.turner@microsoft.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
> > > > > > VarErrorFlag RT cache offset calculation
> > > > > >
> > > > > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2457
> > > > > >
> > > > > > This commit fixes an offset calculation that is used to write the
> > > > > > VarErrorFlag UEFI variable to the UEFI variable runtime cache.
> > > > > >
> > > > > > Currently a physical address is used instead of an offset. This
> > > > > > commit changes the offset to zero with a length of the entire
> > > > > > non-volatile variable store so the entire non-volatile variable
> > > > > > store buffer in SMRAM (with the variable update modification) is
> > > > > > copied to the runtime variable cache. This follows the same
> > > > > > pattern used in other SynchronizeRuntimeVariableCache () calls for
> > > consistency.
> > > > > >
> > > > > > * Observable symptom: An exception in SMM will most likely occur
> > > > > > due to the invalid memory reference when the VarErrorFlag variable
> > > > > > is written. The variable is most commonly written when the UEFI
> > > > > > variable store is full.
> > > > > >
> > > > > > * The issue only occurs when the variable runtime cache is enabled
> > > > > > by the following PCD being set to TRUE:
> > > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache
> > > > > >
> > > > > > Fixes: aab3b9b9a1e5e1f3fa966fb1667fc3e6c47e7706
> > > > > >
> > > > > > Cc: Liming Gao <liming.gao@intel.com>
> > > > > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > > > > Cc: Michael Turner <michael.turner@microsoft.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 | 6 +++---
> > > > > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > > > > > b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > > > > > index b0ee5e50d0..d23aea4bc7 100644
> > > > > > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > > > > > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > > > > > @@ -16,7 +16,7 @@
> > > > > > VariableServiceSetVariable() should also check authenticate
> > > > > > data to avoid buffer overflow,
> > > > > > integer overflow. It should also check attribute to avoid
> > > > > > authentication
> > > > > bypass.
> > > > > >
> > > > > > -Copyright (c) 2006 - 2019, Intel Corporation. All rights
> > > > > > reserved.<BR>
> > > > > > +Copyright (c) 2006 - 2020, Intel Corporation. All rights
> > > > > > +reserved.<BR>
> > > > > > (C) Copyright 2015-2018 Hewlett Packard Enterprise Development
> > > > > > LP<BR>
> > > > > > SPDX-License-Identifier: BSD-2-Clause-Patent
> > > > > >
> > > > > > @@ -335,8 +335,8 @@ RecordVarErrorFlag (
> > > > > > *VarErrFlag = TempFlag;
> > > > > > Status = SynchronizeRuntimeVariableCache (
> > > > > > &mVariableModuleGlobal-
> > > > > >
> > > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache
> > > > > > >,
> > > > > > - (UINTN) VarErrFlag - (UINTN) mNvVariableCache + (UINTN)
> > > > > > mVariableModuleGlobal->VariableGlobal.NonVolatileVariableBase,
> > > > > > - sizeof (TempFlag)
> > > > > > + 0,
> > > > > > + mNvVariableCache->Size
> > > > > > );
> > > > > > ASSERT_EFI_ERROR (Status);
> > > > > > }
> > > > > > --
> > > > > > 2.16.2.windows.1
> > > > > >
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-01-16 5:56 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-01-13 23:19 [PATCH V1 1/1] MdeModulePkg/Variable: Fix VarErrorFlag RT cache offset calculation Kubacki, Michael A
2020-01-14 6:42 ` [edk2-devel] " Wang, Jian J
2020-01-15 3:52 ` Kubacki, Michael A
2020-01-15 4:09 ` Wang, Jian J
2020-01-15 4:31 ` Kubacki, Michael A
2020-01-16 2:28 ` Wang, Jian J
2020-01-16 5:56 ` Liming Gao
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox