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

["Yao, Jiewen" <jiewen.yao@intel.com>]

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
>