Hi Jiewen, my colleague Ladi (CC'd) reported an issue about MORLock in OVMF (and also analyzed it in great depth):
https://bugzilla.redhat.com/show_bug.cgi?id=1496170 Here's my understanding of the "MemoryOverwriteRequestControl" and "MemoryOverwriteRequestControlLock" variables: (1) The "MemoryOverwriteRequestControl" variable comes from the "TCG Platform Reset Attack Mitigation Specification" at <https://www.trustedcomputinggroup.org/wp-content/uploads/Platform-Reset-Attack-Mitigation-Specification.pdf>. (2) The "MemoryOverwriteRequestControlLock" variable is a Microsoft-only addition, called "Secure MOR implementation", from <https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/device-guard-requirements>. (3) "winload.efi" accepts the following cases as "valid": (3a) Both MORC and MORCL are present. In this case MORC is set and MORCL is used to lock both MORC and MORCL. (3b) MORC is present, MORCL is absent. In this case "winload.efi" thinks that the platform follows the TCG spec from (1), and does not support the Microsoft spec from (2). MORC is set, but it is not locked. (3c) Both MORC and MORCL are missing. "winload.efi" thinks that the platform does not know about either spec, and "winload.efi" accepts that as OK. However "winload.efi"rejects the following case as "invalid": (3d) MORCL is present, but MORC is missing. In my opinion, "winload.efi" is right to reject this, because it implies that the platform supports the *dependent* spec (2), but does not support the *prerequisite* spec (1). In edk2, MORC is implemented by the "SecurityPkg/Tcg/MemoryOverwriteControl/TcgMor.inf" driver. A platform is allowed *not* to include this driver, even if it supports SMM. OVMF is such a platform; it does not include "TcgMor.inf". However, MORCL is initialized in the variable driver, independently of platform support for MORC: * If the platform includes the SMM driver backend, then we have: VariableWriteServiceInitialize() [MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c] MorLockInit() [MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c] SetMorLockVariable() [MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c] // // create or set variable // with contents 0 // * If the platform includes no SMM backend for the variable services, then we have: VariableWriteServiceInitialize() [MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c] MorLockInit() [MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c] // // delete the variable and // also lock it so that // nothing else can // create it // In my opinion, if the platform includes the SMM backend, but does not support "MORC", then the "TcgMorLockSmm.c" code is incorrect; instead we should do what we see in "TcgMorLockDxe.c". In other words, SMM support in itself is no indication for claiming support for MORCL. I suggest that we introduce a new PCD for this ("PcdSupportMemoryOverwriteRequest"), in "SecurityPkg.dec", with type UINT8. It should be a bitmap: - BIT0 -- If clear, then MORC is not supported, and BIT1 is ignored. Otherwise, MORC is supported, and BIT1 is meaningful. - BIT1 -- If clear, then MORCL is unsupported. If set, then MORCL is supported. - BIT2 through BIT7 are reserved. In turn, here's how the new PCD should be handled, in my opinion: - MorLockInit() in "TcgMorLockDxe.c" should do what it does now, but it should also assert that BIT1 is clear. - MorLockInit() in "TcgMorLockSmm.c" should do what it does now *only* if BIT1 is set. Otherwise (i.e., BIT1 is clear), it should do what "TcgMorLockDxe.c" does now. - If BIT0 is clear (MORC is not supported), then both MorLockInit() functions should delete, and lock, the MORC variable too, so that nothing else can create it. What do you think? Thank you! Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
