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

["Michael Kubacki" <mikuback@linux.microsoft.com>]

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 <gaoliming@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.Singh@arm.com
>> 主题: Re: [edk2-devel] [PATCH v1 1/1] MdeModulePkg/Variable:
>> TcgMorLockSmm Key Mismatch changes lock state
>>
>> Acked-by: Michael Kubacki <michael.kubacki@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.wang@intel.com>
>>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>>> Signed-off-by: Abhi Singh <Abhi.Singh@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;
>>>
>>>        }
>>>
>>>      }
>>>
>>
>>
>>
>>
> 
> 
> 
> 
> 
> 
>