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
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
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
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
© 2016 - 2024 Red Hat, Inc.