* [PATCH] MdeModulePkg/Variable/RuntimeDxe: delete & lock MOR in the absence of SMM
@ 2017-10-10 13:24 Laszlo Ersek
2017-10-10 13:54 ` Zeng, Star
0 siblings, 1 reply; 4+ messages in thread
From: Laszlo Ersek @ 2017-10-10 13:24 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Eric Dong, Jiewen Yao, Star Zeng
VariableRuntimeDxe deletes and locks the MorLock variable in
MorLockInit(), with the argument that any protection provided by MorLock
can be circumvented if MorLock can be overwritten by unprivileged code
(i.e., outside of SMM).
Extend the argument and the logic to the MOR variable, which is supposed
to be protected by MorLock.
This change was suggested by Star; it is inspired by earlier VariableSmm
commit fda8f631edbb ("MdeModulePkg/Variable/RuntimeDxe: delete and lock
OS-created MOR variable", 2017-10-03).
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Suggested-by: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
Notes:
Repo: https://github.com/lersek/edk2.git
Branch: del_and_lock_mor_without_smm
MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c | 24 ++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
index 7142e2da2073..1610c7aa0706 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
@@ -69,29 +69,53 @@ EFI_STATUS
MorLockInit (
VOID
)
{
//
// Always clear variable to report unsupported to OS.
// The reason is that the DXE version is not proper to provide *protection*.
// BIOS should use SMM version variable driver to provide such capability.
//
VariableServiceSetVariable (
MEMORY_OVERWRITE_REQUEST_CONTROL_LOCK_NAME,
&gEfiMemoryOverwriteRequestControlLockGuid,
EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
0,
NULL
);
//
// Need set this variable to be read-only to prevent other module set it.
//
VariableLockRequestToLock (&mVariableLock, MEMORY_OVERWRITE_REQUEST_CONTROL_LOCK_NAME, &gEfiMemoryOverwriteRequestControlLockGuid);
+
+ //
+ // The MOR variable can effectively improve platform security only when the
+ // MorLock variable protects the MOR variable. In turn MorLock cannot be made
+ // secure without SMM support in the platform firmware (see above).
+ //
+ // Thus, delete the MOR variable, should it exist for any reason (some OSes
+ // are known to create MOR unintentionally, in an attempt to set it), then
+ // also lock the MOR variable, in order to prevent other modules from
+ // creating it.
+ //
+ VariableServiceSetVariable (
+ MEMORY_OVERWRITE_REQUEST_VARIABLE_NAME,
+ &gEfiMemoryOverwriteControlDataGuid,
+ 0, // Attributes
+ 0, // DataSize
+ NULL // Data
+ );
+ VariableLockRequestToLock (
+ &mVariableLock,
+ MEMORY_OVERWRITE_REQUEST_VARIABLE_NAME,
+ &gEfiMemoryOverwriteControlDataGuid
+ );
+
return EFI_SUCCESS;
}
/**
Delayed initialization for MOR Control Lock at EndOfDxe.
This function performs any operations queued by MorLockInit().
**/
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] MdeModulePkg/Variable/RuntimeDxe: delete & lock MOR in the absence of SMM 2017-10-10 13:24 [PATCH] MdeModulePkg/Variable/RuntimeDxe: delete & lock MOR in the absence of SMM Laszlo Ersek @ 2017-10-10 13:54 ` Zeng, Star 2017-10-10 17:22 ` Laszlo Ersek 0 siblings, 1 reply; 4+ messages in thread From: Zeng, Star @ 2017-10-10 13:54 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel-01; +Cc: Dong, Eric, Yao, Jiewen, Zeng, Star Could you help make the code consistent to call VariableServiceSetVariable() for the Attributes? One original for MorLock is using real attributes, the new you added for Mor will use 0 attributes. How about to both use 0 attributes. Another question, why empty MorLockInitAtEndOfDxe() needs to be implemented in TcgMorLockDxe.c and called in VariableDxe.c? Thanks, Star -----Original Message----- From: Laszlo Ersek [mailto:lersek@redhat.com] Sent: Tuesday, October 10, 2017 9:25 PM To: edk2-devel-01 <edk2-devel@lists.01.org> Cc: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com> Subject: [PATCH] MdeModulePkg/Variable/RuntimeDxe: delete & lock MOR in the absence of SMM VariableRuntimeDxe deletes and locks the MorLock variable in MorLockInit(), with the argument that any protection provided by MorLock can be circumvented if MorLock can be overwritten by unprivileged code (i.e., outside of SMM). Extend the argument and the logic to the MOR variable, which is supposed to be protected by MorLock. This change was suggested by Star; it is inspired by earlier VariableSmm commit fda8f631edbb ("MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR variable", 2017-10-03). Cc: Eric Dong <eric.dong@intel.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Star Zeng <star.zeng@intel.com> Suggested-by: Star Zeng <star.zeng@intel.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- Notes: Repo: https://github.com/lersek/edk2.git Branch: del_and_lock_mor_without_smm MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c | 24 ++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c index 7142e2da2073..1610c7aa0706 100644 --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c @@ -69,29 +69,53 @@ EFI_STATUS MorLockInit ( VOID ) { // // Always clear variable to report unsupported to OS. // The reason is that the DXE version is not proper to provide *protection*. // BIOS should use SMM version variable driver to provide such capability. // VariableServiceSetVariable ( MEMORY_OVERWRITE_REQUEST_CONTROL_LOCK_NAME, &gEfiMemoryOverwriteRequestControlLockGuid, EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS, 0, NULL ); // // Need set this variable to be read-only to prevent other module set it. // VariableLockRequestToLock (&mVariableLock, MEMORY_OVERWRITE_REQUEST_CONTROL_LOCK_NAME, &gEfiMemoryOverwriteRequestControlLockGuid); + + // + // The MOR variable can effectively improve platform security only when the + // MorLock variable protects the MOR variable. In turn MorLock cannot be made + // secure without SMM support in the platform firmware (see above). + // + // Thus, delete the MOR variable, should it exist for any reason (some OSes + // are known to create MOR unintentionally, in an attempt to set it), then + // also lock the MOR variable, in order to prevent other modules from + // creating it. + // + VariableServiceSetVariable ( + MEMORY_OVERWRITE_REQUEST_VARIABLE_NAME, + &gEfiMemoryOverwriteControlDataGuid, + 0, // Attributes + 0, // DataSize + NULL // Data + ); + VariableLockRequestToLock ( + &mVariableLock, + MEMORY_OVERWRITE_REQUEST_VARIABLE_NAME, + &gEfiMemoryOverwriteControlDataGuid + ); + return EFI_SUCCESS; } /** Delayed initialization for MOR Control Lock at EndOfDxe. This function performs any operations queued by MorLockInit(). **/ -- 2.14.1.3.gb7cf6e02401b ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] MdeModulePkg/Variable/RuntimeDxe: delete & lock MOR in the absence of SMM 2017-10-10 13:54 ` Zeng, Star @ 2017-10-10 17:22 ` Laszlo Ersek 2017-10-24 2:19 ` Zeng, Star 0 siblings, 1 reply; 4+ messages in thread From: Laszlo Ersek @ 2017-10-10 17:22 UTC (permalink / raw) To: Zeng, Star, edk2-devel-01; +Cc: Dong, Eric, Yao, Jiewen On 10/10/17 15:54, Zeng, Star wrote: > Could you help make the code consistent to call > VariableServiceSetVariable() for the Attributes? One original for > MorLock is using real attributes, the new you added for Mor will use 0 > attributes. How about to both use 0 attributes. Sure, I can do that -- do you want me to set the attributes to 0 for the MorLock deletion in the same patch, or should I do it in a separate patch? > Another question, why empty MorLockInitAtEndOfDxe() needs to be > implemented in TcgMorLockDxe.c and called in VariableDxe.c? Because otherwise we would break the promise we make in "PrivilegePolymorphic.h": > Polymorphic functions that are called from both the privileged driver (i.e., > the DXE_SMM variable module) and the non-privileged drivers (i.e., one or > both of the DXE_RUNTIME variable modules). > > Each of these functions has two implementations, appropriate for privileged > vs. non-privileged driver code. If I don't implement MorLockInitAtEndOfDxe() for VariableRuntimeDxe, then we'll have a declaration with no definition in VariableRuntimeDxe, and only one definition in total. The empty function call should be optimized out in RELEASE builds anyway (assuming LTO is used). It's easy to remove the empty definition and the calls to it, but then the purpose of "PrivilegePolymorphic.h" becomes less clear. For complete clarity, I would have had to introduce *two* new header files: - one for SecureBootHook(), MorLockInit() and SetVariableCheckHandlerMor() -- to be used by both SMM and non-SMM, - and another header file for just MorLockInitAtEndOfDxe() -- to be used only by SMM. (This is necessary because the call is made from "VariableSmm.c" to "TcgMorLockSmm.c"). I thought that introducing two new header files would not be accepted, so I opted for one header file only. But then I felt that the empty MorLockInitAtEndOfDxe() hook should be correctly implemented for VariableRuntimeDxe too. Thanks, Laszlo > > > Thanks, > Star > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Tuesday, October 10, 2017 9:25 PM > To: edk2-devel-01 <edk2-devel@lists.01.org> > Cc: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com> > Subject: [PATCH] MdeModulePkg/Variable/RuntimeDxe: delete & lock MOR in the absence of SMM > > VariableRuntimeDxe deletes and locks the MorLock variable in > MorLockInit(), with the argument that any protection provided by MorLock > can be circumvented if MorLock can be overwritten by unprivileged code > (i.e., outside of SMM). > > Extend the argument and the logic to the MOR variable, which is supposed > to be protected by MorLock. > > This change was suggested by Star; it is inspired by earlier VariableSmm > commit fda8f631edbb ("MdeModulePkg/Variable/RuntimeDxe: delete and lock > OS-created MOR variable", 2017-10-03). > > Cc: Eric Dong <eric.dong@intel.com> > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Star Zeng <star.zeng@intel.com> > Suggested-by: Star Zeng <star.zeng@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > > Notes: > Repo: https://github.com/lersek/edk2.git > Branch: del_and_lock_mor_without_smm > > MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c | 24 ++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c > index 7142e2da2073..1610c7aa0706 100644 > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c > @@ -69,29 +69,53 @@ EFI_STATUS > MorLockInit ( > VOID > ) > { > // > // Always clear variable to report unsupported to OS. > // The reason is that the DXE version is not proper to provide *protection*. > // BIOS should use SMM version variable driver to provide such capability. > // > VariableServiceSetVariable ( > MEMORY_OVERWRITE_REQUEST_CONTROL_LOCK_NAME, > &gEfiMemoryOverwriteRequestControlLockGuid, > EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS, > 0, > NULL > ); > > // > // Need set this variable to be read-only to prevent other module set it. > // > VariableLockRequestToLock (&mVariableLock, MEMORY_OVERWRITE_REQUEST_CONTROL_LOCK_NAME, &gEfiMemoryOverwriteRequestControlLockGuid); > + > + // > + // The MOR variable can effectively improve platform security only when the > + // MorLock variable protects the MOR variable. In turn MorLock cannot be made > + // secure without SMM support in the platform firmware (see above). > + // > + // Thus, delete the MOR variable, should it exist for any reason (some OSes > + // are known to create MOR unintentionally, in an attempt to set it), then > + // also lock the MOR variable, in order to prevent other modules from > + // creating it. > + // > + VariableServiceSetVariable ( > + MEMORY_OVERWRITE_REQUEST_VARIABLE_NAME, > + &gEfiMemoryOverwriteControlDataGuid, > + 0, // Attributes > + 0, // DataSize > + NULL // Data > + ); > + VariableLockRequestToLock ( > + &mVariableLock, > + MEMORY_OVERWRITE_REQUEST_VARIABLE_NAME, > + &gEfiMemoryOverwriteControlDataGuid > + ); > + > return EFI_SUCCESS; > } > > /** > Delayed initialization for MOR Control Lock at EndOfDxe. > > This function performs any operations queued by MorLockInit(). > **/ > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] MdeModulePkg/Variable/RuntimeDxe: delete & lock MOR in the absence of SMM 2017-10-10 17:22 ` Laszlo Ersek @ 2017-10-24 2:19 ` Zeng, Star 0 siblings, 0 replies; 4+ messages in thread From: Zeng, Star @ 2017-10-24 2:19 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel-01; +Cc: Dong, Eric, Yao, Jiewen, Zeng, Star I prefer to set the attributes to 0 for the MorLock deletion in the same patch. It is fine to have empty MorLockInitAtEndOfDxe() in TcgMorLockDxe.c with your explanation. Thanks, Star -----Original Message----- From: Laszlo Ersek [mailto:lersek@redhat.com] Sent: Wednesday, October 11, 2017 1:22 AM To: Zeng, Star <star.zeng@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org> Cc: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com> Subject: Re: [PATCH] MdeModulePkg/Variable/RuntimeDxe: delete & lock MOR in the absence of SMM On 10/10/17 15:54, Zeng, Star wrote: > Could you help make the code consistent to call > VariableServiceSetVariable() for the Attributes? One original for > MorLock is using real attributes, the new you added for Mor will use 0 > attributes. How about to both use 0 attributes. Sure, I can do that -- do you want me to set the attributes to 0 for the MorLock deletion in the same patch, or should I do it in a separate patch? > Another question, why empty MorLockInitAtEndOfDxe() needs to be > implemented in TcgMorLockDxe.c and called in VariableDxe.c? Because otherwise we would break the promise we make in "PrivilegePolymorphic.h": > Polymorphic functions that are called from both the privileged driver (i.e., > the DXE_SMM variable module) and the non-privileged drivers (i.e., one or > both of the DXE_RUNTIME variable modules). > > Each of these functions has two implementations, appropriate for privileged > vs. non-privileged driver code. If I don't implement MorLockInitAtEndOfDxe() for VariableRuntimeDxe, then we'll have a declaration with no definition in VariableRuntimeDxe, and only one definition in total. The empty function call should be optimized out in RELEASE builds anyway (assuming LTO is used). It's easy to remove the empty definition and the calls to it, but then the purpose of "PrivilegePolymorphic.h" becomes less clear. For complete clarity, I would have had to introduce *two* new header files: - one for SecureBootHook(), MorLockInit() and SetVariableCheckHandlerMor() -- to be used by both SMM and non-SMM, - and another header file for just MorLockInitAtEndOfDxe() -- to be used only by SMM. (This is necessary because the call is made from "VariableSmm.c" to "TcgMorLockSmm.c"). I thought that introducing two new header files would not be accepted, so I opted for one header file only. But then I felt that the empty MorLockInitAtEndOfDxe() hook should be correctly implemented for VariableRuntimeDxe too. Thanks, Laszlo > > > Thanks, > Star > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Tuesday, October 10, 2017 9:25 PM > To: edk2-devel-01 <edk2-devel@lists.01.org> > Cc: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen > <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com> > Subject: [PATCH] MdeModulePkg/Variable/RuntimeDxe: delete & lock MOR > in the absence of SMM > > VariableRuntimeDxe deletes and locks the MorLock variable in > MorLockInit(), with the argument that any protection provided by > MorLock can be circumvented if MorLock can be overwritten by > unprivileged code (i.e., outside of SMM). > > Extend the argument and the logic to the MOR variable, which is > supposed to be protected by MorLock. > > This change was suggested by Star; it is inspired by earlier > VariableSmm commit fda8f631edbb ("MdeModulePkg/Variable/RuntimeDxe: > delete and lock OS-created MOR variable", 2017-10-03). > > Cc: Eric Dong <eric.dong@intel.com> > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Star Zeng <star.zeng@intel.com> > Suggested-by: Star Zeng <star.zeng@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > > Notes: > Repo: https://github.com/lersek/edk2.git > Branch: del_and_lock_mor_without_smm > > MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c | 24 > ++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git > a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c > b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c > index 7142e2da2073..1610c7aa0706 100644 > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c > @@ -69,29 +69,53 @@ EFI_STATUS > MorLockInit ( > VOID > ) > { > // > // Always clear variable to report unsupported to OS. > // The reason is that the DXE version is not proper to provide *protection*. > // BIOS should use SMM version variable driver to provide such capability. > // > VariableServiceSetVariable ( > MEMORY_OVERWRITE_REQUEST_CONTROL_LOCK_NAME, > &gEfiMemoryOverwriteRequestControlLockGuid, > EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS, > 0, > NULL > ); > > // > // Need set this variable to be read-only to prevent other module set it. > // > VariableLockRequestToLock (&mVariableLock, > MEMORY_OVERWRITE_REQUEST_CONTROL_LOCK_NAME, > &gEfiMemoryOverwriteRequestControlLockGuid); > + > + // > + // The MOR variable can effectively improve platform security only > + when the // MorLock variable protects the MOR variable. In turn > + MorLock cannot be made // secure without SMM support in the platform firmware (see above). > + // > + // Thus, delete the MOR variable, should it exist for any reason > + (some OSes // are known to create MOR unintentionally, in an > + attempt to set it), then // also lock the MOR variable, in order to > + prevent other modules from // creating it. > + // > + VariableServiceSetVariable ( > + MEMORY_OVERWRITE_REQUEST_VARIABLE_NAME, > + &gEfiMemoryOverwriteControlDataGuid, > + 0, // Attributes > + 0, // DataSize > + NULL // Data > + ); > + VariableLockRequestToLock ( > + &mVariableLock, > + MEMORY_OVERWRITE_REQUEST_VARIABLE_NAME, > + &gEfiMemoryOverwriteControlDataGuid > + ); > + > return EFI_SUCCESS; > } > > /** > Delayed initialization for MOR Control Lock at EndOfDxe. > > This function performs any operations queued by MorLockInit(). > **/ > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-10-24 2:15 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-10-10 13:24 [PATCH] MdeModulePkg/Variable/RuntimeDxe: delete & lock MOR in the absence of SMM Laszlo Ersek 2017-10-10 13:54 ` Zeng, Star 2017-10-10 17:22 ` Laszlo Ersek 2017-10-24 2:19 ` Zeng, Star
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox