Re: [edk2-devel] [PATCH V4 07/10] MdeModulePkg/Variable: Add RT GetVariable() cache support
Jian, thanks a lot your analysis. The intention was very much to constrain the
flow of information
from SMM to the non-SMM environment and not vice versa during runtime cache
operation.
I agree that the buffers pointed to in
SMM_VARIABLE_COMMUNICATE_RUNTIME_VARIABLE_CACHE_CONTEXT
should be checked with VariableSmmIsBufferOutsideSmmValid (). I will send a V5
with this change.
Thanks,
Michael
> -Original Message-
> From: Wang, Jian J
> Sent: Thursday, October 17, 2019 7:23 AM
> To: Wu, Hao A ; Kubacki, Michael A
> ; devel@edk2.groups.io
> Cc: Bi, Dandan ; Ard Biesheuvel
> ; Dong, Eric ; Laszlo Ersek
> ; Gao, Liming ; Kinney, Michael
> D ; Ni, Ray ; Yao, Jiewen
>
> Subject: RE: [PATCH V4 07/10] MdeModulePkg/Variable: Add RT
> GetVariable() cache support
>
> >
> > Again, I would like to ask for help from other reviewers to look at this
> > patch
> > (patch 7/10) and the next one (patch 8/10) (at least from the security
> > perspective). Any help will be appreciated, thanks in advance.
> >
>
> I'm trying to do a simple analysis below from security perspective. Please
> correct me
> if anything wrong.
>
> The patch added 3 new SMI variable Functions
> f(a):
> SMM_VARIABLE_FUNCTION_INIT_RUNTIME_VARIABLE_CACHE_CONTEXT
> f(b): SMM_VARIABLE_FUNCTION_SYNC_RUNTIME_CACHE
> f(c): SMM_VARIABLE_FUNCTION_GET_RUNTIME_CACHE_INFO
>
> f(a) communication buffer is type of
> SMM_VARIABLE_COMMUNICATE_RUNTIME_VARIABLE_CACHE_CONTEXT
>
> which is defined as
> typedef struct {
> BOOLEAN *ReadLock;
> BOOLEAN *PendingUpdate;
> BOOLEAN *HobFlushComplete;
> VARIABLE_STORE_HEADER *RuntimeHobCache;
>VARIABLE_STORE_HEADER *RuntimeNvCache;
> VARIABLE_STORE_HEADER *RuntimeVolatileCache;
> }
> SMM_VARIABLE_COMMUNICATE_RUNTIME_VARIABLE_CACHE_CONTEXT;
>
> There're 6 fields (all pointers) in the buffer. SMM will handle them in
> following ways
> - ReadLock and PendingUpdate are used to avoid race condition between
> smm and
> non-smm code. I don't think there's security breach here.
> - RuntimeHobCache, RuntimeNvCache and RuntimeVolatileCache are buffers
> passed
> to smm code to simply store variable content. Smm code will not read its
> content.
> I think this won't bring any security risk.
>
> The SmmVariableHandler() will call VariableSmmIsBufferOutsideSmmValid()
> to make sure the communication buffer is outside of smram. But the handler
> will
> not check the above 6 pointers passed through communication buffer to see
> if they
> are in non-smram scope. This leaves potential security hole to allow smram
> buffer
> to be passed in from non-smm code.
>
> f(b) doesn't use communication buffer. It's just used to flush variable cache.
> Then
> no security risk here.
>
> f(c) uses communication buffer with type of
>
> typedef struct {
> UINTN TotalHobStorageSize;
> UINTN TotalNvStorageSize;
> UINTN TotalVolatileStorageSize;
> BOOLEAN AuthenticatedVariableUsage;
> } SMM_VARIABLE_COMMUNICATE_GET_RUNTIME_CACHE_INFO;
>
> There's no pointer in the buffer and the SMI handler doesn't read its content
> but
> just store data into it. I think there's no security risk here.
>
> I also checked data flow newly introduced between smm and non-smm
> boundary.
> Most of the data flow is just from smm to non-smm. The only exception is
> ReadLock
> mentioned above. Generally speaking, I don't find security issue in this patch
> series
> except to above pointers passed by communication buffer.
>
> Regards,
> Jian
>
> > -Original Message-
> > From: Wu, Hao A
> > Sent: Wednesday, October 16, 2019 3:57 PM
> > To: Kubacki, Michael A ;
> devel@edk2.groups.io
> > Cc: Bi, Dandan ; Ard Biesheuvel
> > ; Dong, Eric ; Laszlo
> Ersek
> > ; Gao, Liming ; Kinney,
> Michael D
> > ; Ni, Ray ; Wang, Jian J
> > ; Yao, Jiewen
> > Subject: RE: [PATCH V4 07/10] MdeModulePkg/Variable: Add RT
> GetVariable()
> > cache support
> >
> > Again, I would like to ask for help from other reviewers to look at this
> > patch
> > (patch 7/10) and the next one (patch 8/10) (at least from the security
> > perspective). Any help will be appreciated, thanks in advance.
> >
> >
> > One comment inherited from the feedback on the V2 series:
> > I saw AtRuntime() is still being added in file VariableSmmRuntimeDxe.c,
> could
> > you help to double confirm?
> >
> > Another general level comment is that:
> > Please help to update the MdeModulePkg.uni file as well for the introduce
> of
> > the new PCD.
> >
> > Inline comments below:
> >
> >
> > > -Original Message-
> > > From: Kubacki, Michael A
> > > Sent: Tuesday, October 15, 2019 7:30 AM
> > > To: devel@edk2.groups.io
> > > Cc: Bi, Dandan; Ard Biesheuvel; Dong, Eric; Laszlo Ersek; Gao, Liming;
> Kinney,
> > > Michael D; Ni, Ray; Wang, Jian J; Wu, Hao A; Yao, Jiewen
> > > Subject: [PATCH V4 07/10] MdeModulePkg/Variable:
Re: [edk2-devel] [PATCH V4 07/10] MdeModulePkg/Variable: Add RT GetVariable() cache support
>
> Again, I would like to ask for help from other reviewers to look at this patch
> (patch 7/10) and the next one (patch 8/10) (at least from the security
> perspective). Any help will be appreciated, thanks in advance.
>
I'm trying to do a simple analysis below from security perspective. Please
correct me
if anything wrong.
The patch added 3 new SMI variable Functions
f(a): SMM_VARIABLE_FUNCTION_INIT_RUNTIME_VARIABLE_CACHE_CONTEXT
f(b): SMM_VARIABLE_FUNCTION_SYNC_RUNTIME_CACHE
f(c): SMM_VARIABLE_FUNCTION_GET_RUNTIME_CACHE_INFO
f(a) communication buffer is type of
SMM_VARIABLE_COMMUNICATE_RUNTIME_VARIABLE_CACHE_CONTEXT
which is defined as
typedef struct {
BOOLEAN *ReadLock;
BOOLEAN *PendingUpdate;
BOOLEAN *HobFlushComplete;
VARIABLE_STORE_HEADER *RuntimeHobCache;
VARIABLE_STORE_HEADER *RuntimeNvCache;
VARIABLE_STORE_HEADER *RuntimeVolatileCache;
} SMM_VARIABLE_COMMUNICATE_RUNTIME_VARIABLE_CACHE_CONTEXT;
There're 6 fields (all pointers) in the buffer. SMM will handle them in
following ways
- ReadLock and PendingUpdate are used to avoid race condition between smm and
non-smm code. I don't think there's security breach here.
- RuntimeHobCache, RuntimeNvCache and RuntimeVolatileCache are buffers passed
to smm code to simply store variable content. Smm code will not read its
content.
I think this won't bring any security risk.
The SmmVariableHandler() will call VariableSmmIsBufferOutsideSmmValid()
to make sure the communication buffer is outside of smram. But the handler will
not check the above 6 pointers passed through communication buffer to see if
they
are in non-smram scope. This leaves potential security hole to allow smram
buffer
to be passed in from non-smm code.
f(b) doesn't use communication buffer. It's just used to flush variable cache.
Then
no security risk here.
f(c) uses communication buffer with type of
typedef struct {
UINTN TotalHobStorageSize;
UINTN TotalNvStorageSize;
UINTN TotalVolatileStorageSize;
BOOLEAN AuthenticatedVariableUsage;
} SMM_VARIABLE_COMMUNICATE_GET_RUNTIME_CACHE_INFO;
There's no pointer in the buffer and the SMI handler doesn't read its content
but
just store data into it. I think there's no security risk here.
I also checked data flow newly introduced between smm and non-smm boundary.
Most of the data flow is just from smm to non-smm. The only exception is
ReadLock
mentioned above. Generally speaking, I don't find security issue in this patch
series
except to above pointers passed by communication buffer.
Regards,
Jian
> -Original Message-
> From: Wu, Hao A
> Sent: Wednesday, October 16, 2019 3:57 PM
> To: Kubacki, Michael A ; devel@edk2.groups.io
> Cc: Bi, Dandan ; Ard Biesheuvel
> ; Dong, Eric ; Laszlo Ersek
> ; Gao, Liming ; Kinney, Michael D
> ; Ni, Ray ; Wang, Jian J
> ; Yao, Jiewen
> Subject: RE: [PATCH V4 07/10] MdeModulePkg/Variable: Add RT GetVariable()
> cache support
>
> Again, I would like to ask for help from other reviewers to look at this patch
> (patch 7/10) and the next one (patch 8/10) (at least from the security
> perspective). Any help will be appreciated, thanks in advance.
>
>
> One comment inherited from the feedback on the V2 series:
> I saw AtRuntime() is still being added in file VariableSmmRuntimeDxe.c, could
> you help to double confirm?
>
> Another general level comment is that:
> Please help to update the MdeModulePkg.uni file as well for the introduce of
> the new PCD.
>
> Inline comments below:
>
>
> > -Original Message-
> > From: Kubacki, Michael A
> > Sent: Tuesday, October 15, 2019 7:30 AM
> > To: devel@edk2.groups.io
> > Cc: Bi, Dandan; Ard Biesheuvel; Dong, Eric; Laszlo Ersek; Gao, Liming;
> > Kinney,
> > Michael D; Ni, Ray; Wang, Jian J; Wu, Hao A; Yao, Jiewen
> > Subject: [PATCH V4 07/10] MdeModulePkg/Variable: Add RT GetVariable()
> > cache support
> >
> > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2220
> >
> > This change reduces SMIs for GetVariable () by maintaining a
> > UEFI variable cache in Runtime DXE in addition to the pre-
> > existing cache in SMRAM. When the Runtime Service GetVariable()
> > is invoked, a Runtime DXE cache is used instead of triggering an
> > SMI to VariableSmm. This can improve overall system performance
> > by servicing variable read requests without rendezvousing all
> > cores into SMM.
> >
> > The runtime cache can be disabled with by setting the FeaturePCD
> > gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache
> > to FALSE. If the PCD is set to FALSE, the runtime cache will not be
> > used and an SMI will be triggered for Runtime Service
> > GetVariable () and GetNextVariableName () invocations.
> >
> > The following are important points regarding the behavior of the
> > variable drivers when the variable runtime cache is enabled.
> >
> > 1. All of
Re: [edk2-devel] [PATCH V4 07/10] MdeModulePkg/Variable: Add RT GetVariable() cache support
> The "Data" parameter is marked OPTIONAL in FindVariableInRuntimeCache () for > essentially the same reason > it is labeled OPTIONAL in the GetVariable () API in the UEFI specification. > Callers > expect that they can pass a > NULL as an actual parameter for Data and get back the size of a buffer needed > for the given variable name and > GUID. > You're right. I missed some part of code. > I addressed the implementation of AtRuntime () in VariableSmmRuntimeDxe.c > alongside EfiAtRuntime () calls in > the file in another V4 reply. The conclusion is AtRuntime () is called by > functions > in VariableParsing.c which due > to its generic nature is linked against VariableSmmRuntimeDxe and VariableSmm. > VariableSmm cannot call > EfiAtRuntime (). There's various ways to twist this but most I've considered > are > really cosmetic tradeoffs. > I see. Thanks for explanation. > Jian, you left it open to me as to whether the buffers should be freed in > SmmVariableReady () in VariableSmmRuntimeDxe.c > in the failure case. During implementation, I initially thought the platform > would > roughly "earmark" a fixed amount of > overall EfiRuntimeServicesData memory (typical value + some threshold) for S4 > memory map consistency so it's not > consuming memory not already accounted for. I was also not aware of any > harmful effects to not freeing the buffers, > but I did mean to reconsider this in the future. I still don't have enough > justification to form a strong opinion either way, > so please let me know if you think it's necessary. Let's keep it as-is. Regards, Jian > -Original Message- > From: Kubacki, Michael A > Sent: Thursday, October 17, 2019 9:25 AM > To: Wang, Jian J ; devel@edk2.groups.io > Cc: Bi, Dandan ; Ard Biesheuvel > ; Dong, Eric ; Laszlo Ersek > ; Gao, Liming ; Kinney, Michael D > ; Ni, Ray ; Wu, Hao A > ; Yao, Jiewen > Subject: RE: [edk2-devel] [PATCH V4 07/10] MdeModulePkg/Variable: Add RT > GetVariable() cache support > > The "Data" parameter is marked OPTIONAL in FindVariableInRuntimeCache () for > essentially the same reason > it is labeled OPTIONAL in the GetVariable () API in the UEFI specification. > Callers > expect that they can pass a > NULL as an actual parameter for Data and get back the size of a buffer needed > for the given variable name and > GUID. > > I addressed the implementation of AtRuntime () in VariableSmmRuntimeDxe.c > alongside EfiAtRuntime () calls in > the file in another V4 reply. The conclusion is AtRuntime () is called by > functions > in VariableParsing.c which due > to its generic nature is linked against VariableSmmRuntimeDxe and VariableSmm. > VariableSmm cannot call > EfiAtRuntime (). There's various ways to twist this but most I've considered > are > really cosmetic tradeoffs. > > Jian, you left it open to me as to whether the buffers should be freed in > SmmVariableReady () in VariableSmmRuntimeDxe.c > in the failure case. During implementation, I initially thought the platform > would > roughly "earmark" a fixed amount of > overall EfiRuntimeServicesData memory (typical value + some threshold) for S4 > memory map consistency so it's not > consuming memory not already accounted for. I was also not aware of any > harmful effects to not freeing the buffers, > but I did mean to reconsider this in the future. I still don't have enough > justification to form a strong opinion either way, > so please let me know if you think it's necessary. > > Thanks, > Michael > > > -Original Message- > > From: Wang, Jian J > > Sent: Tuesday, October 15, 2019 11:55 PM > > To: devel@edk2.groups.io; Wang, Jian J ; Kubacki, > > Michael A > > Cc: Bi, Dandan ; Ard Biesheuvel > > ; Dong, Eric ; Laszlo Ersek > > ; Gao, Liming ; Kinney, Michael > > D ; Ni, Ray ; Wu, Hao A > > ; Yao, Jiewen > > Subject: RE: [edk2-devel] [PATCH V4 07/10] MdeModulePkg/Variable: Add > > RT GetVariable() cache support > > > > The comments are for VariableRuntimeCache.c only. > > > > Regards, > > Jian > > > > > -----Original Message- > > > From: devel@edk2.groups.io On Behalf Of Wang, > > Jian > > > J > > > Sent: Wednesday, October 16, 2019 2:46 PM > > > To: Kubacki, Michael A ; > > devel@edk2.groups.io > > > Cc: Bi, Dandan ; Ard Biesheuvel > > > ; Dong, Eric ; Laszlo > > Ersek > > > ; Gao, Liming ; Kinney, > > Michael D > > > ; Ni, Ray ; Wu, Hao A > > > ; Yao, Jiewen > > > Subject: Re: [edk2-devel] [PATCH V4 07/10] MdeModulePkg/Variable: Add > > R
Re: [edk2-devel] [PATCH V4 07/10] MdeModulePkg/Variable: Add RT GetVariable() cache support
The "Data" parameter is marked OPTIONAL in FindVariableInRuntimeCache () for essentially the same reason it is labeled OPTIONAL in the GetVariable () API in the UEFI specification. Callers expect that they can pass a NULL as an actual parameter for Data and get back the size of a buffer needed for the given variable name and GUID. I addressed the implementation of AtRuntime () in VariableSmmRuntimeDxe.c alongside EfiAtRuntime () calls in the file in another V4 reply. The conclusion is AtRuntime () is called by functions in VariableParsing.c which due to its generic nature is linked against VariableSmmRuntimeDxe and VariableSmm. VariableSmm cannot call EfiAtRuntime (). There's various ways to twist this but most I've considered are really cosmetic tradeoffs. Jian, you left it open to me as to whether the buffers should be freed in SmmVariableReady () in VariableSmmRuntimeDxe.c in the failure case. During implementation, I initially thought the platform would roughly "earmark" a fixed amount of overall EfiRuntimeServicesData memory (typical value + some threshold) for S4 memory map consistency so it's not consuming memory not already accounted for. I was also not aware of any harmful effects to not freeing the buffers, but I did mean to reconsider this in the future. I still don't have enough justification to form a strong opinion either way, so please let me know if you think it's necessary. Thanks, Michael > -Original Message- > From: Wang, Jian J > Sent: Tuesday, October 15, 2019 11:55 PM > To: devel@edk2.groups.io; Wang, Jian J ; Kubacki, > Michael A > Cc: Bi, Dandan ; Ard Biesheuvel > ; Dong, Eric ; Laszlo Ersek > ; Gao, Liming ; Kinney, Michael > D ; Ni, Ray ; Wu, Hao A > ; Yao, Jiewen > Subject: RE: [edk2-devel] [PATCH V4 07/10] MdeModulePkg/Variable: Add > RT GetVariable() cache support > > The comments are for VariableRuntimeCache.c only. > > Regards, > Jian > > > -Original Message- > > From: devel@edk2.groups.io On Behalf Of Wang, > Jian > > J > > Sent: Wednesday, October 16, 2019 2:46 PM > > To: Kubacki, Michael A ; > devel@edk2.groups.io > > Cc: Bi, Dandan ; Ard Biesheuvel > > ; Dong, Eric ; Laszlo > Ersek > > ; Gao, Liming ; Kinney, > Michael D > > ; Ni, Ray ; Wu, Hao A > > ; Yao, Jiewen > > Subject: Re: [edk2-devel] [PATCH V4 07/10] MdeModulePkg/Variable: Add > RT > > GetVariable() cache support > > > > Hi Michael, > > > > Please see my inline comments. > > > > > -Original Message- > > > From: Kubacki, Michael A > > > Sent: Tuesday, October 15, 2019 7:30 AM > > > To: devel@edk2.groups.io > > > Cc: Bi, Dandan ; Ard Biesheuvel > > > ; Dong, Eric ; Laszlo > Ersek > > > ; Gao, Liming ; Kinney, > Michael > > D > > > ; Ni, Ray ; Wang, Jian J > > > ; Wu, Hao A ; Yao, > Jiewen > > > > > > Subject: [PATCH V4 07/10] MdeModulePkg/Variable: Add RT > GetVariable() > > cache > > > support > > > > > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2220 > > > > > > This change reduces SMIs for GetVariable () by maintaining a > > > UEFI variable cache in Runtime DXE in addition to the pre- > > > existing cache in SMRAM. When the Runtime Service GetVariable() > > > is invoked, a Runtime DXE cache is used instead of triggering an > > > SMI to VariableSmm. This can improve overall system performance > > > by servicing variable read requests without rendezvousing all > > > cores into SMM. > > > > > > The runtime cache can be disabled with by setting the FeaturePCD > > > gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache > > > to FALSE. If the PCD is set to FALSE, the runtime cache will not be > > > used and an SMI will be triggered for Runtime Service > > > GetVariable () and GetNextVariableName () invocations. > > > > > > The following are important points regarding the behavior of the > > > variable drivers when the variable runtime cache is enabled. > > > > > > 1. All of the non-volatile storage contents are loaded into the > > >cache upon driver load. This one time load operation from storage > > >is preferred as opposed to building the cache on demand. An on- > > >demand cache would require a fallback SMI to load data into the > > >cache as variables are requested. > > > > > > 2. SetVariable () requests will continue to always trigger an SMI. > > >This occurs regardless of whether the variable is volatile or > > >non-volatile. > > > >
Re: [edk2-devel] [PATCH V4 07/10] MdeModulePkg/Variable: Add RT GetVariable() cache support
AtRuntime () is implemented in VariableSmmRuntimeDxe.c because it is called by VariableParsing.c which is linked to both VariableSmmRuntimeDxe and VariableSmm. VariableSmm cannot directly invoke EfiAtRuntime (), so prior to this change, VariableSmm stores the runtime status in the global variable mAtRuntime which is returned via AtRuntime (). So this is implemented to allow VariableParsing.c to be linked to both drivers. Thanks, Michael > -Original Message- > From: Wu, Hao A > Sent: Wednesday, October 16, 2019 12:57 AM > To: Kubacki, Michael A ; > devel@edk2.groups.io > Cc: Bi, Dandan ; Ard Biesheuvel > ; Dong, Eric ; Laszlo Ersek > ; Gao, Liming ; Kinney, Michael > D ; Ni, Ray ; Wang, Jian J > ; Yao, Jiewen > Subject: RE: [PATCH V4 07/10] MdeModulePkg/Variable: Add RT > GetVariable() cache support > > Again, I would like to ask for help from other reviewers to look at this patch > (patch 7/10) and the next one (patch 8/10) (at least from the security > perspective). Any help will be appreciated, thanks in advance. > > > One comment inherited from the feedback on the V2 series: > I saw AtRuntime() is still being added in file VariableSmmRuntimeDxe.c, could > you help to double confirm? > > Another general level comment is that: > Please help to update the MdeModulePkg.uni file as well for the introduce > of > the new PCD. > > Inline comments below: > > > > -Original Message- > > From: Kubacki, Michael A > > Sent: Tuesday, October 15, 2019 7:30 AM > > To: devel@edk2.groups.io > > Cc: Bi, Dandan; Ard Biesheuvel; Dong, Eric; Laszlo Ersek; Gao, Liming; > Kinney, > > Michael D; Ni, Ray; Wang, Jian J; Wu, Hao A; Yao, Jiewen > > Subject: [PATCH V4 07/10] MdeModulePkg/Variable: Add RT GetVariable() > > cache support > > > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2220 > > > > This change reduces SMIs for GetVariable () by maintaining a > > UEFI variable cache in Runtime DXE in addition to the pre- > > existing cache in SMRAM. When the Runtime Service GetVariable() > > is invoked, a Runtime DXE cache is used instead of triggering an > > SMI to VariableSmm. This can improve overall system performance > > by servicing variable read requests without rendezvousing all > > cores into SMM. > > > > The runtime cache can be disabled with by setting the FeaturePCD > > gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache > > to FALSE. If the PCD is set to FALSE, the runtime cache will not be > > used and an SMI will be triggered for Runtime Service > > GetVariable () and GetNextVariableName () invocations. > > > > The following are important points regarding the behavior of the > > variable drivers when the variable runtime cache is enabled. > > > > 1. All of the non-volatile storage contents are loaded into the > >cache upon driver load. This one time load operation from storage > >is preferred as opposed to building the cache on demand. An on- > >demand cache would require a fallback SMI to load data into the > >cache as variables are requested. > > > > 2. SetVariable () requests will continue to always trigger an SMI. > >This occurs regardless of whether the variable is volatile or > >non-volatile. > > > > 3. Both volatile and non-volatile variables are cached in a runtime > >buffer. As is the case in the current EDK II variable driver, they > >continue to be cached in separate buffers. > > > > 4. The cache in Runtime DXE and SMM are intended to be exact copies > >of one another. All SMM variable accesses only return data from the > >SMM cache. The runtime caches are only updated after the variable I/O > >operation is successful in SMM. The runtime caches are only updated > >from SMM. > > > > 5. Synchronization mechanisms are in place to ensure the runtime cache > >content integrity with the SMM cache. These may result in updates to > >runtime cache that are the same in content but different in offset and > >size from updates to the SMM cache. > > > > When using SMM variables with runtime cache enabled, two caches will > now > > be present. > > 1. "Runtime Cache" - Maintained in VariableSmmRuntimeDxe. Used to > > service > >Runtime Services GetVariable () and GetNextVariableName () callers. > > 2. "SMM Cache" - Maintained in VariableSmm to service SMM GetVariable > () > >and GetNextVariableName () callers. > >a. This cache is retained so SMM modules do not operate on data outside > > SMRAM. > > > > Because a race condition can occur if an SMI occurs during the execution > > of runtime code reading from the runtime cache, a runtime cache read lock > > is introduced that explicitly moves pending updates from SMM to the > > runtime > > cache if an SMM update occurs while the runtime cache is locked. Note > that > > it is not expected a Runtime services call will interrupt SMM processing > > since all CPU cores rendezvous in SMM. > > > > It is possible to view UEFI variable read and write
Re: [edk2-devel] [PATCH V4 07/10] MdeModulePkg/Variable: Add RT GetVariable() cache support
Again, I would like to ask for help from other reviewers to look at this patch (patch 7/10) and the next one (patch 8/10) (at least from the security perspective). Any help will be appreciated, thanks in advance. One comment inherited from the feedback on the V2 series: I saw AtRuntime() is still being added in file VariableSmmRuntimeDxe.c, could you help to double confirm? Another general level comment is that: Please help to update the MdeModulePkg.uni file as well for the introduce of the new PCD. Inline comments below: > -Original Message- > From: Kubacki, Michael A > Sent: Tuesday, October 15, 2019 7:30 AM > To: devel@edk2.groups.io > Cc: Bi, Dandan; Ard Biesheuvel; Dong, Eric; Laszlo Ersek; Gao, Liming; Kinney, > Michael D; Ni, Ray; Wang, Jian J; Wu, Hao A; Yao, Jiewen > Subject: [PATCH V4 07/10] MdeModulePkg/Variable: Add RT GetVariable() > cache support > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2220 > > This change reduces SMIs for GetVariable () by maintaining a > UEFI variable cache in Runtime DXE in addition to the pre- > existing cache in SMRAM. When the Runtime Service GetVariable() > is invoked, a Runtime DXE cache is used instead of triggering an > SMI to VariableSmm. This can improve overall system performance > by servicing variable read requests without rendezvousing all > cores into SMM. > > The runtime cache can be disabled with by setting the FeaturePCD > gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache > to FALSE. If the PCD is set to FALSE, the runtime cache will not be > used and an SMI will be triggered for Runtime Service > GetVariable () and GetNextVariableName () invocations. > > The following are important points regarding the behavior of the > variable drivers when the variable runtime cache is enabled. > > 1. All of the non-volatile storage contents are loaded into the >cache upon driver load. This one time load operation from storage >is preferred as opposed to building the cache on demand. An on- >demand cache would require a fallback SMI to load data into the >cache as variables are requested. > > 2. SetVariable () requests will continue to always trigger an SMI. >This occurs regardless of whether the variable is volatile or >non-volatile. > > 3. Both volatile and non-volatile variables are cached in a runtime >buffer. As is the case in the current EDK II variable driver, they >continue to be cached in separate buffers. > > 4. The cache in Runtime DXE and SMM are intended to be exact copies >of one another. All SMM variable accesses only return data from the >SMM cache. The runtime caches are only updated after the variable I/O >operation is successful in SMM. The runtime caches are only updated >from SMM. > > 5. Synchronization mechanisms are in place to ensure the runtime cache >content integrity with the SMM cache. These may result in updates to >runtime cache that are the same in content but different in offset and >size from updates to the SMM cache. > > When using SMM variables with runtime cache enabled, two caches will now > be present. > 1. "Runtime Cache" - Maintained in VariableSmmRuntimeDxe. Used to > service >Runtime Services GetVariable () and GetNextVariableName () callers. > 2. "SMM Cache" - Maintained in VariableSmm to service SMM GetVariable () >and GetNextVariableName () callers. >a. This cache is retained so SMM modules do not operate on data outside > SMRAM. > > Because a race condition can occur if an SMI occurs during the execution > of runtime code reading from the runtime cache, a runtime cache read lock > is introduced that explicitly moves pending updates from SMM to the > runtime > cache if an SMM update occurs while the runtime cache is locked. Note that > it is not expected a Runtime services call will interrupt SMM processing > since all CPU cores rendezvous in SMM. > > It is possible to view UEFI variable read and write statistics by setting > the gEfiMdeModulePkgTokenSpaceGuid.PcdVariableCollectStatistics > FeaturePcd > to TRUE and using the VariableInfo UEFI application in MdeModulePkg to > dump > variable statistics to the console. By doing so, a user can view the number > of GetVariable () hits from the Runtime DXE variable driver (Runtime Cache > hits) and the SMM variable driver (SMM Cache hits). SMM Cache hits for > GetVariable () will occur when SMM modules invoke GetVariable (). > > Cc: Dandan Bi > Cc: Ard Biesheuvel > Cc: Eric Dong > Cc: Laszlo Ersek > Cc: Liming Gao > Cc: Michael D Kinney > Cc: Ray Ni > Cc: Jian J Wang > Cc: Hao A Wu > Cc: Jiewen Yao > Signed-off-by: Michael Kubacki > --- > MdeModulePkg/MdeModulePkg.dec| 12 + > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf > | 2 + > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf | 2 > + > >
Re: [edk2-devel] [PATCH V4 07/10] MdeModulePkg/Variable: Add RT GetVariable() cache support
The comments are for VariableRuntimeCache.c only. Regards, Jian > -Original Message- > From: devel@edk2.groups.io On Behalf Of Wang, Jian > J > Sent: Wednesday, October 16, 2019 2:46 PM > To: Kubacki, Michael A ; devel@edk2.groups.io > Cc: Bi, Dandan ; Ard Biesheuvel > ; Dong, Eric ; Laszlo Ersek > ; Gao, Liming ; Kinney, Michael D > ; Ni, Ray ; Wu, Hao A > ; Yao, Jiewen > Subject: Re: [edk2-devel] [PATCH V4 07/10] MdeModulePkg/Variable: Add RT > GetVariable() cache support > > Hi Michael, > > Please see my inline comments. > > > -Original Message- > > From: Kubacki, Michael A > > Sent: Tuesday, October 15, 2019 7:30 AM > > To: devel@edk2.groups.io > > Cc: Bi, Dandan ; Ard Biesheuvel > > ; Dong, Eric ; Laszlo Ersek > > ; Gao, Liming ; Kinney, Michael > D > > ; Ni, Ray ; Wang, Jian J > > ; Wu, Hao A ; Yao, Jiewen > > > > Subject: [PATCH V4 07/10] MdeModulePkg/Variable: Add RT GetVariable() > cache > > support > > > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2220 > > > > This change reduces SMIs for GetVariable () by maintaining a > > UEFI variable cache in Runtime DXE in addition to the pre- > > existing cache in SMRAM. When the Runtime Service GetVariable() > > is invoked, a Runtime DXE cache is used instead of triggering an > > SMI to VariableSmm. This can improve overall system performance > > by servicing variable read requests without rendezvousing all > > cores into SMM. > > > > The runtime cache can be disabled with by setting the FeaturePCD > > gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache > > to FALSE. If the PCD is set to FALSE, the runtime cache will not be > > used and an SMI will be triggered for Runtime Service > > GetVariable () and GetNextVariableName () invocations. > > > > The following are important points regarding the behavior of the > > variable drivers when the variable runtime cache is enabled. > > > > 1. All of the non-volatile storage contents are loaded into the > >cache upon driver load. This one time load operation from storage > >is preferred as opposed to building the cache on demand. An on- > >demand cache would require a fallback SMI to load data into the > >cache as variables are requested. > > > > 2. SetVariable () requests will continue to always trigger an SMI. > >This occurs regardless of whether the variable is volatile or > >non-volatile. > > > > 3. Both volatile and non-volatile variables are cached in a runtime > >buffer. As is the case in the current EDK II variable driver, they > >continue to be cached in separate buffers. > > > > 4. The cache in Runtime DXE and SMM are intended to be exact copies > >of one another. All SMM variable accesses only return data from the > >SMM cache. The runtime caches are only updated after the variable I/O > >operation is successful in SMM. The runtime caches are only updated > >from SMM. > > > > 5. Synchronization mechanisms are in place to ensure the runtime cache > >content integrity with the SMM cache. These may result in updates to > >runtime cache that are the same in content but different in offset and > >size from updates to the SMM cache. > > > > When using SMM variables with runtime cache enabled, two caches will now > > be present. > > 1. "Runtime Cache" - Maintained in VariableSmmRuntimeDxe. Used to service > >Runtime Services GetVariable () and GetNextVariableName () callers. > > 2. "SMM Cache" - Maintained in VariableSmm to service SMM GetVariable () > >and GetNextVariableName () callers. > >a. This cache is retained so SMM modules do not operate on data outside > > SMRAM. > > > > Because a race condition can occur if an SMI occurs during the execution > > of runtime code reading from the runtime cache, a runtime cache read lock > > is introduced that explicitly moves pending updates from SMM to the runtime > > cache if an SMM update occurs while the runtime cache is locked. Note that > > it is not expected a Runtime services call will interrupt SMM processing > > since all CPU cores rendezvous in SMM. > > > > It is possible to view UEFI variable read and write statistics by setting > > the gEfiMdeModulePkgTokenSpaceGuid.PcdVariableCollectStatistics > FeaturePcd > > to TRUE and using the VariableInfo UEFI application in MdeModulePkg to > dump > > variable statistics to the console. By doing so, a user can view the number > > o
Re: [edk2-devel] [PATCH V4 07/10] MdeModulePkg/Variable: Add RT GetVariable() cache support
Hi Michael, Please see my inline comments. > -Original Message- > From: Kubacki, Michael A > Sent: Tuesday, October 15, 2019 7:30 AM > To: devel@edk2.groups.io > Cc: Bi, Dandan ; Ard Biesheuvel > ; Dong, Eric ; Laszlo Ersek > ; Gao, Liming ; Kinney, Michael D > ; Ni, Ray ; Wang, Jian J > ; Wu, Hao A ; Yao, Jiewen > > Subject: [PATCH V4 07/10] MdeModulePkg/Variable: Add RT GetVariable() cache > support > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2220 > > This change reduces SMIs for GetVariable () by maintaining a > UEFI variable cache in Runtime DXE in addition to the pre- > existing cache in SMRAM. When the Runtime Service GetVariable() > is invoked, a Runtime DXE cache is used instead of triggering an > SMI to VariableSmm. This can improve overall system performance > by servicing variable read requests without rendezvousing all > cores into SMM. > > The runtime cache can be disabled with by setting the FeaturePCD > gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache > to FALSE. If the PCD is set to FALSE, the runtime cache will not be > used and an SMI will be triggered for Runtime Service > GetVariable () and GetNextVariableName () invocations. > > The following are important points regarding the behavior of the > variable drivers when the variable runtime cache is enabled. > > 1. All of the non-volatile storage contents are loaded into the >cache upon driver load. This one time load operation from storage >is preferred as opposed to building the cache on demand. An on- >demand cache would require a fallback SMI to load data into the >cache as variables are requested. > > 2. SetVariable () requests will continue to always trigger an SMI. >This occurs regardless of whether the variable is volatile or >non-volatile. > > 3. Both volatile and non-volatile variables are cached in a runtime >buffer. As is the case in the current EDK II variable driver, they >continue to be cached in separate buffers. > > 4. The cache in Runtime DXE and SMM are intended to be exact copies >of one another. All SMM variable accesses only return data from the >SMM cache. The runtime caches are only updated after the variable I/O >operation is successful in SMM. The runtime caches are only updated >from SMM. > > 5. Synchronization mechanisms are in place to ensure the runtime cache >content integrity with the SMM cache. These may result in updates to >runtime cache that are the same in content but different in offset and >size from updates to the SMM cache. > > When using SMM variables with runtime cache enabled, two caches will now > be present. > 1. "Runtime Cache" - Maintained in VariableSmmRuntimeDxe. Used to service >Runtime Services GetVariable () and GetNextVariableName () callers. > 2. "SMM Cache" - Maintained in VariableSmm to service SMM GetVariable () >and GetNextVariableName () callers. >a. This cache is retained so SMM modules do not operate on data outside > SMRAM. > > Because a race condition can occur if an SMI occurs during the execution > of runtime code reading from the runtime cache, a runtime cache read lock > is introduced that explicitly moves pending updates from SMM to the runtime > cache if an SMM update occurs while the runtime cache is locked. Note that > it is not expected a Runtime services call will interrupt SMM processing > since all CPU cores rendezvous in SMM. > > It is possible to view UEFI variable read and write statistics by setting > the gEfiMdeModulePkgTokenSpaceGuid.PcdVariableCollectStatistics FeaturePcd > to TRUE and using the VariableInfo UEFI application in MdeModulePkg to dump > variable statistics to the console. By doing so, a user can view the number > of GetVariable () hits from the Runtime DXE variable driver (Runtime Cache > hits) and the SMM variable driver (SMM Cache hits). SMM Cache hits for > GetVariable () will occur when SMM modules invoke GetVariable (). > > Cc: Dandan Bi > Cc: Ard Biesheuvel > Cc: Eric Dong > Cc: Laszlo Ersek > Cc: Liming Gao > Cc: Michael D Kinney > Cc: Ray Ni > Cc: Jian J Wang > Cc: Hao A Wu > Cc: Jiewen Yao > Signed-off-by: Michael Kubacki > --- > MdeModulePkg/MdeModulePkg.dec| 12 + > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf| 2 > + > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf | 2 + > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf | > 20 +- > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf | > 2 + > MdeModulePkg/Include/Guid/SmmVariableCommon.h| 29 +- > MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h| 32 +- > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeCache.h| > 51 ++ > MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c| 50 +- >
