Re: multiple levels of support for MOR / MORLock

["Yao, Jiewen" <jiewen.yao@intel.com>]

Thanks Laszlo.

Yes, I agree it is bug. Would you please help to file a bugzilar in EDKII?

For the fix, I think we have a way to resolve it without PCD. (I do not want to bother a platform developer to set a new PCD.)

The only invalid case we need handle is: MOR is absent, but MORL is present.

My thought is to let Variable driver check if MOR is present. Variable driver can defer the MORL setting at EndOfDxe event based upon the presence of MOR. If MOR driver is present, it sets MOR at entrypoint. EndOfDxe is good enough to know the state.

Also, because EndOfDxe is PI event, the UEFI OS is not aware of that.

Thank you
Yao Jiewen


> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, September 28, 2017 8:55 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: Ladi Prosek <lprosek@redhat.com>; edk2-devel-01 <edk2-devel@lists.01.org>
> Subject: multiple levels of support for MOR / MORLock
> 
> 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