Re: 回复: [edk2-devel] [PATCH v1 1/1] MdeModulePkg/Variable: TcgMorLockSmm Key Mismatch changes lock state

[Michael Kubacki]

On 6/24/2023 9:33 PM, gaoliming via groups.io wrote:

Abhi:
   Sorry for the missing patch. I agree Michael comment. Can you help update the 
patch? If yes, you can add my Reviewed-by: Liming Gao <gaolim...@byosoft.com.cn>

Thanks
Liming

-----邮件原件-----
发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Michael
Kubacki
发送时间: 2023年6月9日 4:58
收件人: devel@edk2.groups.io; abhi.si...@arm.com
主题: Re: [edk2-devel] [PATCH v1 1/1] MdeModulePkg/Variable:
TcgMorLockSmm Key Mismatch changes lock state

Acked-by: Michael Kubacki <michael.kuba...@microsoft.com>

Inline code comment below.

On 4/12/2023 5:25 PM, Abhimanyu Singh wrote:

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4410

Inside TcgMorLockSmm.c, the SetVariableCheckHandlerMorLock() function
contains a scenario to prevent a possible dictionary attack on the MorLock
Key in accordance with the TCG Platform Reset Mitigation Spec v1.10.

The mechanism to prevent this attack must also change the MorLock

Variable

Value to 0x01 to indicate Locked Without Key.

Cc: Jian J Wang <jian.j.w...@intel.com>
Cc: Liming Gao <gaolim...@byosoft.com.cn>
Signed-off-by: Abhi Singh <abhi.si...@arm.com>
---
   MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c | 4

++++

   1 file changed, 4 insertions(+)

diff --git

a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c

index da1105ff073e..a76db18ef877 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
@@ -312,6 +312,10 @@ SetVariableCheckHandlerMorLock (
         mMorLockState    = MorLockStateLocked;

         mMorLockKeyEmpty = TRUE;

         ZeroMem (mMorLockKey, sizeof (mMorLockKey));

+      //

+      // Update value to reflect locked without key

+      //

+      SetMorLockVariable (MOR_LOCK_DATA_LOCKED_WITHOUT_KEY);

I know the TCG Reset Attack Mitigation Specification requires
EFI_ACCESS_DENIED to be returned from this function in this case but
SetMorLockVariable() returns a status code.

I suggest capturing that followed by an ASSERT_EFI_ERROR (Status) to at
least help raise visibility of unexpected errors in builds with asserts
enabled.

Do you mean ASSERT_EFI_ERROR (Status) return from SetMorLockVariable () API?
I agree this suggestion.

Correct

Thanks
Liming

         return EFI_ACCESS_DENIED;

       }

     }

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106407): https://edk2.groups.io/g/devel/message/106407
Mute This Topic: https://groups.io/mt/99763325/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-