Repo:   https://github.com/lersek/edk2.git
Branch: mor_lock_init_at_end_of_dxe

This patch set fixes the issue reported in the following items:

* Inconsistent MOR control variables exposed by OVMF, breaks Windows
  Device Guard

  https://bugzilla.redhat.com/show_bug.cgi?id=1496170

* VariableSmm MorLockInit(): create MORLock only if / after MOR exists

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

Patches #1 through #3 are cleanups.

Patch #4 is a small helper patch for patch #5.

Patch #5 is the actual fix, following Jiewen's suggestions from the
edk2-devel thread

* [edk2] multiple levels of support for MOR / MORLock

  https://lists.01.org/pipermail/edk2-devel/2017-September/015444.html
  https://lists.01.org/pipermail/edk2-devel/2017-October/015530.html

Patch #6 is a workaround for some OSes (minimally Fedora 24-26, and some
Debian versions) that create the MOR variable even if the platform
doesn't offer it up-front. This patch also follows Jiewen's suggestion
from the same edk2-devel thread.

(

BTW, at Paolo's recommendation, I've now reported this kernel issue for
Fedora, under

* incorrect downstream-only Platform Reset Attack Mitigation patch in
  the F24-F26 kernels

  https://bugzilla.redhat.com/show_bug.cgi?id=1498159

)

I've checked this set for basic regressions, using OVMF, normal boot and
S3 suspend/resume:

* Q35, SMM, IA32:
  - Fedora 25 -- verified patch #6 specifically

* i440fx, no SMM, X64:
  - Fedora 24

* Q35, SMM, IA32X64:
  - Fedora 26 -- verified patch #6 specifically
  - Windows 7
  - Windows 8.1
  - Windows 10
  - Windows Server 2008 R2
  - Windows Server 2012 R2

I didn't / couldn't test this set in the following two environments:

- on platforms where TcgMor.inf is included in the firmware, and the MOR
  variable exists genuinely,

- in the nested virt setup where Ladi reported the Device Guard
  breakage. (If I understand correctly, ATM this requires additional
  host kernel (KVM) patches.)

Test results / feedback from those envs would be appreciated.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ladi Prosek <lprosek@redhat.com>
Cc: Star Zeng <star.zeng@intel.com>

Thanks,
Laszlo

Laszlo Ersek (6):
  MdeModulePkg/Variable/RuntimeDxe: move SecureBootHook() decl to new
    header
  MdeModulePkg/Variable/RuntimeDxe: move MOR func. declarations to
    header
  MdeModulePkg/Variable/RuntimeDxe: introduce MorLockInitAtEndOfDxe()
    hook
  MdeModulePkg/Variable/RuntimeDxe: permit MorLock deletion for passthru
    req
  MdeModulePkg/Variable/RuntimeDxe: delay MorLock creation until
    EndOfDxe
  MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR
    variable

 MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c             |   2 +
 MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h    |  89 ++++++++++
 MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c           |  45 +++--
 MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c           | 173 ++++++++++++++++++--
 MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c                |  51 ------
 MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h                |   2 +
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c             |   2 +
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf    |   1 +
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c             |   2 +
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf           |   4 +
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c   |  16 +-
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf |   1 +
 12 files changed, 294 insertions(+), 94 deletions(-)
 create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h

-- 
2.14.1.3.gb7cf6e02401b

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Thanks. Laszlo.
This patch looks good to me in general.

One minor comment is below:
MorLockInitAtEndOfDxe()
+  if (!mMorLockInitializationRequired) {
+    //
+    // The EFI_SMM_FAULT_TOLERANT_WRITE_PROTOCOL has never been installed, thus
+    // the variable write service is unavailable. Do nothing.
+    //
+    return;
+  }
I think it is an illegal case. I would add ASSERT before return.


And I hope Ladi can help us double confirm if this patch fixed the device guard problem. :-)

Thank you
Yao Jiewen


> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, October 4, 2017 5:28 AM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Ladi
> Prosek <lprosek@redhat.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock
> inconsistency
> 
> Repo:   https://github.com/lersek/edk2.git
> Branch: mor_lock_init_at_end_of_dxe
> 
> This patch set fixes the issue reported in the following items:
> 
> * Inconsistent MOR control variables exposed by OVMF, breaks Windows
>   Device Guard
> 
>   https://bugzilla.redhat.com/show_bug.cgi?id=1496170
> 
> * VariableSmm MorLockInit(): create MORLock only if / after MOR exists
> 
>   https://bugzilla.tianocore.org/show_bug.cgi?id=727
> 
> Patches #1 through #3 are cleanups.
> 
> Patch #4 is a small helper patch for patch #5.
> 
> Patch #5 is the actual fix, following Jiewen's suggestions from the
> edk2-devel thread
> 
> * [edk2] multiple levels of support for MOR / MORLock
> 
>   https://lists.01.org/pipermail/edk2-devel/2017-September/015444.html
>   https://lists.01.org/pipermail/edk2-devel/2017-October/015530.html
> 
> Patch #6 is a workaround for some OSes (minimally Fedora 24-26, and some
> Debian versions) that create the MOR variable even if the platform
> doesn't offer it up-front. This patch also follows Jiewen's suggestion
> from the same edk2-devel thread.
> 
> (
> 
> BTW, at Paolo's recommendation, I've now reported this kernel issue for
> Fedora, under
> 
> * incorrect downstream-only Platform Reset Attack Mitigation patch in
>   the F24-F26 kernels
> 
>   https://bugzilla.redhat.com/show_bug.cgi?id=1498159
> 
> )
> 
> I've checked this set for basic regressions, using OVMF, normal boot and
> S3 suspend/resume:
> 
> * Q35, SMM, IA32:
>   - Fedora 25 -- verified patch #6 specifically
> 
> * i440fx, no SMM, X64:
>   - Fedora 24
> 
> * Q35, SMM, IA32X64:
>   - Fedora 26 -- verified patch #6 specifically
>   - Windows 7
>   - Windows 8.1
>   - Windows 10
>   - Windows Server 2008 R2
>   - Windows Server 2012 R2
> 
> I didn't / couldn't test this set in the following two environments:
> 
> - on platforms where TcgMor.inf is included in the firmware, and the MOR
>   variable exists genuinely,
> 
> - in the nested virt setup where Ladi reported the Device Guard
>   breakage. (If I understand correctly, ATM this requires additional
>   host kernel (KVM) patches.)
> 
> Test results / feedback from those envs would be appreciated.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Ladi Prosek <lprosek@redhat.com>
> Cc: Star Zeng <star.zeng@intel.com>
> 
> Thanks,
> Laszlo
> 
> Laszlo Ersek (6):
>   MdeModulePkg/Variable/RuntimeDxe: move SecureBootHook() decl to new
>     header
>   MdeModulePkg/Variable/RuntimeDxe: move MOR func. declarations to
>     header
>   MdeModulePkg/Variable/RuntimeDxe: introduce MorLockInitAtEndOfDxe()
>     hook
>   MdeModulePkg/Variable/RuntimeDxe: permit MorLock deletion for passthru
>     req
>   MdeModulePkg/Variable/RuntimeDxe: delay MorLock creation until
>     EndOfDxe
>   MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR
>     variable
> 
>  MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c
> |   2 +
>  MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h    |
> 89 ++++++++++
>  MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
> |  45 +++--
>  MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> | 173 ++++++++++++++++++--
>  MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c                |
> 51 ------
>  MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h                |
> 2 +
>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c             |
> 2 +
>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf    |
> 1 +
>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> |   2 +
>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> |   4 +
>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c
> |  16 +-
>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf
> |   1 +
>  12 files changed, 294 insertions(+), 94 deletions(-)
>  create mode 100644
> MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
> 
> --
> 2.14.1.3.gb7cf6e02401b

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 10/04/17 03:18, Yao, Jiewen wrote:
> Thanks. Laszlo.
> This patch looks good to me in general.
> 
> One minor comment is below:
> MorLockInitAtEndOfDxe()
> +  if (!mMorLockInitializationRequired) {
> +    //
> +    // The EFI_SMM_FAULT_TOLERANT_WRITE_PROTOCOL has never been installed, thus
> +    // the variable write service is unavailable. Do nothing.
> +    //
> +    return;
> +  }
> I think it is an illegal case. I would add ASSERT before return.

OK, I will replace the sentence "Do nothing." with "This should never
happen.", and also add ASSERT (FALSE) above the "return".

> And I hope Ladi can help us double confirm if this patch fixed the device guard problem. :-)

Right!

Ladi stated in the RHBZ that he had built OVMF; I'll talk with him
off-list regardless, to see if I can help with anything.

Thank you Jiewen!
Laszlo


> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Wednesday, October 4, 2017 5:28 AM
>> To: edk2-devel-01 <edk2-devel@lists.01.org>
>> Cc: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Ladi
>> Prosek <lprosek@redhat.com>; Zeng, Star <star.zeng@intel.com>
>> Subject: [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock
>> inconsistency
>>
>> Repo:   https://github.com/lersek/edk2.git
>> Branch: mor_lock_init_at_end_of_dxe
>>
>> This patch set fixes the issue reported in the following items:
>>
>> * Inconsistent MOR control variables exposed by OVMF, breaks Windows
>>   Device Guard
>>
>>   https://bugzilla.redhat.com/show_bug.cgi?id=1496170
>>
>> * VariableSmm MorLockInit(): create MORLock only if / after MOR exists
>>
>>   https://bugzilla.tianocore.org/show_bug.cgi?id=727
>>
>> Patches #1 through #3 are cleanups.
>>
>> Patch #4 is a small helper patch for patch #5.
>>
>> Patch #5 is the actual fix, following Jiewen's suggestions from the
>> edk2-devel thread
>>
>> * [edk2] multiple levels of support for MOR / MORLock
>>
>>   https://lists.01.org/pipermail/edk2-devel/2017-September/015444.html
>>   https://lists.01.org/pipermail/edk2-devel/2017-October/015530.html
>>
>> Patch #6 is a workaround for some OSes (minimally Fedora 24-26, and some
>> Debian versions) that create the MOR variable even if the platform
>> doesn't offer it up-front. This patch also follows Jiewen's suggestion
>> from the same edk2-devel thread.
>>
>> (
>>
>> BTW, at Paolo's recommendation, I've now reported this kernel issue for
>> Fedora, under
>>
>> * incorrect downstream-only Platform Reset Attack Mitigation patch in
>>   the F24-F26 kernels
>>
>>   https://bugzilla.redhat.com/show_bug.cgi?id=1498159
>>
>> )
>>
>> I've checked this set for basic regressions, using OVMF, normal boot and
>> S3 suspend/resume:
>>
>> * Q35, SMM, IA32:
>>   - Fedora 25 -- verified patch #6 specifically
>>
>> * i440fx, no SMM, X64:
>>   - Fedora 24
>>
>> * Q35, SMM, IA32X64:
>>   - Fedora 26 -- verified patch #6 specifically
>>   - Windows 7
>>   - Windows 8.1
>>   - Windows 10
>>   - Windows Server 2008 R2
>>   - Windows Server 2012 R2
>>
>> I didn't / couldn't test this set in the following two environments:
>>
>> - on platforms where TcgMor.inf is included in the firmware, and the MOR
>>   variable exists genuinely,
>>
>> - in the nested virt setup where Ladi reported the Device Guard
>>   breakage. (If I understand correctly, ATM this requires additional
>>   host kernel (KVM) patches.)
>>
>> Test results / feedback from those envs would be appreciated.
>>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Ladi Prosek <lprosek@redhat.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>>
>> Thanks,
>> Laszlo
>>
>> Laszlo Ersek (6):
>>   MdeModulePkg/Variable/RuntimeDxe: move SecureBootHook() decl to new
>>     header
>>   MdeModulePkg/Variable/RuntimeDxe: move MOR func. declarations to
>>     header
>>   MdeModulePkg/Variable/RuntimeDxe: introduce MorLockInitAtEndOfDxe()
>>     hook
>>   MdeModulePkg/Variable/RuntimeDxe: permit MorLock deletion for passthru
>>     req
>>   MdeModulePkg/Variable/RuntimeDxe: delay MorLock creation until
>>     EndOfDxe
>>   MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR
>>     variable
>>
>>  MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c
>> |   2 +
>>  MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h    |
>> 89 ++++++++++
>>  MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
>> |  45 +++--
>>  MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
>> | 173 ++++++++++++++++++--
>>  MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c                |
>> 51 ------
>>  MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h                |
>> 2 +
>>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c             |
>> 2 +
>>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf    |
>> 1 +
>>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
>> |   2 +
>>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
>> |   4 +
>>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c
>> |  16 +-
>>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf
>> |   1 +
>>  12 files changed, 294 insertions(+), 94 deletions(-)
>>  create mode 100644
>> MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
>>
>> --
>> 2.14.1.3.gb7cf6e02401b
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On Wed, Oct 4, 2017 at 12:39 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> On 10/04/17 03:18, Yao, Jiewen wrote:
>> Thanks. Laszlo.
>> This patch looks good to me in general.
>>
>> One minor comment is below:
>> MorLockInitAtEndOfDxe()
>> +  if (!mMorLockInitializationRequired) {
>> +    //
>> +    // The EFI_SMM_FAULT_TOLERANT_WRITE_PROTOCOL has never been installed, thus
>> +    // the variable write service is unavailable. Do nothing.
>> +    //
>> +    return;
>> +  }
>> I think it is an illegal case. I would add ASSERT before return.
>
> OK, I will replace the sentence "Do nothing." with "This should never
> happen.", and also add ASSERT (FALSE) above the "return".
>
>> And I hope Ladi can help us double confirm if this patch fixed the device guard problem. :-)
>
> Right!

I have tested the patch and can confirm that it fixes the Windows
device guard problem.

Tested-by: Ladi Prosek <lprosek@redhat.com>

> Ladi stated in the RHBZ that he had built OVMF; I'll talk with him
> off-list regardless, to see if I can help with anything.

This was my first time building OVMF on my machine (the builds I had
done before filing the RHBZ were all using Red Hat's internal build
service). The only thing that threw me off was -D SMM_REQUIRE which I
was initially missing so I couldn't reproduce the issue on baseline.

> Thank you Jiewen!
> Laszlo

Thank you Laszlo and Jiewen for the very quick turnaround on this!
Ladi

>
>>
>>> -----Original Message-----
>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>> Sent: Wednesday, October 4, 2017 5:28 AM
>>> To: edk2-devel-01 <edk2-devel@lists.01.org>
>>> Cc: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Ladi
>>> Prosek <lprosek@redhat.com>; Zeng, Star <star.zeng@intel.com>
>>> Subject: [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock
>>> inconsistency
>>>
>>> Repo:   https://github.com/lersek/edk2.git
>>> Branch: mor_lock_init_at_end_of_dxe
>>>
>>> This patch set fixes the issue reported in the following items:
>>>
>>> * Inconsistent MOR control variables exposed by OVMF, breaks Windows
>>>   Device Guard
>>>
>>>   https://bugzilla.redhat.com/show_bug.cgi?id=1496170
>>>
>>> * VariableSmm MorLockInit(): create MORLock only if / after MOR exists
>>>
>>>   https://bugzilla.tianocore.org/show_bug.cgi?id=727
>>>
>>> Patches #1 through #3 are cleanups.
>>>
>>> Patch #4 is a small helper patch for patch #5.
>>>
>>> Patch #5 is the actual fix, following Jiewen's suggestions from the
>>> edk2-devel thread
>>>
>>> * [edk2] multiple levels of support for MOR / MORLock
>>>
>>>   https://lists.01.org/pipermail/edk2-devel/2017-September/015444.html
>>>   https://lists.01.org/pipermail/edk2-devel/2017-October/015530.html
>>>
>>> Patch #6 is a workaround for some OSes (minimally Fedora 24-26, and some
>>> Debian versions) that create the MOR variable even if the platform
>>> doesn't offer it up-front. This patch also follows Jiewen's suggestion
>>> from the same edk2-devel thread.
>>>
>>> (
>>>
>>> BTW, at Paolo's recommendation, I've now reported this kernel issue for
>>> Fedora, under
>>>
>>> * incorrect downstream-only Platform Reset Attack Mitigation patch in
>>>   the F24-F26 kernels
>>>
>>>   https://bugzilla.redhat.com/show_bug.cgi?id=1498159
>>>
>>> )
>>>
>>> I've checked this set for basic regressions, using OVMF, normal boot and
>>> S3 suspend/resume:
>>>
>>> * Q35, SMM, IA32:
>>>   - Fedora 25 -- verified patch #6 specifically
>>>
>>> * i440fx, no SMM, X64:
>>>   - Fedora 24
>>>
>>> * Q35, SMM, IA32X64:
>>>   - Fedora 26 -- verified patch #6 specifically
>>>   - Windows 7
>>>   - Windows 8.1
>>>   - Windows 10
>>>   - Windows Server 2008 R2
>>>   - Windows Server 2012 R2
>>>
>>> I didn't / couldn't test this set in the following two environments:
>>>
>>> - on platforms where TcgMor.inf is included in the firmware, and the MOR
>>>   variable exists genuinely,
>>>
>>> - in the nested virt setup where Ladi reported the Device Guard
>>>   breakage. (If I understand correctly, ATM this requires additional
>>>   host kernel (KVM) patches.)
>>>
>>> Test results / feedback from those envs would be appreciated.
>>>
>>> Cc: Eric Dong <eric.dong@intel.com>
>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>> Cc: Ladi Prosek <lprosek@redhat.com>
>>> Cc: Star Zeng <star.zeng@intel.com>
>>>
>>> Thanks,
>>> Laszlo
>>>
>>> Laszlo Ersek (6):
>>>   MdeModulePkg/Variable/RuntimeDxe: move SecureBootHook() decl to new
>>>     header
>>>   MdeModulePkg/Variable/RuntimeDxe: move MOR func. declarations to
>>>     header
>>>   MdeModulePkg/Variable/RuntimeDxe: introduce MorLockInitAtEndOfDxe()
>>>     hook
>>>   MdeModulePkg/Variable/RuntimeDxe: permit MorLock deletion for passthru
>>>     req
>>>   MdeModulePkg/Variable/RuntimeDxe: delay MorLock creation until
>>>     EndOfDxe
>>>   MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR
>>>     variable
>>>
>>>  MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c
>>> |   2 +
>>>  MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h    |
>>> 89 ++++++++++
>>>  MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
>>> |  45 +++--
>>>  MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
>>> | 173 ++++++++++++++++++--
>>>  MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c                |
>>> 51 ------
>>>  MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h                |
>>> 2 +
>>>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c             |
>>> 2 +
>>>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf    |
>>> 1 +
>>>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
>>> |   2 +
>>>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
>>> |   4 +
>>>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c
>>> |  16 +-
>>>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf
>>> |   1 +
>>>  12 files changed, 294 insertions(+), 94 deletions(-)
>>>  create mode 100644
>>> MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
>>>
>>> --
>>> 2.14.1.3.gb7cf6e02401b
>>
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Thanks.
Please use ASSERT() when you check in.

Series Reviewed-by: Jiewen.yao@intel.com<mailto:Jiewen.yao@intel.com>


From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Wednesday, October 4, 2017 6:40 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Dong, Eric <eric.dong@intel.com>; Ladi Prosek <lprosek@redhat.com>; Zeng, Star <star.zeng@intel.com>
Subject: Re: [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency

On 10/04/17 03:18, Yao, Jiewen wrote:
> Thanks. Laszlo.
> This patch looks good to me in general.
>
> One minor comment is below:
> MorLockInitAtEndOfDxe()
> +  if (!mMorLockInitializationRequired) {
> +    //
> +    // The EFI_SMM_FAULT_TOLERANT_WRITE_PROTOCOL has never been installed, thus
> +    // the variable write service is unavailable. Do nothing.
> +    //
> +    return;
> +  }
> I think it is an illegal case. I would add ASSERT before return.

OK, I will replace the sentence "Do nothing." with "This should never
happen.", and also add ASSERT (FALSE) above the "return".

> And I hope Ladi can help us double confirm if this patch fixed the device guard problem. :-)

Right!

Ladi stated in the RHBZ that he had built OVMF; I'll talk with him
off-list regardless, to see if I can help with anything.

Thank you Jiewen!
Laszlo


>
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Wednesday, October 4, 2017 5:28 AM
>> To: edk2-devel-01 <edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>>
>> Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Ladi
>> Prosek <lprosek@redhat.com<mailto:lprosek@redhat.com>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>>
>> Subject: [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock
>> inconsistency
>>
>> Repo:   https://github.com/lersek/edk2.git
>> Branch: mor_lock_init_at_end_of_dxe
>>
>> This patch set fixes the issue reported in the following items:
>>
>> * Inconsistent MOR control variables exposed by OVMF, breaks Windows
>>   Device Guard
>>
>>   https://bugzilla.redhat.com/show_bug.cgi?id=1496170
>>
>> * VariableSmm MorLockInit(): create MORLock only if / after MOR exists
>>
>>   https://bugzilla.tianocore.org/show_bug.cgi?id=727
>>
>> Patches #1 through #3 are cleanups.
>>
>> Patch #4 is a small helper patch for patch #5.
>>
>> Patch #5 is the actual fix, following Jiewen's suggestions from the
>> edk2-devel thread
>>
>> * [edk2] multiple levels of support for MOR / MORLock
>>
>>   https://lists.01.org/pipermail/edk2-devel/2017-September/015444.html
>>   https://lists.01.org/pipermail/edk2-devel/2017-October/015530.html
>>
>> Patch #6 is a workaround for some OSes (minimally Fedora 24-26, and some
>> Debian versions) that create the MOR variable even if the platform
>> doesn't offer it up-front. This patch also follows Jiewen's suggestion
>> from the same edk2-devel thread.
>>
>> (
>>
>> BTW, at Paolo's recommendation, I've now reported this kernel issue for
>> Fedora, under
>>
>> * incorrect downstream-only Platform Reset Attack Mitigation patch in
>>   the F24-F26 kernels
>>
>>   https://bugzilla.redhat.com/show_bug.cgi?id=1498159
>>
>> )
>>
>> I've checked this set for basic regressions, using OVMF, normal boot and
>> S3 suspend/resume:
>>
>> * Q35, SMM, IA32:
>>   - Fedora 25 -- verified patch #6 specifically
>>
>> * i440fx, no SMM, X64:
>>   - Fedora 24
>>
>> * Q35, SMM, IA32X64:
>>   - Fedora 26 -- verified patch #6 specifically
>>   - Windows 7
>>   - Windows 8.1
>>   - Windows 10
>>   - Windows Server 2008 R2
>>   - Windows Server 2012 R2
>>
>> I didn't / couldn't test this set in the following two environments:
>>
>> - on platforms where TcgMor.inf is included in the firmware, and the MOR
>>   variable exists genuinely,
>>
>> - in the nested virt setup where Ladi reported the Device Guard
>>   breakage. (If I understand correctly, ATM this requires additional
>>   host kernel (KVM) patches.)
>>
>> Test results / feedback from those envs would be appreciated.
>>
>> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>>
>> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
>> Cc: Ladi Prosek <lprosek@redhat.com<mailto:lprosek@redhat.com>>
>> Cc: Star Zeng <star.zeng@intel.com<mailto:star.zeng@intel.com>>
>>
>> Thanks,
>> Laszlo
>>
>> Laszlo Ersek (6):
>>   MdeModulePkg/Variable/RuntimeDxe: move SecureBootHook() decl to new
>>     header
>>   MdeModulePkg/Variable/RuntimeDxe: move MOR func. declarations to
>>     header
>>   MdeModulePkg/Variable/RuntimeDxe: introduce MorLockInitAtEndOfDxe()
>>     hook
>>   MdeModulePkg/Variable/RuntimeDxe: permit MorLock deletion for passthru
>>     req
>>   MdeModulePkg/Variable/RuntimeDxe: delay MorLock creation until
>>     EndOfDxe
>>   MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR
>>     variable
>>
>>  MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c
>> |   2 +
>>  MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h    |
>> 89 ++++++++++
>>  MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
>> |  45 +++--
>>  MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
>> | 173 ++++++++++++++++++--
>>  MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c                |
>> 51 ------
>>  MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h                |
>> 2 +
>>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c             |
>> 2 +
>>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf    |
>> 1 +
>>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
>> |   2 +
>>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
>> |   4 +
>>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c
>> |  16 +-
>>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf
>> |   1 +
>>  12 files changed, 294 insertions(+), 94 deletions(-)
>>  create mode 100644
>> MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
>>
>> --
>> 2.14.1.3.gb7cf6e02401b
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 10/10/17 06:17, Yao, Jiewen wrote:
> Thanks.
> Please use ASSERT() when you check in.
>
> Series Reviewed-by: Jiewen.yao@intel.com<mailto:Jiewen.yao@intel.com>

Thank you all for the (quick!) feedback; I've pushed the series:
35ac962b5473..fda8f631edbb.

Here's the cumulative diff between the posted (v1) and the pushed
series, based on the comments:

> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h b/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
> index 759e47db7f29..b98b8556a23a 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
> @@ -62,9 +62,7 @@ MorLockInitAtEndOfDxe (
>    @param[in]  VariableName the name of the vendor's variable, as a
>                             Null-Terminated Unicode String
>    @param[in]  VendorGuid   Unify identifier for vendor.
> -  @param[in]  Attributes   Point to memory location to return the attributes of
> -                           variable. If the point is NULL, the parameter would
> -                           be ignored.
> +  @param[in]  Attributes   Attributes bitmask to set for the variable.
>    @param[in]  DataSize     The size in bytes of Data-Buffer.
>    @param[in]  Data         Point to the content of the variable.
>
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
> index a91fc42ff465..7142e2da2073 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
> @@ -31,9 +31,7 @@ extern EDKII_VARIABLE_LOCK_PROTOCOL     mVariableLock;
>    @param[in]  VariableName the name of the vendor's variable, as a
>                             Null-Terminated Unicode String
>    @param[in]  VendorGuid   Unify identifier for vendor.
> -  @param[in]  Attributes   Point to memory location to return the attributes of
> -                           variable. If the point is NULL, the parameter would
> -                           be ignored.
> +  @param[in]  Attributes   Attributes bitmask to set for the variable.
>    @param[in]  DataSize     The size in bytes of Data-Buffer.
>    @param[in]  Data         Point to the content of the variable.
>
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> index 0a0281e44bc1..93a300a84677 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> @@ -319,9 +319,7 @@ SetVariableCheckHandlerMorLock (
>    @param[in]  VariableName the name of the vendor's variable, as a
>                             Null-Terminated Unicode String
>    @param[in]  VendorGuid   Unify identifier for vendor.
> -  @param[in]  Attributes   Point to memory location to return the attributes of
> -                           variable. If the point is NULL, the parameter would
> -                           be ignored.
> +  @param[in]  Attributes   Attributes bitmask to set for the variable.
>    @param[in]  DataSize     The size in bytes of Data-Buffer.
>    @param[in]  Data         Point to the content of the variable.
>
> @@ -427,8 +425,9 @@ MorLockInitAtEndOfDxe (
>    if (!mMorLockInitializationRequired) {
>      //
>      // The EFI_SMM_FAULT_TOLERANT_WRITE_PROTOCOL has never been installed, thus
> -    // the variable write service is unavailable. Do nothing.
> +    // the variable write service is unavailable. This should never happen.
>      //
> +    ASSERT (FALSE);
>      return;
>    }
>

In addition, I modified the commit message of patch #2 (now commit
03877377e326, "MdeModulePkg/Variable/RuntimeDxe: move MOR func.
declarations to header", 2017-09-30), to credit Star for the fixed
"Attributes" param description.

(The ASSERT from Jiewen needed no commit message update; commit
7516532f9c2d ("MdeModulePkg/Variable/RuntimeDxe: delay MorLock creation
until EndOfDxe", 2017-09-30) already carried "Suggested-by: Jiewen Yao
<jiewen.yao@intel.com>".)


I plan to post the separate patch for option (a) soon (see
<http://mid.mail-archive.com/74D8A39837DF1E4DA445A8C0B3885C503A9D71F4@shsmsx102.ccr.corp.intel.com>).

Thank you!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Good to see the series pushed.

Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Tuesday, October 10, 2017 6:09 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com>
Cc: edk2-devel-01 <edk2-devel@lists.01.org>; Dong, Eric <eric.dong@intel.com>; Ladi Prosek <lprosek@redhat.com>
Subject: Re: [edk2] [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency

On 10/10/17 06:17, Yao, Jiewen wrote:
> Thanks.
> Please use ASSERT() when you check in.
>
> Series Reviewed-by: Jiewen.yao@intel.com<mailto:Jiewen.yao@intel.com>

Thank you all for the (quick!) feedback; I've pushed the series:
35ac962b5473..fda8f631edbb.

Here's the cumulative diff between the posted (v1) and the pushed series, based on the comments:

> diff --git 
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h 
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
> index 759e47db7f29..b98b8556a23a 100644
> --- 
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.
> +++ h
> @@ -62,9 +62,7 @@ MorLockInitAtEndOfDxe (
>    @param[in]  VariableName the name of the vendor's variable, as a
>                             Null-Terminated Unicode String
>    @param[in]  VendorGuid   Unify identifier for vendor.
> -  @param[in]  Attributes   Point to memory location to return the attributes of
> -                           variable. If the point is NULL, the parameter would
> -                           be ignored.
> +  @param[in]  Attributes   Attributes bitmask to set for the variable.
>    @param[in]  DataSize     The size in bytes of Data-Buffer.
>    @param[in]  Data         Point to the content of the variable.
>
> diff --git 
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c 
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
> index a91fc42ff465..7142e2da2073 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
> @@ -31,9 +31,7 @@ extern EDKII_VARIABLE_LOCK_PROTOCOL     mVariableLock;
>    @param[in]  VariableName the name of the vendor's variable, as a
>                             Null-Terminated Unicode String
>    @param[in]  VendorGuid   Unify identifier for vendor.
> -  @param[in]  Attributes   Point to memory location to return the attributes of
> -                           variable. If the point is NULL, the parameter would
> -                           be ignored.
> +  @param[in]  Attributes   Attributes bitmask to set for the variable.
>    @param[in]  DataSize     The size in bytes of Data-Buffer.
>    @param[in]  Data         Point to the content of the variable.
>
> diff --git 
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c 
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> index 0a0281e44bc1..93a300a84677 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> @@ -319,9 +319,7 @@ SetVariableCheckHandlerMorLock (
>    @param[in]  VariableName the name of the vendor's variable, as a
>                             Null-Terminated Unicode String
>    @param[in]  VendorGuid   Unify identifier for vendor.
> -  @param[in]  Attributes   Point to memory location to return the attributes of
> -                           variable. If the point is NULL, the parameter would
> -                           be ignored.
> +  @param[in]  Attributes   Attributes bitmask to set for the variable.
>    @param[in]  DataSize     The size in bytes of Data-Buffer.
>    @param[in]  Data         Point to the content of the variable.
>
> @@ -427,8 +425,9 @@ MorLockInitAtEndOfDxe (
>    if (!mMorLockInitializationRequired) {
>      //
>      // The EFI_SMM_FAULT_TOLERANT_WRITE_PROTOCOL has never been installed, thus
> -    // the variable write service is unavailable. Do nothing.
> +    // the variable write service is unavailable. This should never happen.
>      //
> +    ASSERT (FALSE);
>      return;
>    }
>

In addition, I modified the commit message of patch #2 (now commit 03877377e326, "MdeModulePkg/Variable/RuntimeDxe: move MOR func.
declarations to header", 2017-09-30), to credit Star for the fixed "Attributes" param description.

(The ASSERT from Jiewen needed no commit message update; commit 7516532f9c2d ("MdeModulePkg/Variable/RuntimeDxe: delay MorLock creation until EndOfDxe", 2017-09-30) already carried "Suggested-by: Jiewen Yao
<jiewen.yao@intel.com>".)


I plan to post the separate patch for option (a) soon (see <http://mid.mail-archive.com/74D8A39837DF1E4DA445A8C0B3885C503A9D71F4@shsmsx102.ccr.corp.intel.com>).

Thank you!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Laszlo,

If the series is not so urgent to be pushed, I want to take some time(of maybe one or two days) to look at the discussion background and the patches.
If it is urgent, go ahead to push the patches if you have got Jiewen's RB.


Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Wednesday, October 4, 2017 5:28 AM
To: edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Ladi Prosek <lprosek@redhat.com>; Zeng, Star <star.zeng@intel.com>
Subject: [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency

Repo:   https://github.com/lersek/edk2.git
Branch: mor_lock_init_at_end_of_dxe

This patch set fixes the issue reported in the following items:

* Inconsistent MOR control variables exposed by OVMF, breaks Windows
  Device Guard

  https://bugzilla.redhat.com/show_bug.cgi?id=1496170

* VariableSmm MorLockInit(): create MORLock only if / after MOR exists

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

Patches #1 through #3 are cleanups.

Patch #4 is a small helper patch for patch #5.

Patch #5 is the actual fix, following Jiewen's suggestions from the edk2-devel thread

* [edk2] multiple levels of support for MOR / MORLock

  https://lists.01.org/pipermail/edk2-devel/2017-September/015444.html
  https://lists.01.org/pipermail/edk2-devel/2017-October/015530.html

Patch #6 is a workaround for some OSes (minimally Fedora 24-26, and some Debian versions) that create the MOR variable even if the platform doesn't offer it up-front. This patch also follows Jiewen's suggestion from the same edk2-devel thread.

(

BTW, at Paolo's recommendation, I've now reported this kernel issue for Fedora, under

* incorrect downstream-only Platform Reset Attack Mitigation patch in
  the F24-F26 kernels

  https://bugzilla.redhat.com/show_bug.cgi?id=1498159

)

I've checked this set for basic regressions, using OVMF, normal boot and
S3 suspend/resume:

* Q35, SMM, IA32:
  - Fedora 25 -- verified patch #6 specifically

* i440fx, no SMM, X64:
  - Fedora 24

* Q35, SMM, IA32X64:
  - Fedora 26 -- verified patch #6 specifically
  - Windows 7
  - Windows 8.1
  - Windows 10
  - Windows Server 2008 R2
  - Windows Server 2012 R2

I didn't / couldn't test this set in the following two environments:

- on platforms where TcgMor.inf is included in the firmware, and the MOR
  variable exists genuinely,

- in the nested virt setup where Ladi reported the Device Guard
  breakage. (If I understand correctly, ATM this requires additional
  host kernel (KVM) patches.)

Test results / feedback from those envs would be appreciated.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ladi Prosek <lprosek@redhat.com>
Cc: Star Zeng <star.zeng@intel.com>

Thanks,
Laszlo

Laszlo Ersek (6):
  MdeModulePkg/Variable/RuntimeDxe: move SecureBootHook() decl to new
    header
  MdeModulePkg/Variable/RuntimeDxe: move MOR func. declarations to
    header
  MdeModulePkg/Variable/RuntimeDxe: introduce MorLockInitAtEndOfDxe()
    hook
  MdeModulePkg/Variable/RuntimeDxe: permit MorLock deletion for passthru
    req
  MdeModulePkg/Variable/RuntimeDxe: delay MorLock creation until
    EndOfDxe
  MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR
    variable

 MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c             |   2 +
 MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h    |  89 ++++++++++
 MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c           |  45 +++--
 MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c           | 173 ++++++++++++++++++--
 MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c                |  51 ------
 MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h                |   2 +
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c             |   2 +
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf    |   1 +
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c             |   2 +
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf           |   4 +
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c   |  16 +-
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf |   1 +
 12 files changed, 294 insertions(+), 94 deletions(-)  create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h

--
2.14.1.3.gb7cf6e02401b

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Hi Star,

On 10/05/17 09:42, Zeng, Star wrote:
> Laszlo,
> 
> If the series is not so urgent to be pushed, I want to take some time(of maybe one or two days) to look at the discussion background and the patches.
> If it is urgent, go ahead to push the patches if you have got Jiewen's RB.

Jiewen hasn't given his official R-b yet; he said that the patches
looked good to him in general.

Beyond an R-b from Jiewen and/or you and/or Eric, I wouldn't like to
push the patches until Intel QA (or one of you guys) can regression-test
the series, on a platform where TcgMor.inf is included -- that is, on a
platform where the MOR and MorLock variables exist *genuinely*. I don't
have access to such a platform (OVMF does not support these variables),
so I couldn't regression-test the series that way.

The variable driver is very important and it is shipped on all physical
platforms as well, so we shouldn't push these patches before thorough
regression-testing. I'd rather delay committing this set and do a bit
more work in RHEL7 downstream (backports) than have to fix an ugly
upstream regression in a panic.

Please take your time and review the patches and the background
discussion in detail. And, again, I would very much appreciate if you
guys or someone from Intel QA could fetch the branch and regression-test
the work, using a platform that supports MOR and MorLock for real.

Thanks!
Laszlo

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com] 
> Sent: Wednesday, October 4, 2017 5:28 AM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Ladi Prosek <lprosek@redhat.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency
> 
> Repo:   https://github.com/lersek/edk2.git
> Branch: mor_lock_init_at_end_of_dxe
> 
> This patch set fixes the issue reported in the following items:
> 
> * Inconsistent MOR control variables exposed by OVMF, breaks Windows
>   Device Guard
> 
>   https://bugzilla.redhat.com/show_bug.cgi?id=1496170
> 
> * VariableSmm MorLockInit(): create MORLock only if / after MOR exists
> 
>   https://bugzilla.tianocore.org/show_bug.cgi?id=727
> 
> Patches #1 through #3 are cleanups.
> 
> Patch #4 is a small helper patch for patch #5.
> 
> Patch #5 is the actual fix, following Jiewen's suggestions from the edk2-devel thread
> 
> * [edk2] multiple levels of support for MOR / MORLock
> 
>   https://lists.01.org/pipermail/edk2-devel/2017-September/015444.html
>   https://lists.01.org/pipermail/edk2-devel/2017-October/015530.html
> 
> Patch #6 is a workaround for some OSes (minimally Fedora 24-26, and some Debian versions) that create the MOR variable even if the platform doesn't offer it up-front. This patch also follows Jiewen's suggestion from the same edk2-devel thread.
> 
> (
> 
> BTW, at Paolo's recommendation, I've now reported this kernel issue for Fedora, under
> 
> * incorrect downstream-only Platform Reset Attack Mitigation patch in
>   the F24-F26 kernels
> 
>   https://bugzilla.redhat.com/show_bug.cgi?id=1498159
> 
> )
> 
> I've checked this set for basic regressions, using OVMF, normal boot and
> S3 suspend/resume:
> 
> * Q35, SMM, IA32:
>   - Fedora 25 -- verified patch #6 specifically
> 
> * i440fx, no SMM, X64:
>   - Fedora 24
> 
> * Q35, SMM, IA32X64:
>   - Fedora 26 -- verified patch #6 specifically
>   - Windows 7
>   - Windows 8.1
>   - Windows 10
>   - Windows Server 2008 R2
>   - Windows Server 2012 R2
> 
> I didn't / couldn't test this set in the following two environments:
> 
> - on platforms where TcgMor.inf is included in the firmware, and the MOR
>   variable exists genuinely,
> 
> - in the nested virt setup where Ladi reported the Device Guard
>   breakage. (If I understand correctly, ATM this requires additional
>   host kernel (KVM) patches.)
> 
> Test results / feedback from those envs would be appreciated.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Ladi Prosek <lprosek@redhat.com>
> Cc: Star Zeng <star.zeng@intel.com>
> 
> Thanks,
> Laszlo
> 
> Laszlo Ersek (6):
>   MdeModulePkg/Variable/RuntimeDxe: move SecureBootHook() decl to new
>     header
>   MdeModulePkg/Variable/RuntimeDxe: move MOR func. declarations to
>     header
>   MdeModulePkg/Variable/RuntimeDxe: introduce MorLockInitAtEndOfDxe()
>     hook
>   MdeModulePkg/Variable/RuntimeDxe: permit MorLock deletion for passthru
>     req
>   MdeModulePkg/Variable/RuntimeDxe: delay MorLock creation until
>     EndOfDxe
>   MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR
>     variable
> 
>  MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c             |   2 +
>  MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h    |  89 ++++++++++
>  MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c           |  45 +++--
>  MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c           | 173 ++++++++++++++++++--
>  MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c                |  51 ------
>  MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h                |   2 +
>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c             |   2 +
>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf    |   1 +
>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c             |   2 +
>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf           |   4 +
>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c   |  16 +-
>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf |   1 +
>  12 files changed, 294 insertions(+), 94 deletions(-)  create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
> 
> --
> 2.14.1.3.gb7cf6e02401b
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Thank you, Laszlo, for your patient.

Yes, I also hope to have a real platform to validate, before I give RB. :-)

Maybe some time in next week.

thank you!
Yao, Jiewen


> 在 2017年10月5日，下午3:57，Laszlo Ersek <lersek@redhat.com> 写道：
> 
> Hi Star,
> 
>> On 10/05/17 09:42, Zeng, Star wrote:
>> Laszlo,
>> 
>> If the series is not so urgent to be pushed, I want to take some time(of maybe one or two days) to look at the discussion background and the patches.
>> If it is urgent, go ahead to push the patches if you have got Jiewen's RB.
> 
> Jiewen hasn't given his official R-b yet; he said that the patches
> looked good to him in general.
> 
> Beyond an R-b from Jiewen and/or you and/or Eric, I wouldn't like to
> push the patches until Intel QA (or one of you guys) can regression-test
> the series, on a platform where TcgMor.inf is included -- that is, on a
> platform where the MOR and MorLock variables exist *genuinely*. I don't
> have access to such a platform (OVMF does not support these variables),
> so I couldn't regression-test the series that way.
> 
> The variable driver is very important and it is shipped on all physical
> platforms as well, so we shouldn't push these patches before thorough
> regression-testing. I'd rather delay committing this set and do a bit
> more work in RHEL7 downstream (backports) than have to fix an ugly
> upstream regression in a panic.
> 
> Please take your time and review the patches and the background
> discussion in detail. And, again, I would very much appreciate if you
> guys or someone from Intel QA could fetch the branch and regression-test
> the work, using a platform that supports MOR and MorLock for real.
> 
> Thanks!
> Laszlo
> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com] 
>> Sent: Wednesday, October 4, 2017 5:28 AM
>> To: edk2-devel-01 <edk2-devel@lists.01.org>
>> Cc: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Ladi Prosek <lprosek@redhat.com>; Zeng, Star <star.zeng@intel.com>
>> Subject: [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency
>> 
>> Repo:   https://github.com/lersek/edk2.git
>> Branch: mor_lock_init_at_end_of_dxe
>> 
>> This patch set fixes the issue reported in the following items:
>> 
>> * Inconsistent MOR control variables exposed by OVMF, breaks Windows
>>  Device Guard
>> 
>>  https://bugzilla.redhat.com/show_bug.cgi?id=1496170
>> 
>> * VariableSmm MorLockInit(): create MORLock only if / after MOR exists
>> 
>>  https://bugzilla.tianocore.org/show_bug.cgi?id=727
>> 
>> Patches #1 through #3 are cleanups.
>> 
>> Patch #4 is a small helper patch for patch #5.
>> 
>> Patch #5 is the actual fix, following Jiewen's suggestions from the edk2-devel thread
>> 
>> * [edk2] multiple levels of support for MOR / MORLock
>> 
>>  https://lists.01.org/pipermail/edk2-devel/2017-September/015444.html
>>  https://lists.01.org/pipermail/edk2-devel/2017-October/015530.html
>> 
>> Patch #6 is a workaround for some OSes (minimally Fedora 24-26, and some Debian versions) that create the MOR variable even if the platform doesn't offer it up-front. This patch also follows Jiewen's suggestion from the same edk2-devel thread.
>> 
>> (
>> 
>> BTW, at Paolo's recommendation, I've now reported this kernel issue for Fedora, under
>> 
>> * incorrect downstream-only Platform Reset Attack Mitigation patch in
>>  the F24-F26 kernels
>> 
>>  https://bugzilla.redhat.com/show_bug.cgi?id=1498159
>> 
>> )
>> 
>> I've checked this set for basic regressions, using OVMF, normal boot and
>> S3 suspend/resume:
>> 
>> * Q35, SMM, IA32:
>>  - Fedora 25 -- verified patch #6 specifically
>> 
>> * i440fx, no SMM, X64:
>>  - Fedora 24
>> 
>> * Q35, SMM, IA32X64:
>>  - Fedora 26 -- verified patch #6 specifically
>>  - Windows 7
>>  - Windows 8.1
>>  - Windows 10
>>  - Windows Server 2008 R2
>>  - Windows Server 2012 R2
>> 
>> I didn't / couldn't test this set in the following two environments:
>> 
>> - on platforms where TcgMor.inf is included in the firmware, and the MOR
>>  variable exists genuinely,
>> 
>> - in the nested virt setup where Ladi reported the Device Guard
>>  breakage. (If I understand correctly, ATM this requires additional
>>  host kernel (KVM) patches.)
>> 
>> Test results / feedback from those envs would be appreciated.
>> 
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Ladi Prosek <lprosek@redhat.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>> 
>> Thanks,
>> Laszlo
>> 
>> Laszlo Ersek (6):
>>  MdeModulePkg/Variable/RuntimeDxe: move SecureBootHook() decl to new
>>    header
>>  MdeModulePkg/Variable/RuntimeDxe: move MOR func. declarations to
>>    header
>>  MdeModulePkg/Variable/RuntimeDxe: introduce MorLockInitAtEndOfDxe()
>>    hook
>>  MdeModulePkg/Variable/RuntimeDxe: permit MorLock deletion for passthru
>>    req
>>  MdeModulePkg/Variable/RuntimeDxe: delay MorLock creation until
>>    EndOfDxe
>>  MdeModulePkg/Variable/RuntimeDxe: delete and lock OS-created MOR
>>    variable
>> 
>> MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c             |   2 +
>> MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h    |  89 ++++++++++
>> MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c           |  45 +++--
>> MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c           | 173 ++++++++++++++++++--
>> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c                |  51 ------
>> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h                |   2 +
>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c             |   2 +
>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf    |   1 +
>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c             |   2 +
>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf           |   4 +
>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c   |  16 +-
>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf |   1 +
>> 12 files changed, 294 insertions(+), 94 deletions(-)  create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h
>> 
>> --
>> 2.14.1.3.gb7cf6e02401b
>> 
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel