Re: [PATCH 0/6] MdeModulePkg/VariableSmm: fix MOR / MorLock inconsistency

[Ladi Prosek <lprosek@redhat.com>]

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