multiple levels of support for MOR / MORLock

multiple levels of support for MOR / MORLock

[Laszlo Ersek]

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

Re: multiple levels of support for MOR / MORLock

[Yao, Jiewen]

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

Re: multiple levels of support for MOR / MORLock

[Laszlo Ersek]

On 09/29/17 03:52, Yao, Jiewen wrote:
> 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.

Sounds great; thanks a lot!

I've filed:

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

If possible I'd like the fix to be committed sometime next week. Is it
OK if I try and submit a patch soon? My plan is the following:

- In MorLockInit() [TcgMorLockSmm.c], call
gSmst->SmmRegisterProtocolNotify() in order to register a callback for
gEfiSmmEndOfDxeProtocolGuid

- In the callback function, call VariableServiceGetVariable(), with size
0, to see if MOR is present -- if EFI_BUFFER_TOO_SMALL is returned, MOR
is present; otherwise MOR is absent.

- If MOR is present, then call SetMorLockVariable(0) from the callback
function; like MorLockInit() does now. Otherwise, do nothing.

- It looks like there are no circumstances under which I should
de-register the callback. (I.e. call SmmRegisterProtocolNotify() with a
NULL Function argument.)


Now, I can see that VariableSmm.c already installs such a callback --
SmmEndOfDxeCallback(). Should I hook into that callback function through
a new BOOLEAN variable, such as "mDelayedMorLockInit" (and then
MorLockInit() would only set this variable to TRUE), or should I install
a separate callback?

Either way, I don't think that I should do the MOR/MORL stuff in the
current SmmEndOfDxeCallback() function *unconditionally*, because that
callback is set up when the *read* half of the variable services is
initialized, but MORL only becomes relevant when the *write* half of the
variable services is initialized (which occurs in the
SmmFtwNotificationEvent() callback, i.e. when the FaultTolerantWrite SMM
protocol becomes available). Hence I think we need either a separate
callback registration, or a new boolean for the existent callback.

Thanks!
Laszlo

Re: multiple levels of support for MOR / MORLock

[Yao, Jiewen]

Sure. Feel free to submit patch.

For the fix, I suggest we keep all MORL related code into TcgMorLockXXX.c.
I do not suggest we mix the MORL stuff into VariableXXX.c.


A reminder that, the PRC team will have the PRC national holiday in next week. As such, we won't have too much activity in next week.

If you submit the patch, I will try to review in next week to unblock the Linux work.

In the patch, if you can add the detailed test result, that will be very helpful for us.
For example, test SMM/non-SMM version, test with MOR present/absent, etc.


Thank you
Yao Jiewen

From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Friday, September 29, 2017 7:06 PM
To: Yao, Jiewen <jiewen.yao@intel.com>
Cc: edk2-devel-01 <edk2-devel@lists.01.org>
Subject: Re: [edk2] multiple levels of support for MOR / MORLock

On 09/29/17 03:52, Yao, Jiewen wrote:
> 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.

Sounds great; thanks a lot!

I've filed:

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

If possible I'd like the fix to be committed sometime next week. Is it
OK if I try and submit a patch soon? My plan is the following:

- In MorLockInit() [TcgMorLockSmm.c], call
gSmst->SmmRegisterProtocolNotify() in order to register a callback for
gEfiSmmEndOfDxeProtocolGuid

- In the callback function, call VariableServiceGetVariable(), with size
0, to see if MOR is present -- if EFI_BUFFER_TOO_SMALL is returned, MOR
is present; otherwise MOR is absent.

- If MOR is present, then call SetMorLockVariable(0) from the callback
function; like MorLockInit() does now. Otherwise, do nothing.

- It looks like there are no circumstances under which I should
de-register the callback. (I.e. call SmmRegisterProtocolNotify() with a
NULL Function argument.)


Now, I can see that VariableSmm.c already installs such a callback --
SmmEndOfDxeCallback(). Should I hook into that callback function through
a new BOOLEAN variable, such as "mDelayedMorLockInit" (and then
MorLockInit() would only set this variable to TRUE), or should I install
a separate callback?

Either way, I don't think that I should do the MOR/MORL stuff in the
current SmmEndOfDxeCallback() function *unconditionally*, because that
callback is set up when the *read* half of the variable services is
initialized, but MORL only becomes relevant when the *write* half of the
variable services is initialized (which occurs in the
SmmFtwNotificationEvent() callback, i.e. when the FaultTolerantWrite SMM
protocol becomes available). Hence I think we need either a separate
callback registration, or a new boolean for the existent callback.

Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel

Re: multiple levels of support for MOR / MORLock

[Laszlo Ersek]

On 09/29/17 15:26, Yao, Jiewen wrote:
> Sure. Feel free to submit patch.
> 
> For the fix, I suggest we keep all MORL related code into TcgMorLockXXX.c.
> I do not suggest we mix the MORL stuff into VariableXXX.c.

I've had more thoughts about this, since sending my last email.

Currently SmmEndOfDxeCallback()
[MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c] performs the
following actions:

- sets "mEndOfDxe" to TRUE
- calls VarCheckLibInitializeAtEndOfDxe()
- calls InitializeVariableQuota()
- calls ReclaimForOS() optionally

If we register a separate callback for gEfiSmmEndOfDxeProtocolGuid, in
order to create the MORL variable, then that callback could be invoked
after SmmEndOfDxeCallback(). I think it would not be optimal to create
MORL after SmmEndOfDxeCallback() has run, even if it worked, technically
speaking. This suggests to hook the MORL creation to the top of
SmmEndOfDxeCallback(). What do you suggest?

> A reminder that, the PRC team will have the PRC national holiday in next week. As such, we won't have too much activity in next week.
> 
> If you submit the patch, I will try to review in next week to unblock the Linux work.

Ah, sorry, I haven't been aware of the holidays.

I might send a patch soon, but please don't let me disturb you during
the holidays! Especially because this patch might need more discussion
and more versins. Holidays are for getting rested from work! :) We can
sort it out after you return.

> In the patch, if you can add the detailed test result, that will be very helpful for us.
> For example, test SMM/non-SMM version, test with MOR present/absent, etc.

For the "no MOR, no MORL" case, I'll have to rely on Ladi, for test
results (Ladi offered to help test that case, thanks again for that, Ladi).

However, for regression-testing the "both MOR and MORL" case, I can only
rely on Intel developers and quality engineers -- OVMF doesn't have any
TPM / MOR solution at the moment. (If we had one, then we wouldn't have
found or reported this issue in the first place. :) )

The fact that I can't regression-test the MOR+MORL case myself supports
the idea that we should postpone the review and commit (if appropriate)
of the patch until after the PRC national holiday.

Thanks!
Laszlo


> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
> Sent: Friday, September 29, 2017 7:06 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: edk2-devel-01 <edk2-devel@lists.01.org>
> Subject: Re: [edk2] multiple levels of support for MOR / MORLock
> 
> On 09/29/17 03:52, Yao, Jiewen wrote:
>> 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.
> 
> Sounds great; thanks a lot!
> 
> I've filed:
> 
>   https://bugzilla.tianocore.org/show_bug.cgi?id=727
> 
> If possible I'd like the fix to be committed sometime next week. Is it
> OK if I try and submit a patch soon? My plan is the following:
> 
> - In MorLockInit() [TcgMorLockSmm.c], call
> gSmst->SmmRegisterProtocolNotify() in order to register a callback for
> gEfiSmmEndOfDxeProtocolGuid
> 
> - In the callback function, call VariableServiceGetVariable(), with size
> 0, to see if MOR is present -- if EFI_BUFFER_TOO_SMALL is returned, MOR
> is present; otherwise MOR is absent.
> 
> - If MOR is present, then call SetMorLockVariable(0) from the callback
> function; like MorLockInit() does now. Otherwise, do nothing.
> 
> - It looks like there are no circumstances under which I should
> de-register the callback. (I.e. call SmmRegisterProtocolNotify() with a
> NULL Function argument.)
> 
> 
> Now, I can see that VariableSmm.c already installs such a callback --
> SmmEndOfDxeCallback(). Should I hook into that callback function through
> a new BOOLEAN variable, such as "mDelayedMorLockInit" (and then
> MorLockInit() would only set this variable to TRUE), or should I install
> a separate callback?
> 
> Either way, I don't think that I should do the MOR/MORL stuff in the
> current SmmEndOfDxeCallback() function *unconditionally*, because that
> callback is set up when the *read* half of the variable services is
> initialized, but MORL only becomes relevant when the *write* half of the
> variable services is initialized (which occurs in the
> SmmFtwNotificationEvent() callback, i.e. when the FaultTolerantWrite SMM
> protocol becomes available). Hence I think we need either a separate
> callback registration, or a new boolean for the existent callback.
> 
> Thanks!
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

Re: multiple levels of support for MOR / MORLock

[Yao, Jiewen]

Agree. Adding a hook function is OK for me, as long as the hook function implementation is in TcgMorLockXXX.c

Thank you
Yao Jiewen


From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Saturday, September 30, 2017 12:39 AM
To: Yao, Jiewen <jiewen.yao@intel.com>
Cc: edk2-devel-01 <edk2-devel@lists.01.org>; Ladi Prosek <lprosek@redhat.com>
Subject: Re: [edk2] multiple levels of support for MOR / MORLock

On 09/29/17 15:26, Yao, Jiewen wrote:
> Sure. Feel free to submit patch.
>
> For the fix, I suggest we keep all MORL related code into TcgMorLockXXX.c.
> I do not suggest we mix the MORL stuff into VariableXXX.c.

I've had more thoughts about this, since sending my last email.

Currently SmmEndOfDxeCallback()
[MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c] performs the
following actions:

- sets "mEndOfDxe" to TRUE
- calls VarCheckLibInitializeAtEndOfDxe()
- calls InitializeVariableQuota()
- calls ReclaimForOS() optionally

If we register a separate callback for gEfiSmmEndOfDxeProtocolGuid, in
order to create the MORL variable, then that callback could be invoked
after SmmEndOfDxeCallback(). I think it would not be optimal to create
MORL after SmmEndOfDxeCallback() has run, even if it worked, technically
speaking. This suggests to hook the MORL creation to the top of
SmmEndOfDxeCallback(). What do you suggest?

> A reminder that, the PRC team will have the PRC national holiday in next week. As such, we won't have too much activity in next week.
>
> If you submit the patch, I will try to review in next week to unblock the Linux work.

Ah, sorry, I haven't been aware of the holidays.

I might send a patch soon, but please don't let me disturb you during
the holidays! Especially because this patch might need more discussion
and more versins. Holidays are for getting rested from work! :) We can
sort it out after you return.

> In the patch, if you can add the detailed test result, that will be very helpful for us.
> For example, test SMM/non-SMM version, test with MOR present/absent, etc.

For the "no MOR, no MORL" case, I'll have to rely on Ladi, for test
results (Ladi offered to help test that case, thanks again for that, Ladi).

However, for regression-testing the "both MOR and MORL" case, I can only
rely on Intel developers and quality engineers -- OVMF doesn't have any
TPM / MOR solution at the moment. (If we had one, then we wouldn't have
found or reported this issue in the first place. :) )

The fact that I can't regression-test the MOR+MORL case myself supports
the idea that we should postpone the review and commit (if appropriate)
of the patch until after the PRC national holiday.

Thanks!
Laszlo


> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
> Sent: Friday, September 29, 2017 7:06 PM
> To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
> Cc: edk2-devel-01 <edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>>
> Subject: Re: [edk2] multiple levels of support for MOR / MORLock
>
> On 09/29/17 03:52, Yao, Jiewen wrote:
>> 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.
>
> Sounds great; thanks a lot!
>
> I've filed:
>
>   https://bugzilla.tianocore.org/show_bug.cgi?id=727
>
> If possible I'd like the fix to be committed sometime next week. Is it
> OK if I try and submit a patch soon? My plan is the following:
>
> - In MorLockInit() [TcgMorLockSmm.c], call
> gSmst->SmmRegisterProtocolNotify() in order to register a callback for
> gEfiSmmEndOfDxeProtocolGuid
>
> - In the callback function, call VariableServiceGetVariable(), with size
> 0, to see if MOR is present -- if EFI_BUFFER_TOO_SMALL is returned, MOR
> is present; otherwise MOR is absent.
>
> - If MOR is present, then call SetMorLockVariable(0) from the callback
> function; like MorLockInit() does now. Otherwise, do nothing.
>
> - It looks like there are no circumstances under which I should
> de-register the callback. (I.e. call SmmRegisterProtocolNotify() with a
> NULL Function argument.)
>
>
> Now, I can see that VariableSmm.c already installs such a callback --
> SmmEndOfDxeCallback(). Should I hook into that callback function through
> a new BOOLEAN variable, such as "mDelayedMorLockInit" (and then
> MorLockInit() would only set this variable to TRUE), or should I install
> a separate callback?
>
> Either way, I don't think that I should do the MOR/MORL stuff in the
> current SmmEndOfDxeCallback() function *unconditionally*, because that
> callback is set up when the *read* half of the variable services is
> initialized, but MORL only becomes relevant when the *write* half of the
> variable services is initialized (which occurs in the
> SmmFtwNotificationEvent() callback, i.e. when the FaultTolerantWrite SMM
> protocol becomes available). Hence I think we need either a separate
> callback registration, or a new boolean for the existent callback.
>
> Thanks!
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
> https://lists.01.org/mailman/listinfo/edk2-devel
>

Re: multiple levels of support for MOR / MORLock

[Laszlo Ersek]

Hello Jiewen,

On 09/30/17 05:53, Yao, Jiewen wrote:
> Agree. Adding a hook function is OK for me, as long as the hook
> function implementation is in TcgMorLockXXX.c

I've implemented this, but I've run into a problem while testing it.

Your idea requires that *nothing* create the MOR variable other than

  SecurityPkg/Tcg/MemoryOverwriteControl/TcgMor.inf

Unfortunately, released Fedora operating systems break this assumption.
(It is a Linux kernel bug, no question about that, but it still breaks
the idea.)

Namely, in the current upstream Linux kernel (the *upcoming* v4.14
release), there is a function called
efi_enable_reset_attack_mitigation(), added in commit ccc829ba3624b:

  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ccc829ba3624b

> /*
>  * Enable reboot attack mitigation. This requests that the firmware clear the
>  * RAM on next reboot before proceeding with boot, ensuring that any secrets
>  * are cleared. If userland has ensured that all secrets have been removed
>  * from RAM before reboot it can simply reset this variable.
>  */
> void efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg)
> {
>         u8 val = 1;
>         efi_guid_t var_guid = MEMORY_ONLY_RESET_CONTROL_GUID;
>         efi_status_t status;
>         unsigned long datasize = 0;
>
>         status = get_efi_var(efi_MemoryOverWriteRequest_name, &var_guid,
>                              NULL, &datasize, NULL);
>
>         if (status == EFI_NOT_FOUND)
>                 return;
>
>         set_efi_var(efi_MemoryOverWriteRequest_name, &var_guid,
>                     EFI_VARIABLE_NON_VOLATILE |
>                     EFI_VARIABLE_BOOTSERVICE_ACCESS |
>                     EFI_VARIABLE_RUNTIME_ACCESS, sizeof(val), &val);
> }

This function is correct in that it first checks whether the MOR
variable exists. If it doesn't exist, the function doesn't *create* the
variable.

However, several Fedora distribution kernels *already* contain an
*earlier* version of the same function (which had never been merged into
upstream Linux), and that variant of the function is incorrect:

  https://git.kernel.org/pub/scm/linux/kernel/git/jwboyer/fedora.git/commit/?id=65673e37e61d8

> static void enable_reset_attack_mitigation(void)
> {
>         static const efi_guid_t var_guid = MEMORY_ONLY_RESET_CONTROL_GUID;
>         static const efi_char16_t MemoryOverwriteRequestControl_name[] = {
>                 'M', 'e', 'm', 'o', 'r', 'y',
>                 'O', 'v', 'e', 'r', 'w', 'r', 'i', 't', 'e',
>                 'R', 'e', 'q', 'u', 'e', 's', 't',
>                 'C', 'o', 'n', 't', 'r', 'o', 'l',
>                 0
>         };
>         u8 val = 1;
>
>         /* Ignore the return value here - there's not really a lot we can do */
>         efi_call_runtime(set_variable,
>                         (efi_char16_t *)MemoryOverwriteRequestControl_name,
>                         (efi_guid_t *)&var_guid,
>                         EFI_VARIABLE_NON_VOLATILE |
>                         EFI_VARIABLE_BOOTSERVICE_ACCESS |
>                         EFI_VARIABLE_RUNTIME_ACCESS,
>                         sizeof(val), val);
> }

This variant does not check if the MOR variable exists.

If the variable doesn't exist, then this function will create it
(incorrectly). At the next boot, the SmmEndOfDxe callback in
"TcgMorLockSmm.c" will think that the firmware platform includes support
for the MOR variable (it assumes that TcgMor.inf is part of the firmware
and that TcgMor.inf created the variable), and then the callback
creates/sets MorLock as well.

Again, this is clearly a kernel bug, but it is part of at least the
following releases:

- Fedora 26:
  https://git.kernel.org/pub/scm/linux/kernel/git/jwboyer/fedora.git/commit/?id=65673e37e61d

- Fedora 25:
  https://git.kernel.org/pub/scm/linux/kernel/git/jwboyer/fedora.git/commit/?id=1f4e5e657685

- Fedora 24:
  https://git.kernel.org/pub/scm/linux/kernel/git/jwboyer/fedora.git/commit/?id=677765d4db8e

Thus the SmmEndOfDxe callback wouldn't work correctly together with
these Fedora releases.

Thanks,
Laszlo

> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Saturday, September 30, 2017 12:39 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: edk2-devel-01 <edk2-devel@lists.01.org>; Ladi Prosek <lprosek@redhat.com>
> Subject: Re: [edk2] multiple levels of support for MOR / MORLock
>
> On 09/29/17 15:26, Yao, Jiewen wrote:
>> Sure. Feel free to submit patch.
>>
>> For the fix, I suggest we keep all MORL related code into
>> TcgMorLockXXX.c. I do not suggest we mix the MORL stuff into
>> VariableXXX.c.
>
> I've had more thoughts about this, since sending my last email.
>
> Currently SmmEndOfDxeCallback()
> [MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c] performs
> the following actions:
>
> - sets "mEndOfDxe" to TRUE
> - calls VarCheckLibInitializeAtEndOfDxe()
> - calls InitializeVariableQuota()
> - calls ReclaimForOS() optionally
>
> If we register a separate callback for gEfiSmmEndOfDxeProtocolGuid, in
> order to create the MORL variable, then that callback could be invoked
> after SmmEndOfDxeCallback(). I think it would not be optimal to create
> MORL after SmmEndOfDxeCallback() has run, even if it worked,
> technically speaking. This suggests to hook the MORL creation to the
> top of SmmEndOfDxeCallback(). What do you suggest?
>
>> A reminder that, the PRC team will have the PRC national holiday in
>> next week. As such, we won't have too much activity in next week.
>>
>> If you submit the patch, I will try to review in next week to unblock
>> the Linux work.
>
> Ah, sorry, I haven't been aware of the holidays.
>
> I might send a patch soon, but please don't let me disturb you during
> the holidays! Especially because this patch might need more discussion
> and more versins. Holidays are for getting rested from work! :) We can
> sort it out after you return.
>
>> In the patch, if you can add the detailed test result, that will be
>> very helpful for us. For example, test SMM/non-SMM version, test with
>> MOR present/absent, etc.
>
> For the "no MOR, no MORL" case, I'll have to rely on Ladi, for test
> results (Ladi offered to help test that case, thanks again for that,
> Ladi).
>
> However, for regression-testing the "both MOR and MORL" case, I can
> only rely on Intel developers and quality engineers -- OVMF doesn't
> have any TPM / MOR solution at the moment. (If we had one, then we
> wouldn't have found or reported this issue in the first place. :) )
>
> The fact that I can't regression-test the MOR+MORL case myself
> supports the idea that we should postpone the review and commit (if
> appropriate) of the patch until after the PRC national holiday.
>
> Thanks!
> Laszlo
>
>
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
>> Sent: Friday, September 29, 2017 7:06 PM
>> To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
>> Cc: edk2-devel-01 <edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>>
>> Subject: Re: [edk2] multiple levels of support for MOR / MORLock
>>
>> On 09/29/17 03:52, Yao, Jiewen wrote:
>>> 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.
>>
>> Sounds great; thanks a lot!
>>
>> I've filed:
>>
>>   https://bugzilla.tianocore.org/show_bug.cgi?id=727
>>
>> If possible I'd like the fix to be committed sometime next week. Is
>> it OK if I try and submit a patch soon? My plan is the following:
>>
>> - In MorLockInit() [TcgMorLockSmm.c], call
>> gSmst->SmmRegisterProtocolNotify() in order to register a callback
>> for gEfiSmmEndOfDxeProtocolGuid
>>
>> - In the callback function, call VariableServiceGetVariable(), with
>> size 0, to see if MOR is present -- if EFI_BUFFER_TOO_SMALL is
>> returned, MOR is present; otherwise MOR is absent.
>>
>> - If MOR is present, then call SetMorLockVariable(0) from the
>> callback function; like MorLockInit() does now. Otherwise, do
>> nothing.
>>
>> - It looks like there are no circumstances under which I should
>> de-register the callback. (I.e. call SmmRegisterProtocolNotify() with
>> a NULL Function argument.)
>>
>>
>> Now, I can see that VariableSmm.c already installs such a callback --
>> SmmEndOfDxeCallback(). Should I hook into that callback function
>> through a new BOOLEAN variable, such as "mDelayedMorLockInit" (and
>> then MorLockInit() would only set this variable to TRUE), or should I
>> install a separate callback?
>>
>> Either way, I don't think that I should do the MOR/MORL stuff in the
>> current SmmEndOfDxeCallback() function *unconditionally*, because
>> that callback is set up when the *read* half of the variable services
>> is initialized, but MORL only becomes relevant when the *write* half
>> of the variable services is initialized (which occurs in the
>> SmmFtwNotificationEvent() callback, i.e. when the FaultTolerantWrite
>> SMM protocol becomes available). Hence I think we need either a
>> separate callback registration, or a new boolean for the existent
>> callback.
>>
>> Thanks!
>> Laszlo

Re: multiple levels of support for MOR / MORLock

[Yao, Jiewen]

Good test! Thanks for the investigation.

That is a little annoying.
I still do not want to use PCD. My thought is below:

Current MOR has TCG protocol dependence.   So can we add one more check - to check the existence of TCG/TCG2 protocol in variable driver? Both TCG and TCG2 are in mdepkg, there is no dependency issue.

Even if MOR is present, but neither TCG nor TCG2 is present, don't create MORL but delete MOR. (Just in case people boot fedora, then boot windows) I believe that can handle the fedora issue.

thank you!
Yao, Jiewen


> 在 2017年10月1日，上午4:33，Laszlo Ersek <lersek@redhat.com> 写道：
> 
> Hello Jiewen,
> 
>> On 09/30/17 05:53, Yao, Jiewen wrote:
>> Agree. Adding a hook function is OK for me, as long as the hook
>> function implementation is in TcgMorLockXXX.c
> 
> I've implemented this, but I've run into a problem while testing it.
> 
> Your idea requires that *nothing* create the MOR variable other than
> 
>  SecurityPkg/Tcg/MemoryOverwriteControl/TcgMor.inf
> 
> Unfortunately, released Fedora operating systems break this assumption.
> (It is a Linux kernel bug, no question about that, but it still breaks
> the idea.)
> 
> Namely, in the current upstream Linux kernel (the *upcoming* v4.14
> release), there is a function called
> efi_enable_reset_attack_mitigation(), added in commit ccc829ba3624b:
> 
>  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ccc829ba3624b
> 
>> /*
>> * Enable reboot attack mitigation. This requests that the firmware clear the
>> * RAM on next reboot before proceeding with boot, ensuring that any secrets
>> * are cleared. If userland has ensured that all secrets have been removed
>> * from RAM before reboot it can simply reset this variable.
>> */
>> void efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg)
>> {
>>        u8 val = 1;
>>        efi_guid_t var_guid = MEMORY_ONLY_RESET_CONTROL_GUID;
>>        efi_status_t status;
>>        unsigned long datasize = 0;
>> 
>>        status = get_efi_var(efi_MemoryOverWriteRequest_name, &var_guid,
>>                             NULL, &datasize, NULL);
>> 
>>        if (status == EFI_NOT_FOUND)
>>                return;
>> 
>>        set_efi_var(efi_MemoryOverWriteRequest_name, &var_guid,
>>                    EFI_VARIABLE_NON_VOLATILE |
>>                    EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>                    EFI_VARIABLE_RUNTIME_ACCESS, sizeof(val), &val);
>> }
> 
> This function is correct in that it first checks whether the MOR
> variable exists. If it doesn't exist, the function doesn't *create* the
> variable.
> 
> However, several Fedora distribution kernels *already* contain an
> *earlier* version of the same function (which had never been merged into
> upstream Linux), and that variant of the function is incorrect:
> 
>  https://git.kernel.org/pub/scm/linux/kernel/git/jwboyer/fedora.git/commit/?id=65673e37e61d8
> 
>> static void enable_reset_attack_mitigation(void)
>> {
>>        static const efi_guid_t var_guid = MEMORY_ONLY_RESET_CONTROL_GUID;
>>        static const efi_char16_t MemoryOverwriteRequestControl_name[] = {
>>                'M', 'e', 'm', 'o', 'r', 'y',
>>                'O', 'v', 'e', 'r', 'w', 'r', 'i', 't', 'e',
>>                'R', 'e', 'q', 'u', 'e', 's', 't',
>>                'C', 'o', 'n', 't', 'r', 'o', 'l',
>>                0
>>        };
>>        u8 val = 1;
>> 
>>        /* Ignore the return value here - there's not really a lot we can do */
>>        efi_call_runtime(set_variable,
>>                        (efi_char16_t *)MemoryOverwriteRequestControl_name,
>>                        (efi_guid_t *)&var_guid,
>>                        EFI_VARIABLE_NON_VOLATILE |
>>                        EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>                        EFI_VARIABLE_RUNTIME_ACCESS,
>>                        sizeof(val), val);
>> }
> 
> This variant does not check if the MOR variable exists.
> 
> If the variable doesn't exist, then this function will create it
> (incorrectly). At the next boot, the SmmEndOfDxe callback in
> "TcgMorLockSmm.c" will think that the firmware platform includes support
> for the MOR variable (it assumes that TcgMor.inf is part of the firmware
> and that TcgMor.inf created the variable), and then the callback
> creates/sets MorLock as well.
> 
> Again, this is clearly a kernel bug, but it is part of at least the
> following releases:
> 
> - Fedora 26:
>  https://git.kernel.org/pub/scm/linux/kernel/git/jwboyer/fedora.git/commit/?id=65673e37e61d
> 
> - Fedora 25:
>  https://git.kernel.org/pub/scm/linux/kernel/git/jwboyer/fedora.git/commit/?id=1f4e5e657685
> 
> - Fedora 24:
>  https://git.kernel.org/pub/scm/linux/kernel/git/jwboyer/fedora.git/commit/?id=677765d4db8e
> 
> Thus the SmmEndOfDxe callback wouldn't work correctly together with
> these Fedora releases.
> 
> Thanks,
> Laszlo
> 
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Saturday, September 30, 2017 12:39 AM
>> To: Yao, Jiewen <jiewen.yao@intel.com>
>> Cc: edk2-devel-01 <edk2-devel@lists.01.org>; Ladi Prosek <lprosek@redhat.com>
>> Subject: Re: [edk2] multiple levels of support for MOR / MORLock
>> 
>>> On 09/29/17 15:26, Yao, Jiewen wrote:
>>> Sure. Feel free to submit patch.
>>> 
>>> For the fix, I suggest we keep all MORL related code into
>>> TcgMorLockXXX.c. I do not suggest we mix the MORL stuff into
>>> VariableXXX.c.
>> 
>> I've had more thoughts about this, since sending my last email.
>> 
>> Currently SmmEndOfDxeCallback()
>> [MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c] performs
>> the following actions:
>> 
>> - sets "mEndOfDxe" to TRUE
>> - calls VarCheckLibInitializeAtEndOfDxe()
>> - calls InitializeVariableQuota()
>> - calls ReclaimForOS() optionally
>> 
>> If we register a separate callback for gEfiSmmEndOfDxeProtocolGuid, in
>> order to create the MORL variable, then that callback could be invoked
>> after SmmEndOfDxeCallback(). I think it would not be optimal to create
>> MORL after SmmEndOfDxeCallback() has run, even if it worked,
>> technically speaking. This suggests to hook the MORL creation to the
>> top of SmmEndOfDxeCallback(). What do you suggest?
>> 
>>> A reminder that, the PRC team will have the PRC national holiday in
>>> next week. As such, we won't have too much activity in next week.
>>> 
>>> If you submit the patch, I will try to review in next week to unblock
>>> the Linux work.
>> 
>> Ah, sorry, I haven't been aware of the holidays.
>> 
>> I might send a patch soon, but please don't let me disturb you during
>> the holidays! Especially because this patch might need more discussion
>> and more versins. Holidays are for getting rested from work! :) We can
>> sort it out after you return.
>> 
>>> In the patch, if you can add the detailed test result, that will be
>>> very helpful for us. For example, test SMM/non-SMM version, test with
>>> MOR present/absent, etc.
>> 
>> For the "no MOR, no MORL" case, I'll have to rely on Ladi, for test
>> results (Ladi offered to help test that case, thanks again for that,
>> Ladi).
>> 
>> However, for regression-testing the "both MOR and MORL" case, I can
>> only rely on Intel developers and quality engineers -- OVMF doesn't
>> have any TPM / MOR solution at the moment. (If we had one, then we
>> wouldn't have found or reported this issue in the first place. :) )
>> 
>> The fact that I can't regression-test the MOR+MORL case myself
>> supports the idea that we should postpone the review and commit (if
>> appropriate) of the patch until after the PRC national holiday.
>> 
>> Thanks!
>> Laszlo
>> 
>> 
>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
>>> Sent: Friday, September 29, 2017 7:06 PM
>>> To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
>>> Cc: edk2-devel-01 <edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>>
>>> Subject: Re: [edk2] multiple levels of support for MOR / MORLock
>>> 
>>>> On 09/29/17 03:52, Yao, Jiewen wrote:
>>>> 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.
>>> 
>>> Sounds great; thanks a lot!
>>> 
>>> I've filed:
>>> 
>>>  https://bugzilla.tianocore.org/show_bug.cgi?id=727
>>> 
>>> If possible I'd like the fix to be committed sometime next week. Is
>>> it OK if I try and submit a patch soon? My plan is the following:
>>> 
>>> - In MorLockInit() [TcgMorLockSmm.c], call
>>> gSmst->SmmRegisterProtocolNotify() in order to register a callback
>>> for gEfiSmmEndOfDxeProtocolGuid
>>> 
>>> - In the callback function, call VariableServiceGetVariable(), with
>>> size 0, to see if MOR is present -- if EFI_BUFFER_TOO_SMALL is
>>> returned, MOR is present; otherwise MOR is absent.
>>> 
>>> - If MOR is present, then call SetMorLockVariable(0) from the
>>> callback function; like MorLockInit() does now. Otherwise, do
>>> nothing.
>>> 
>>> - It looks like there are no circumstances under which I should
>>> de-register the callback. (I.e. call SmmRegisterProtocolNotify() with
>>> a NULL Function argument.)
>>> 
>>> 
>>> Now, I can see that VariableSmm.c already installs such a callback --
>>> SmmEndOfDxeCallback(). Should I hook into that callback function
>>> through a new BOOLEAN variable, such as "mDelayedMorLockInit" (and
>>> then MorLockInit() would only set this variable to TRUE), or should I
>>> install a separate callback?
>>> 
>>> Either way, I don't think that I should do the MOR/MORL stuff in the
>>> current SmmEndOfDxeCallback() function *unconditionally*, because
>>> that callback is set up when the *read* half of the variable services
>>> is initialized, but MORL only becomes relevant when the *write* half
>>> of the variable services is initialized (which occurs in the
>>> SmmFtwNotificationEvent() callback, i.e. when the FaultTolerantWrite
>>> SMM protocol becomes available). Hence I think we need either a
>>> separate callback registration, or a new boolean for the existent
>>> callback.
>>> 
>>> Thanks!
>>> Laszlo

Re: multiple levels of support for MOR / MORLock

[Laszlo Ersek]

On 10/01/17 05:56, Yao, Jiewen wrote:
> Good test! Thanks for the investigation.
> 
> That is a little annoying.

Agreed!

In a virtualization-only scenario, I wouldn't care much (it's very
unusual to install two OSes in the same VM and "dual boot" them), but on
physical machines, dual-booting Windows and Linux is very frequent, and
whatever we put in the Variable driver now will end up on physical
machines sooner or later.

> I still do not want to use PCD. My thought is below:
> 
> Current MOR has TCG protocol dependence.   So can we add one more
> check - to check the existence of TCG/TCG2 protocol in variable
> driver? Both TCG and TCG2 are in mdepkg, there is no dependency
> issue.
> 
> Even if MOR is present, but neither TCG nor TCG2 is present, don't
> create MORL but delete MOR. (Just in case people boot fedora, then
> boot windows) I believe that can handle the fedora issue.
According to the PI spec, "1.8 MM Traditional Driver Initialization", in
the VariableSmm driver I can only locate UEFI protocols in the entry
point function:

    [...] An MM Driver’s initialization phase ends when the entry point
    returns. [...] UEFI protocols can be located and used by MM drivers
    only during MM Initialization. [...]

The SmmEndOfDxe callback does not run as part of the VariableSmm
driver's entry point function (VariableServiceInitialize()), so I
believe I might not be allowed to call gBS->LocateProtocol() in the
callback. Is that correct?


Another option would be to register a UEFI protocol notify for the
TCG/TCG2 protocols, and to set a global variable (in SMRAM) when this
notify fires. Then the SmmEndOfDxe callback could check the variable.

However, what happens if the TCG/TCG2 protocols are installed *after*
PiSmmCpuDxeSmm is dispatched, and the SMM IPL closes SMRAM? Then the
protocol notify could never set the variable in SMRAM -- in fact, the
protocol notify couldn't even be invoked, because the notify function
itself is located in SMRAM (as part of the VariableSmm executable).


So, for this to work, I believe we'd have to modify the SMM_CORE to
"forward" the TCG/TCG2 UEFI protocols into the SMM protocol database
(similarly to EndOfDxe/SmmEndOfDxe, although in that case, EndOfDxe is a
UEFI event group, not a UEFI protocol).

Is my understanding correct? What do you suggest?


Thank you for taking the time to discuss this with me! If it's becoming
too long, we can continue after the weekend & the holidays end.

Cheers!
Laszlo

>> 在 2017年10月1日，上午4:33，Laszlo Ersek <lersek@redhat.com> 写道：
>>
>> Hello Jiewen,
>>
>>> On 09/30/17 05:53, Yao, Jiewen wrote:
>>> Agree. Adding a hook function is OK for me, as long as the hook
>>> function implementation is in TcgMorLockXXX.c
>>
>> I've implemented this, but I've run into a problem while testing it.
>>
>> Your idea requires that *nothing* create the MOR variable other than
>>
>>  SecurityPkg/Tcg/MemoryOverwriteControl/TcgMor.inf
>>
>> Unfortunately, released Fedora operating systems break this assumption.
>> (It is a Linux kernel bug, no question about that, but it still breaks
>> the idea.)
>>
>> Namely, in the current upstream Linux kernel (the *upcoming* v4.14
>> release), there is a function called
>> efi_enable_reset_attack_mitigation(), added in commit ccc829ba3624b:
>>
>>  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ccc829ba3624b
>>
>>> /*
>>> * Enable reboot attack mitigation. This requests that the firmware clear the
>>> * RAM on next reboot before proceeding with boot, ensuring that any secrets
>>> * are cleared. If userland has ensured that all secrets have been removed
>>> * from RAM before reboot it can simply reset this variable.
>>> */
>>> void efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg)
>>> {
>>>        u8 val = 1;
>>>        efi_guid_t var_guid = MEMORY_ONLY_RESET_CONTROL_GUID;
>>>        efi_status_t status;
>>>        unsigned long datasize = 0;
>>>
>>>        status = get_efi_var(efi_MemoryOverWriteRequest_name, &var_guid,
>>>                             NULL, &datasize, NULL);
>>>
>>>        if (status == EFI_NOT_FOUND)
>>>                return;
>>>
>>>        set_efi_var(efi_MemoryOverWriteRequest_name, &var_guid,
>>>                    EFI_VARIABLE_NON_VOLATILE |
>>>                    EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>>                    EFI_VARIABLE_RUNTIME_ACCESS, sizeof(val), &val);
>>> }
>>
>> This function is correct in that it first checks whether the MOR
>> variable exists. If it doesn't exist, the function doesn't *create* the
>> variable.
>>
>> However, several Fedora distribution kernels *already* contain an
>> *earlier* version of the same function (which had never been merged into
>> upstream Linux), and that variant of the function is incorrect:
>>
>>  https://git.kernel.org/pub/scm/linux/kernel/git/jwboyer/fedora.git/commit/?id=65673e37e61d8
>>
>>> static void enable_reset_attack_mitigation(void)
>>> {
>>>        static const efi_guid_t var_guid = MEMORY_ONLY_RESET_CONTROL_GUID;
>>>        static const efi_char16_t MemoryOverwriteRequestControl_name[] = {
>>>                'M', 'e', 'm', 'o', 'r', 'y',
>>>                'O', 'v', 'e', 'r', 'w', 'r', 'i', 't', 'e',
>>>                'R', 'e', 'q', 'u', 'e', 's', 't',
>>>                'C', 'o', 'n', 't', 'r', 'o', 'l',
>>>                0
>>>        };
>>>        u8 val = 1;
>>>
>>>        /* Ignore the return value here - there's not really a lot we can do */
>>>        efi_call_runtime(set_variable,
>>>                        (efi_char16_t *)MemoryOverwriteRequestControl_name,
>>>                        (efi_guid_t *)&var_guid,
>>>                        EFI_VARIABLE_NON_VOLATILE |
>>>                        EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>>                        EFI_VARIABLE_RUNTIME_ACCESS,
>>>                        sizeof(val), val);
>>> }
>>
>> This variant does not check if the MOR variable exists.
>>
>> If the variable doesn't exist, then this function will create it
>> (incorrectly). At the next boot, the SmmEndOfDxe callback in
>> "TcgMorLockSmm.c" will think that the firmware platform includes support
>> for the MOR variable (it assumes that TcgMor.inf is part of the firmware
>> and that TcgMor.inf created the variable), and then the callback
>> creates/sets MorLock as well.
>>
>> Again, this is clearly a kernel bug, but it is part of at least the
>> following releases:
>>
>> - Fedora 26:
>>  https://git.kernel.org/pub/scm/linux/kernel/git/jwboyer/fedora.git/commit/?id=65673e37e61d
>>
>> - Fedora 25:
>>  https://git.kernel.org/pub/scm/linux/kernel/git/jwboyer/fedora.git/commit/?id=1f4e5e657685
>>
>> - Fedora 24:
>>  https://git.kernel.org/pub/scm/linux/kernel/git/jwboyer/fedora.git/commit/?id=677765d4db8e
>>
>> Thus the SmmEndOfDxe callback wouldn't work correctly together with
>> these Fedora releases.
>>
>> Thanks,
>> Laszlo
>>
>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>> Sent: Saturday, September 30, 2017 12:39 AM
>>> To: Yao, Jiewen <jiewen.yao@intel.com>
>>> Cc: edk2-devel-01 <edk2-devel@lists.01.org>; Ladi Prosek <lprosek@redhat.com>
>>> Subject: Re: [edk2] multiple levels of support for MOR / MORLock
>>>
>>>> On 09/29/17 15:26, Yao, Jiewen wrote:
>>>> Sure. Feel free to submit patch.
>>>>
>>>> For the fix, I suggest we keep all MORL related code into
>>>> TcgMorLockXXX.c. I do not suggest we mix the MORL stuff into
>>>> VariableXXX.c.
>>>
>>> I've had more thoughts about this, since sending my last email.
>>>
>>> Currently SmmEndOfDxeCallback()
>>> [MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c] performs
>>> the following actions:
>>>
>>> - sets "mEndOfDxe" to TRUE
>>> - calls VarCheckLibInitializeAtEndOfDxe()
>>> - calls InitializeVariableQuota()
>>> - calls ReclaimForOS() optionally
>>>
>>> If we register a separate callback for gEfiSmmEndOfDxeProtocolGuid, in
>>> order to create the MORL variable, then that callback could be invoked
>>> after SmmEndOfDxeCallback(). I think it would not be optimal to create
>>> MORL after SmmEndOfDxeCallback() has run, even if it worked,
>>> technically speaking. This suggests to hook the MORL creation to the
>>> top of SmmEndOfDxeCallback(). What do you suggest?
>>>
>>>> A reminder that, the PRC team will have the PRC national holiday in
>>>> next week. As such, we won't have too much activity in next week.
>>>>
>>>> If you submit the patch, I will try to review in next week to unblock
>>>> the Linux work.
>>>
>>> Ah, sorry, I haven't been aware of the holidays.
>>>
>>> I might send a patch soon, but please don't let me disturb you during
>>> the holidays! Especially because this patch might need more discussion
>>> and more versins. Holidays are for getting rested from work! :) We can
>>> sort it out after you return.
>>>
>>>> In the patch, if you can add the detailed test result, that will be
>>>> very helpful for us. For example, test SMM/non-SMM version, test with
>>>> MOR present/absent, etc.
>>>
>>> For the "no MOR, no MORL" case, I'll have to rely on Ladi, for test
>>> results (Ladi offered to help test that case, thanks again for that,
>>> Ladi).
>>>
>>> However, for regression-testing the "both MOR and MORL" case, I can
>>> only rely on Intel developers and quality engineers -- OVMF doesn't
>>> have any TPM / MOR solution at the moment. (If we had one, then we
>>> wouldn't have found or reported this issue in the first place. :) )
>>>
>>> The fact that I can't regression-test the MOR+MORL case myself
>>> supports the idea that we should postpone the review and commit (if
>>> appropriate) of the patch until after the PRC national holiday.
>>>
>>> Thanks!
>>> Laszlo
>>>
>>>
>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
>>>> Sent: Friday, September 29, 2017 7:06 PM
>>>> To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
>>>> Cc: edk2-devel-01 <edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>>
>>>> Subject: Re: [edk2] multiple levels of support for MOR / MORLock
>>>>
>>>>> On 09/29/17 03:52, Yao, Jiewen wrote:
>>>>> 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.
>>>>
>>>> Sounds great; thanks a lot!
>>>>
>>>> I've filed:
>>>>
>>>>  https://bugzilla.tianocore.org/show_bug.cgi?id=727
>>>>
>>>> If possible I'd like the fix to be committed sometime next week. Is
>>>> it OK if I try and submit a patch soon? My plan is the following:
>>>>
>>>> - In MorLockInit() [TcgMorLockSmm.c], call
>>>> gSmst->SmmRegisterProtocolNotify() in order to register a callback
>>>> for gEfiSmmEndOfDxeProtocolGuid
>>>>
>>>> - In the callback function, call VariableServiceGetVariable(), with
>>>> size 0, to see if MOR is present -- if EFI_BUFFER_TOO_SMALL is
>>>> returned, MOR is present; otherwise MOR is absent.
>>>>
>>>> - If MOR is present, then call SetMorLockVariable(0) from the
>>>> callback function; like MorLockInit() does now. Otherwise, do
>>>> nothing.
>>>>
>>>> - It looks like there are no circumstances under which I should
>>>> de-register the callback. (I.e. call SmmRegisterProtocolNotify() with
>>>> a NULL Function argument.)
>>>>
>>>>
>>>> Now, I can see that VariableSmm.c already installs such a callback --
>>>> SmmEndOfDxeCallback(). Should I hook into that callback function
>>>> through a new BOOLEAN variable, such as "mDelayedMorLockInit" (and
>>>> then MorLockInit() would only set this variable to TRUE), or should I
>>>> install a separate callback?
>>>>
>>>> Either way, I don't think that I should do the MOR/MORL stuff in the
>>>> current SmmEndOfDxeCallback() function *unconditionally*, because
>>>> that callback is set up when the *read* half of the variable services
>>>> is initialized, but MORL only becomes relevant when the *write* half
>>>> of the variable services is initialized (which occurs in the
>>>> SmmFtwNotificationEvent() callback, i.e. when the FaultTolerantWrite
>>>> SMM protocol becomes available). Hence I think we need either a
>>>> separate callback registration, or a new boolean for the existent
>>>> callback.
>>>>
>>>> Thanks!
>>>> Laszlo

Re: multiple levels of support for MOR / MORLock

[Yao, Jiewen]

I believe it is legal that SMM code calls UEFI services before SMM_READY_TO_LOCK event.

For example, SMM_CORE calls gBS->LocateProtocol(), and mSecurity->FileAuthenticationState(), until SmmReadyToLock event triggered.
SMM_CPU driver calls gDS->GetMemorySpaceMap() at SmmReadyToLock event.

Because EndOfDxe happens before SmmReadyToLock, it is OK to call UEFI service at that point.

After SMM_READY_TO_LOCK, it is illegal to call UEFI services in SMM.

Thank you
Yao Jiewen


From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Sunday, October 1, 2017 4:23 PM
To: Yao, Jiewen <jiewen.yao@intel.com>
Cc: edk2-devel-01 <edk2-devel@lists.01.org>; Ladi Prosek <lprosek@redhat.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: Re: [edk2] multiple levels of support for MOR / MORLock

On 10/01/17 05:56, Yao, Jiewen wrote:
> Good test! Thanks for the investigation.
>
> That is a little annoying.

Agreed!

In a virtualization-only scenario, I wouldn't care much (it's very
unusual to install two OSes in the same VM and "dual boot" them), but on
physical machines, dual-booting Windows and Linux is very frequent, and
whatever we put in the Variable driver now will end up on physical
machines sooner or later.

> I still do not want to use PCD. My thought is below:
>
> Current MOR has TCG protocol dependence.   So can we add one more
> check - to check the existence of TCG/TCG2 protocol in variable
> driver? Both TCG and TCG2 are in mdepkg, there is no dependency
> issue.
>
> Even if MOR is present, but neither TCG nor TCG2 is present, don't
> create MORL but delete MOR. (Just in case people boot fedora, then
> boot windows) I believe that can handle the fedora issue.
According to the PI spec, "1.8 MM Traditional Driver Initialization", in
the VariableSmm driver I can only locate UEFI protocols in the entry
point function:

    [...] An MM Driver’s initialization phase ends when the entry point
    returns. [...] UEFI protocols can be located and used by MM drivers
    only during MM Initialization. [...]

The SmmEndOfDxe callback does not run as part of the VariableSmm
driver's entry point function (VariableServiceInitialize()), so I
believe I might not be allowed to call gBS->LocateProtocol() in the
callback. Is that correct?


Another option would be to register a UEFI protocol notify for the
TCG/TCG2 protocols, and to set a global variable (in SMRAM) when this
notify fires. Then the SmmEndOfDxe callback could check the variable.

However, what happens if the TCG/TCG2 protocols are installed *after*
PiSmmCpuDxeSmm is dispatched, and the SMM IPL closes SMRAM? Then the
protocol notify could never set the variable in SMRAM -- in fact, the
protocol notify couldn't even be invoked, because the notify function
itself is located in SMRAM (as part of the VariableSmm executable).


So, for this to work, I believe we'd have to modify the SMM_CORE to
"forward" the TCG/TCG2 UEFI protocols into the SMM protocol database
(similarly to EndOfDxe/SmmEndOfDxe, although in that case, EndOfDxe is a
UEFI event group, not a UEFI protocol).

Is my understanding correct? What do you suggest?


Thank you for taking the time to discuss this with me! If it's becoming
too long, we can continue after the weekend & the holidays end.

Cheers!
Laszlo

>> 在 2017年10月1日，上午4:33，Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> 写道：
>>
>> Hello Jiewen,
>>
>>> On 09/30/17 05:53, Yao, Jiewen wrote:
>>> Agree. Adding a hook function is OK for me, as long as the hook
>>> function implementation is in TcgMorLockXXX.c
>>
>> I've implemented this, but I've run into a problem while testing it.
>>
>> Your idea requires that *nothing* create the MOR variable other than
>>
>>  SecurityPkg/Tcg/MemoryOverwriteControl/TcgMor.inf
>>
>> Unfortunately, released Fedora operating systems break this assumption.
>> (It is a Linux kernel bug, no question about that, but it still breaks
>> the idea.)
>>
>> Namely, in the current upstream Linux kernel (the *upcoming* v4.14
>> release), there is a function called
>> efi_enable_reset_attack_mitigation(), added in commit ccc829ba3624b:
>>
>>  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ccc829ba3624b
>>
>>> /*
>>> * Enable reboot attack mitigation. This requests that the firmware clear the
>>> * RAM on next reboot before proceeding with boot, ensuring that any secrets
>>> * are cleared. If userland has ensured that all secrets have been removed
>>> * from RAM before reboot it can simply reset this variable.
>>> */
>>> void efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg)
>>> {
>>>        u8 val = 1;
>>>        efi_guid_t var_guid = MEMORY_ONLY_RESET_CONTROL_GUID;
>>>        efi_status_t status;
>>>        unsigned long datasize = 0;
>>>
>>>        status = get_efi_var(efi_MemoryOverWriteRequest_name, &var_guid,
>>>                             NULL, &datasize, NULL);
>>>
>>>        if (status == EFI_NOT_FOUND)
>>>                return;
>>>
>>>        set_efi_var(efi_MemoryOverWriteRequest_name, &var_guid,
>>>                    EFI_VARIABLE_NON_VOLATILE |
>>>                    EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>>                    EFI_VARIABLE_RUNTIME_ACCESS, sizeof(val), &val);
>>> }
>>
>> This function is correct in that it first checks whether the MOR
>> variable exists. If it doesn't exist, the function doesn't *create* the
>> variable.
>>
>> However, several Fedora distribution kernels *already* contain an
>> *earlier* version of the same function (which had never been merged into
>> upstream Linux), and that variant of the function is incorrect:
>>
>>  https://git.kernel.org/pub/scm/linux/kernel/git/jwboyer/fedora.git/commit/?id=65673e37e61d8
>>
>>> static void enable_reset_attack_mitigation(void)
>>> {
>>>        static const efi_guid_t var_guid = MEMORY_ONLY_RESET_CONTROL_GUID;
>>>        static const efi_char16_t MemoryOverwriteRequestControl_name[] = {
>>>                'M', 'e', 'm', 'o', 'r', 'y',
>>>                'O', 'v', 'e', 'r', 'w', 'r', 'i', 't', 'e',
>>>                'R', 'e', 'q', 'u', 'e', 's', 't',
>>>                'C', 'o', 'n', 't', 'r', 'o', 'l',
>>>                0
>>>        };
>>>        u8 val = 1;
>>>
>>>        /* Ignore the return value here - there's not really a lot we can do */
>>>        efi_call_runtime(set_variable,
>>>                        (efi_char16_t *)MemoryOverwriteRequestControl_name,
>>>                        (efi_guid_t *)&var_guid,
>>>                        EFI_VARIABLE_NON_VOLATILE |
>>>                        EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>>                        EFI_VARIABLE_RUNTIME_ACCESS,
>>>                        sizeof(val), val);
>>> }
>>
>> This variant does not check if the MOR variable exists.
>>
>> If the variable doesn't exist, then this function will create it
>> (incorrectly). At the next boot, the SmmEndOfDxe callback in
>> "TcgMorLockSmm.c" will think that the firmware platform includes support
>> for the MOR variable (it assumes that TcgMor.inf is part of the firmware
>> and that TcgMor.inf created the variable), and then the callback
>> creates/sets MorLock as well.
>>
>> Again, this is clearly a kernel bug, but it is part of at least the
>> following releases:
>>
>> - Fedora 26:
>>  https://git.kernel.org/pub/scm/linux/kernel/git/jwboyer/fedora.git/commit/?id=65673e37e61d
>>
>> - Fedora 25:
>>  https://git.kernel.org/pub/scm/linux/kernel/git/jwboyer/fedora.git/commit/?id=1f4e5e657685
>>
>> - Fedora 24:
>>  https://git.kernel.org/pub/scm/linux/kernel/git/jwboyer/fedora.git/commit/?id=677765d4db8e
>>
>> Thus the SmmEndOfDxe callback wouldn't work correctly together with
>> these Fedora releases.
>>
>> Thanks,
>> Laszlo
>>
>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>> Sent: Saturday, September 30, 2017 12:39 AM
>>> To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
>>> Cc: edk2-devel-01 <edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>>; Ladi Prosek <lprosek@redhat.com<mailto:lprosek@redhat.com>>
>>> Subject: Re: [edk2] multiple levels of support for MOR / MORLock
>>>
>>>> On 09/29/17 15:26, Yao, Jiewen wrote:
>>>> Sure. Feel free to submit patch.
>>>>
>>>> For the fix, I suggest we keep all MORL related code into
>>>> TcgMorLockXXX.c. I do not suggest we mix the MORL stuff into
>>>> VariableXXX.c.
>>>
>>> I've had more thoughts about this, since sending my last email.
>>>
>>> Currently SmmEndOfDxeCallback()
>>> [MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c] performs
>>> the following actions:
>>>
>>> - sets "mEndOfDxe" to TRUE
>>> - calls VarCheckLibInitializeAtEndOfDxe()
>>> - calls InitializeVariableQuota()
>>> - calls ReclaimForOS() optionally
>>>
>>> If we register a separate callback for gEfiSmmEndOfDxeProtocolGuid, in
>>> order to create the MORL variable, then that callback could be invoked
>>> after SmmEndOfDxeCallback(). I think it would not be optimal to create
>>> MORL after SmmEndOfDxeCallback() has run, even if it worked,
>>> technically speaking. This suggests to hook the MORL creation to the
>>> top of SmmEndOfDxeCallback(). What do you suggest?
>>>
>>>> A reminder that, the PRC team will have the PRC national holiday in
>>>> next week. As such, we won't have too much activity in next week.
>>>>
>>>> If you submit the patch, I will try to review in next week to unblock
>>>> the Linux work.
>>>
>>> Ah, sorry, I haven't been aware of the holidays.
>>>
>>> I might send a patch soon, but please don't let me disturb you during
>>> the holidays! Especially because this patch might need more discussion
>>> and more versins. Holidays are for getting rested from work! :) We can
>>> sort it out after you return.
>>>
>>>> In the patch, if you can add the detailed test result, that will be
>>>> very helpful for us. For example, test SMM/non-SMM version, test with
>>>> MOR present/absent, etc.
>>>
>>> For the "no MOR, no MORL" case, I'll have to rely on Ladi, for test
>>> results (Ladi offered to help test that case, thanks again for that,
>>> Ladi).
>>>
>>> However, for regression-testing the "both MOR and MORL" case, I can
>>> only rely on Intel developers and quality engineers -- OVMF doesn't
>>> have any TPM / MOR solution at the moment. (If we had one, then we
>>> wouldn't have found or reported this issue in the first place. :) )
>>>
>>> The fact that I can't regression-test the MOR+MORL case myself
>>> supports the idea that we should postpone the review and commit (if
>>> appropriate) of the patch until after the PRC national holiday.
>>>
>>> Thanks!
>>> Laszlo
>>>
>>>
>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
>>>> Sent: Friday, September 29, 2017 7:06 PM
>>>> To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com<mailto:jiewen.yao@intel.com%3cmailto:jiewen.yao@intel.com>>>
>>>> Cc: edk2-devel-01 <edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>>
>>>> Subject: Re: [edk2] multiple levels of support for MOR / MORLock
>>>>
>>>>> On 09/29/17 03:52, Yao, Jiewen wrote:
>>>>> 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.
>>>>
>>>> Sounds great; thanks a lot!
>>>>
>>>> I've filed:
>>>>
>>>>  https://bugzilla.tianocore.org/show_bug.cgi?id=727
>>>>
>>>> If possible I'd like the fix to be committed sometime next week. Is
>>>> it OK if I try and submit a patch soon? My plan is the following:
>>>>
>>>> - In MorLockInit() [TcgMorLockSmm.c], call
>>>> gSmst->SmmRegisterProtocolNotify() in order to register a callback
>>>> for gEfiSmmEndOfDxeProtocolGuid
>>>>
>>>> - In the callback function, call VariableServiceGetVariable(), with
>>>> size 0, to see if MOR is present -- if EFI_BUFFER_TOO_SMALL is
>>>> returned, MOR is present; otherwise MOR is absent.
>>>>
>>>> - If MOR is present, then call SetMorLockVariable(0) from the
>>>> callback function; like MorLockInit() does now. Otherwise, do
>>>> nothing.
>>>>
>>>> - It looks like there are no circumstances under which I should
>>>> de-register the callback. (I.e. call SmmRegisterProtocolNotify() with
>>>> a NULL Function argument.)
>>>>
>>>>
>>>> Now, I can see that VariableSmm.c already installs such a callback --
>>>> SmmEndOfDxeCallback(). Should I hook into that callback function
>>>> through a new BOOLEAN variable, such as "mDelayedMorLockInit" (and
>>>> then MorLockInit() would only set this variable to TRUE), or should I
>>>> install a separate callback?
>>>>
>>>> Either way, I don't think that I should do the MOR/MORL stuff in the
>>>> current SmmEndOfDxeCallback() function *unconditionally*, because
>>>> that callback is set up when the *read* half of the variable services
>>>> is initialized, but MORL only becomes relevant when the *write* half
>>>> of the variable services is initialized (which occurs in the
>>>> SmmFtwNotificationEvent() callback, i.e. when the FaultTolerantWrite
>>>> SMM protocol becomes available). Hence I think we need either a
>>>> separate callback registration, or a new boolean for the existent
>>>> callback.
>>>>
>>>> Thanks!
>>>> Laszlo

Re: multiple levels of support for MOR / MORLock

[Laszlo Ersek]

On 10/01/17 13:36, Yao, Jiewen wrote:
> I believe it is legal that SMM code calls UEFI services before SMM_READY_TO_LOCK event.
> 
> For example, SMM_CORE calls gBS->LocateProtocol(), and mSecurity->FileAuthenticationState(), until SmmReadyToLock event triggered.
> SMM_CPU driver calls gDS->GetMemorySpaceMap() at SmmReadyToLock event.
> 
> Because EndOfDxe happens before SmmReadyToLock, it is OK to call UEFI service at that point.
> 
> After SMM_READY_TO_LOCK, it is illegal to call UEFI services in SMM.

Great, thank you for confirming!

In OVMF, we do signal gEfiEndOfDxeEventGroupGuid before we install
gEfiDxeSmmReadyToLockProtocolGuid.

I'll go ahead and implement this as well then.

Cheers!
Laszlo

> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Sunday, October 1, 2017 4:23 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: edk2-devel-01 <edk2-devel@lists.01.org>; Ladi Prosek <lprosek@redhat.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: Re: [edk2] multiple levels of support for MOR / MORLock
> 
> On 10/01/17 05:56, Yao, Jiewen wrote:
>> Good test! Thanks for the investigation.
>>
>> That is a little annoying.
> 
> Agreed!
> 
> In a virtualization-only scenario, I wouldn't care much (it's very
> unusual to install two OSes in the same VM and "dual boot" them), but on
> physical machines, dual-booting Windows and Linux is very frequent, and
> whatever we put in the Variable driver now will end up on physical
> machines sooner or later.
> 
>> I still do not want to use PCD. My thought is below:
>>
>> Current MOR has TCG protocol dependence.   So can we add one more
>> check - to check the existence of TCG/TCG2 protocol in variable
>> driver? Both TCG and TCG2 are in mdepkg, there is no dependency
>> issue.
>>
>> Even if MOR is present, but neither TCG nor TCG2 is present, don't
>> create MORL but delete MOR. (Just in case people boot fedora, then
>> boot windows) I believe that can handle the fedora issue.
> According to the PI spec, "1.8 MM Traditional Driver Initialization", in
> the VariableSmm driver I can only locate UEFI protocols in the entry
> point function:
> 
>     [...] An MM Driver’s initialization phase ends when the entry point
>     returns. [...] UEFI protocols can be located and used by MM drivers
>     only during MM Initialization. [...]
> 
> The SmmEndOfDxe callback does not run as part of the VariableSmm
> driver's entry point function (VariableServiceInitialize()), so I
> believe I might not be allowed to call gBS->LocateProtocol() in the
> callback. Is that correct?
> 
> 
> Another option would be to register a UEFI protocol notify for the
> TCG/TCG2 protocols, and to set a global variable (in SMRAM) when this
> notify fires. Then the SmmEndOfDxe callback could check the variable.
> 
> However, what happens if the TCG/TCG2 protocols are installed *after*
> PiSmmCpuDxeSmm is dispatched, and the SMM IPL closes SMRAM? Then the
> protocol notify could never set the variable in SMRAM -- in fact, the
> protocol notify couldn't even be invoked, because the notify function
> itself is located in SMRAM (as part of the VariableSmm executable).
> 
> 
> So, for this to work, I believe we'd have to modify the SMM_CORE to
> "forward" the TCG/TCG2 UEFI protocols into the SMM protocol database
> (similarly to EndOfDxe/SmmEndOfDxe, although in that case, EndOfDxe is a
> UEFI event group, not a UEFI protocol).
> 
> Is my understanding correct? What do you suggest?
> 
> 
> Thank you for taking the time to discuss this with me! If it's becoming
> too long, we can continue after the weekend & the holidays end.
> 
> Cheers!
> Laszlo
> 
>>> 在 2017年10月1日，上午4:33，Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> 写道：
>>>
>>> Hello Jiewen,
>>>
>>>> On 09/30/17 05:53, Yao, Jiewen wrote:
>>>> Agree. Adding a hook function is OK for me, as long as the hook
>>>> function implementation is in TcgMorLockXXX.c
>>>
>>> I've implemented this, but I've run into a problem while testing it.
>>>
>>> Your idea requires that *nothing* create the MOR variable other than
>>>
>>>  SecurityPkg/Tcg/MemoryOverwriteControl/TcgMor.inf
>>>
>>> Unfortunately, released Fedora operating systems break this assumption.
>>> (It is a Linux kernel bug, no question about that, but it still breaks
>>> the idea.)
>>>
>>> Namely, in the current upstream Linux kernel (the *upcoming* v4.14
>>> release), there is a function called
>>> efi_enable_reset_attack_mitigation(), added in commit ccc829ba3624b:
>>>
>>>  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ccc829ba3624b
>>>
>>>> /*
>>>> * Enable reboot attack mitigation. This requests that the firmware clear the
>>>> * RAM on next reboot before proceeding with boot, ensuring that any secrets
>>>> * are cleared. If userland has ensured that all secrets have been removed
>>>> * from RAM before reboot it can simply reset this variable.
>>>> */
>>>> void efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg)
>>>> {
>>>>        u8 val = 1;
>>>>        efi_guid_t var_guid = MEMORY_ONLY_RESET_CONTROL_GUID;
>>>>        efi_status_t status;
>>>>        unsigned long datasize = 0;
>>>>
>>>>        status = get_efi_var(efi_MemoryOverWriteRequest_name, &var_guid,
>>>>                             NULL, &datasize, NULL);
>>>>
>>>>        if (status == EFI_NOT_FOUND)
>>>>                return;
>>>>
>>>>        set_efi_var(efi_MemoryOverWriteRequest_name, &var_guid,
>>>>                    EFI_VARIABLE_NON_VOLATILE |
>>>>                    EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>>>                    EFI_VARIABLE_RUNTIME_ACCESS, sizeof(val), &val);
>>>> }
>>>
>>> This function is correct in that it first checks whether the MOR
>>> variable exists. If it doesn't exist, the function doesn't *create* the
>>> variable.
>>>
>>> However, several Fedora distribution kernels *already* contain an
>>> *earlier* version of the same function (which had never been merged into
>>> upstream Linux), and that variant of the function is incorrect:
>>>
>>>  https://git.kernel.org/pub/scm/linux/kernel/git/jwboyer/fedora.git/commit/?id=65673e37e61d8
>>>
>>>> static void enable_reset_attack_mitigation(void)
>>>> {
>>>>        static const efi_guid_t var_guid = MEMORY_ONLY_RESET_CONTROL_GUID;
>>>>        static const efi_char16_t MemoryOverwriteRequestControl_name[] = {
>>>>                'M', 'e', 'm', 'o', 'r', 'y',
>>>>                'O', 'v', 'e', 'r', 'w', 'r', 'i', 't', 'e',
>>>>                'R', 'e', 'q', 'u', 'e', 's', 't',
>>>>                'C', 'o', 'n', 't', 'r', 'o', 'l',
>>>>                0
>>>>        };
>>>>        u8 val = 1;
>>>>
>>>>        /* Ignore the return value here - there's not really a lot we can do */
>>>>        efi_call_runtime(set_variable,
>>>>                        (efi_char16_t *)MemoryOverwriteRequestControl_name,
>>>>                        (efi_guid_t *)&var_guid,
>>>>                        EFI_VARIABLE_NON_VOLATILE |
>>>>                        EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>>>                        EFI_VARIABLE_RUNTIME_ACCESS,
>>>>                        sizeof(val), val);
>>>> }
>>>
>>> This variant does not check if the MOR variable exists.
>>>
>>> If the variable doesn't exist, then this function will create it
>>> (incorrectly). At the next boot, the SmmEndOfDxe callback in
>>> "TcgMorLockSmm.c" will think that the firmware platform includes support
>>> for the MOR variable (it assumes that TcgMor.inf is part of the firmware
>>> and that TcgMor.inf created the variable), and then the callback
>>> creates/sets MorLock as well.
>>>
>>> Again, this is clearly a kernel bug, but it is part of at least the
>>> following releases:
>>>
>>> - Fedora 26:
>>>  https://git.kernel.org/pub/scm/linux/kernel/git/jwboyer/fedora.git/commit/?id=65673e37e61d
>>>
>>> - Fedora 25:
>>>  https://git.kernel.org/pub/scm/linux/kernel/git/jwboyer/fedora.git/commit/?id=1f4e5e657685
>>>
>>> - Fedora 24:
>>>  https://git.kernel.org/pub/scm/linux/kernel/git/jwboyer/fedora.git/commit/?id=677765d4db8e
>>>
>>> Thus the SmmEndOfDxe callback wouldn't work correctly together with
>>> these Fedora releases.
>>>
>>> Thanks,
>>> Laszlo
>>>
>>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>>> Sent: Saturday, September 30, 2017 12:39 AM
>>>> To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
>>>> Cc: edk2-devel-01 <edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>>; Ladi Prosek <lprosek@redhat.com<mailto:lprosek@redhat.com>>
>>>> Subject: Re: [edk2] multiple levels of support for MOR / MORLock
>>>>
>>>>> On 09/29/17 15:26, Yao, Jiewen wrote:
>>>>> Sure. Feel free to submit patch.
>>>>>
>>>>> For the fix, I suggest we keep all MORL related code into
>>>>> TcgMorLockXXX.c. I do not suggest we mix the MORL stuff into
>>>>> VariableXXX.c.
>>>>
>>>> I've had more thoughts about this, since sending my last email.
>>>>
>>>> Currently SmmEndOfDxeCallback()
>>>> [MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c] performs
>>>> the following actions:
>>>>
>>>> - sets "mEndOfDxe" to TRUE
>>>> - calls VarCheckLibInitializeAtEndOfDxe()
>>>> - calls InitializeVariableQuota()
>>>> - calls ReclaimForOS() optionally
>>>>
>>>> If we register a separate callback for gEfiSmmEndOfDxeProtocolGuid, in
>>>> order to create the MORL variable, then that callback could be invoked
>>>> after SmmEndOfDxeCallback(). I think it would not be optimal to create
>>>> MORL after SmmEndOfDxeCallback() has run, even if it worked,
>>>> technically speaking. This suggests to hook the MORL creation to the
>>>> top of SmmEndOfDxeCallback(). What do you suggest?
>>>>
>>>>> A reminder that, the PRC team will have the PRC national holiday in
>>>>> next week. As such, we won't have too much activity in next week.
>>>>>
>>>>> If you submit the patch, I will try to review in next week to unblock
>>>>> the Linux work.
>>>>
>>>> Ah, sorry, I haven't been aware of the holidays.
>>>>
>>>> I might send a patch soon, but please don't let me disturb you during
>>>> the holidays! Especially because this patch might need more discussion
>>>> and more versins. Holidays are for getting rested from work! :) We can
>>>> sort it out after you return.
>>>>
>>>>> In the patch, if you can add the detailed test result, that will be
>>>>> very helpful for us. For example, test SMM/non-SMM version, test with
>>>>> MOR present/absent, etc.
>>>>
>>>> For the "no MOR, no MORL" case, I'll have to rely on Ladi, for test
>>>> results (Ladi offered to help test that case, thanks again for that,
>>>> Ladi).
>>>>
>>>> However, for regression-testing the "both MOR and MORL" case, I can
>>>> only rely on Intel developers and quality engineers -- OVMF doesn't
>>>> have any TPM / MOR solution at the moment. (If we had one, then we
>>>> wouldn't have found or reported this issue in the first place. :) )
>>>>
>>>> The fact that I can't regression-test the MOR+MORL case myself
>>>> supports the idea that we should postpone the review and commit (if
>>>> appropriate) of the patch until after the PRC national holiday.
>>>>
>>>> Thanks!
>>>> Laszlo
>>>>
>>>>
>>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
>>>>> Sent: Friday, September 29, 2017 7:06 PM
>>>>> To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com<mailto:jiewen.yao@intel.com%3cmailto:jiewen.yao@intel.com>>>
>>>>> Cc: edk2-devel-01 <edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>>
>>>>> Subject: Re: [edk2] multiple levels of support for MOR / MORLock
>>>>>
>>>>>> On 09/29/17 03:52, Yao, Jiewen wrote:
>>>>>> 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.
>>>>>
>>>>> Sounds great; thanks a lot!
>>>>>
>>>>> I've filed:
>>>>>
>>>>>  https://bugzilla.tianocore.org/show_bug.cgi?id=727
>>>>>
>>>>> If possible I'd like the fix to be committed sometime next week. Is
>>>>> it OK if I try and submit a patch soon? My plan is the following:
>>>>>
>>>>> - In MorLockInit() [TcgMorLockSmm.c], call
>>>>> gSmst->SmmRegisterProtocolNotify() in order to register a callback
>>>>> for gEfiSmmEndOfDxeProtocolGuid
>>>>>
>>>>> - In the callback function, call VariableServiceGetVariable(), with
>>>>> size 0, to see if MOR is present -- if EFI_BUFFER_TOO_SMALL is
>>>>> returned, MOR is present; otherwise MOR is absent.
>>>>>
>>>>> - If MOR is present, then call SetMorLockVariable(0) from the
>>>>> callback function; like MorLockInit() does now. Otherwise, do
>>>>> nothing.
>>>>>
>>>>> - It looks like there are no circumstances under which I should
>>>>> de-register the callback. (I.e. call SmmRegisterProtocolNotify() with
>>>>> a NULL Function argument.)
>>>>>
>>>>>
>>>>> Now, I can see that VariableSmm.c already installs such a callback --
>>>>> SmmEndOfDxeCallback(). Should I hook into that callback function
>>>>> through a new BOOLEAN variable, such as "mDelayedMorLockInit" (and
>>>>> then MorLockInit() would only set this variable to TRUE), or should I
>>>>> install a separate callback?
>>>>>
>>>>> Either way, I don't think that I should do the MOR/MORL stuff in the
>>>>> current SmmEndOfDxeCallback() function *unconditionally*, because
>>>>> that callback is set up when the *read* half of the variable services
>>>>> is initialized, but MORL only becomes relevant when the *write* half
>>>>> of the variable services is initialized (which occurs in the
>>>>> SmmFtwNotificationEvent() callback, i.e. when the FaultTolerantWrite
>>>>> SMM protocol becomes available). Hence I think we need either a
>>>>> separate callback registration, or a new boolean for the existent
>>>>> callback.
>>>>>
>>>>> Thanks!
>>>>> Laszlo