[PATCH v1 1/1] MdeModulePkg/Variable: TcgMorLockSmm Key Mismatch changes lock state

[PATCH v1 1/1] MdeModulePkg/Variable: TcgMorLockSmm Key Mismatch changes lock state

[Abhimanyu Singh]

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);
       return EFI_ACCESS_DENIED;
     }
   }
-- 
2.34.1

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

[Abhimanyu Singh]

[-- Attachment #1: Type: text/plain, Size: 186 bytes --]

Working link to Bugzilla Ticket: 4410 – TcgMorLockSmm: Key Mismatch doesn't update the MorLock Variable Value. (tianocore.org) ( https://bugzilla.tianocore.org/show_bug.cgi?id=4410 )

[-- Attachment #2: Type: text/html, Size: 205 bytes --]

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

[Abhimanyu Singh]

[-- Attachment #1: Type: text/plain, Size: 168 bytes --]

Jian and Liming,

I was wondering if y'all could review this patch? It is a small change so hopefully there is not much downtime to review/test.

Thank you,
Abhi

[-- Attachment #2: Type: text/html, Size: 188 bytes --]

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

[Michael Kubacki]

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.

> 
>         return EFI_ACCESS_DENIED;
> 
>       }
> 
>     }
> 

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

[gaoliming]

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. 

Thanks
Liming
> >
> >         return EFI_ACCESS_DENIED;
> >
> >       }
> >
> >     }
> >
> 
> 
> 
> 

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 <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;
>>>
>>>        }
>>>
>>>      }
>>>
>>
>>
>>
>>
> 
> 
> 
> 
> 
> 
> 

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

[Abhimanyu Singh]

[-- Attachment #1: Type: text/plain, Size: 139 bytes --]

Hi Liming and Michael,

I have added the requested change, here is the PR: https://github.com/tianocore/edk2/pull/4546

Thanks,
Abhi

[-- Attachment #2: Type: text/html, Size: 222 bytes --]

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

[gaoliming]

[-- Attachment #1: Type: text/plain, Size: 552 bytes --]

Abhi:

 I just approve it. It will be merged soon.

 

Thanks

Liming

发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Abhimanyu Singh
发送时间: 2023年7月10日 7:14
收件人: gaoliming <gaoliming@byosoft.com.cn>; devel@edk2.groups.io
主题: Re: [edk2-devel] 回复: [edk2-devel] [PATCH v1 1/1] MdeModulePkg/Variable: TcgMorLockSmm Key Mismatch changes lock state

 

Hi Liming and Michael,

I have added the requested change, here is the PR: https://github.com/tianocore/edk2/pull/4546

Thanks,
Abhi 




[-- Attachment #2: Type: text/html, Size: 4065 bytes --]