[EXTERNAL] [PATCH v8 00/14] Add the VariablePolicy feature


Bret Barkelew
 

To whom it may concern,

This is as done as it’s going to get.

 

Thank you all for your help!

 

- Bret

 

From: Bret Barkelew
Sent: Tuesday, September 22, 2020 11:08 PM
To: devel@edk2.groups.io
Cc: Yao, Jiewen; Dandan Bi; Chao Zhang; Jian J Wang; Hao A Wu; liming.gao; Jordan Justen; Laszlo Ersek; Ard Biesheuvel; Andrew Fish; Ni, Ray; Bret Barkelew
Subject: [EXTERNAL] [PATCH v8 00/14] Add the VariablePolicy feature

 

The 14 patches in this series add the VariablePolicy feature to the core,
deprecate Edk2VarLock (while adding a compatibility layer to reduce code
churn), and integrate the VariablePolicy libraries and protocols into
Variable Services.

Since the integration requires multiple changes, including adding libraries,
a protocol, an SMI communication handler, and VariableServices integration,
the patches are broken up by individual library additions and then a final
integration. Security-sensitive changes like bypassing Authenticated
Variable enforcement are also broken out into individual patches so that
attention can be called directly to them.

Platform porting instructions are described in this wiki entry:
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FVariablePolicy-Protocol---Enhanced-Method-for-Managing-Variables%23platform-porting&data=02%7C01%7CBret.Barkelew%40microsoft.com%7C1b508888579a496271ae08d85f870cf2%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637364380959491498&sdata=n2SYXG8iUBvDc2bo0%2B%2BB9Ftut46VNtVgdpJSFnX6%2Fu4%3D&reserved=0

Discussion of the feature can be found in multiple places throughout
the last year on the RFC channel, staging branches, and in devel.

Most recently, this subject was discussed in this thread:
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F53712&data=02%7C01%7CBret.Barkelew%40microsoft.com%7C1b508888579a496271ae08d85f870cf2%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637364380959491498&sdata=81ta2Tht%2FwjDjk8LqQ8wRI0I1ggK14fePhWlMxDOCGA%3D&reserved=0
(the code branches shared in that discussion are now out of date, but the
whitepapers and discussion are relevant).

Cc: Jiewen Yao <jiewen.yao@...>
Cc: Dandan Bi <dandan.bi@...>
Cc: Chao Zhang <chao.b.zhang@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Liming Gao <liming.gao@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Ard Biesheuvel <ard.biesheuvel@...>
Cc: Andrew Fish <afish@...>
Cc: Ray Ni <ray.ni@...>
Cc: Bret Barkelew <brbarkel@...>
Signed-off-by: Bret Barkelew <brbarkel@...>

v8 changes:
* Rebase
* Small tweaks from final PRs
* Drank a lot
* Enrolled several members and a steward in CatFacts

v7 changes:
* Address comments from Dandan about security of the MM handler
* Add readme
* Fix bug around hex characters in BOOT####, etc
* Add additional testing for hex characters
* Add additional testing for authenticated variables

v6 changes:
* Fix an issue with uninitialized Status in InitVariablePolicyLib() and DeinitVariablePolicyLib()
* Fix GCC building in shell-based functional test
* Rebase on latest origin/master

v5 changes:
* Fix the CONST mismatch in VariablePolicy.h and VariablePolicySmmDxe.c
* Fix EFIAPI mismatches in the functional unittest
* Rebase on latest origin/master

v4 changes:
* Remove Optional PcdAllowVariablePolicyEnforcementDisable PCD from platforms
* Rebase on master
* Migrate to new MmCommunicate2 protocol
* Fix an oversight in the default return value for InitMmCommonCommBuffer
* Fix in VariablePolicyLib to allow ExtraInitRuntimeDxe to consume variables

V3 changes:
* Address all non-unittest issues with ECC
* Make additional style changes
* Include section name in hunk headers in "ini-style" files
* Remove requirement for the EdkiiPiSmmCommunicationsRegionTable driver
  (now allocates its own buffer)
* Change names from VARIABLE_POLICY_PROTOCOL and gVariablePolicyProtocolGuid
  to EDKII_VARIABLE_POLICY_PROTOCOL and gEdkiiVariablePolicyProtocolGuid
* Fix GCC warning about initializing externs
* Add UNI strings for new PCD
* Add patches for ArmVirtPkg, OvmfXen, and UefiPayloadPkg
* Reorder patches according to Liming's feedback about adding to platforms
  before changing variable driver

V2 changes:
* Fixed implementation for RuntimeDxe
* Add PCD to block DisableVariablePolicy
* Fix the DumpVariablePolicy pagination in SMM


Bret Barkelew (14):
  MdeModulePkg: Define the VariablePolicy protocol interface
  MdeModulePkg: Define the VariablePolicyLib
  MdeModulePkg: Define the VariablePolicyHelperLib
  MdeModulePkg: Define the VarCheckPolicyLib and SMM interface
  OvmfPkg: Add VariablePolicy engine to OvmfPkg platform
  EmulatorPkg: Add VariablePolicy engine to EmulatorPkg platform
  ArmVirtPkg: Add VariablePolicy engine to ArmVirtPkg platform
  UefiPayloadPkg: Add VariablePolicy engine to UefiPayloadPkg platform
  MdeModulePkg: Connect VariablePolicy business logic to
    VariableServices
  MdeModulePkg: Allow VariablePolicy state to delete protected variables
  SecurityPkg: Allow VariablePolicy state to delete authenticated
    variables
  MdeModulePkg: Change TCG MOR variables to use VariablePolicy
  MdeModulePkg: Drop VarLock from RuntimeDxe variable driver
  MdeModulePkg: Add a shell-based functional test for VariablePolicy

 MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.c                               |  345 +++
 MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.c                   |  396 ++++
 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitNull.c                     |   46 +
 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitRuntimeDxe.c               |   85 +
 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c                               |  830 +++++++
 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyUnitTest/VariablePolicyUnitTest.c   | 2452 ++++++++++++++++++++
 MdeModulePkg/Test/ShellTest/VariablePolicyFuncTestApp/VariablePolicyFuncTestApp.c        | 2226 ++++++++++++++++++
 MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c                               |   52 +-
 MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c                               |   60 +-
 MdeModulePkg/Universal/Variable/RuntimeDxe/VarCheck.c                                    |   49 +-
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c                                 |   56 +
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestToLock.c                   |   71 +
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c                        |  642 +++++
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c                       |   14 +
 SecurityPkg/Library/AuthVariableLib/AuthService.c                                        |   30 +-
 ArmVirtPkg/ArmVirt.dsc.inc                                                               |    4 +
 EmulatorPkg/EmulatorPkg.dsc                                                              |    3 +
 MdeModulePkg/Include/Guid/VarCheckPolicyMmi.h                                            |   54 +
 MdeModulePkg/Include/Library/VariablePolicyHelperLib.h                                   |  164 ++
 MdeModulePkg/Include/Library/VariablePolicyLib.h                                         |  207 ++
 MdeModulePkg/Include/Protocol/VariablePolicy.h                                           |  157 ++
 MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.inf                             |   42 +
 MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.uni                             |   12 +
 MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.inf                 |   35 +
 MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.uni                 |   12 +
 MdeModulePkg/Library/VariablePolicyLib/ReadMe.md                                         |  410 ++++
 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.inf                             |   49 +
 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.uni                             |   12 +
 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLibRuntimeDxe.inf                   |   51 +
 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyUnitTest/VariablePolicyUnitTest.inf |   45 +
 MdeModulePkg/MdeModulePkg.ci.yaml                                                        |    8 +-
 MdeModulePkg/MdeModulePkg.dec                                                            |   26 +-
 MdeModulePkg/MdeModulePkg.dsc                                                            |    9 +
 MdeModulePkg/MdeModulePkg.uni                                                            |    7 +
 MdeModulePkg/Test/MdeModulePkgHostTest.dsc                                               |   11 +
 MdeModulePkg/Test/ShellTest/VariablePolicyFuncTestApp/Readme.md                          |   55 +
 MdeModulePkg/Test/ShellTest/VariablePolicyFuncTestApp/VariablePolicyFuncTestApp.inf      |   47 +
 MdeModulePkg/Test/ShellTest/VariablePolicyFuncTestApp/VariablePolicyTestAuthVar.h        |  128 +
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf                        |    5 +
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf                               |    4 +
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf                     |   11 +
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf                      |    4 +
 OvmfPkg/OvmfPkgIa32.dsc                                                                  |    5 +
 OvmfPkg/OvmfPkgIa32X64.dsc                                                               |    5 +
 OvmfPkg/OvmfPkgX64.dsc                                                                   |    5 +
 OvmfPkg/OvmfXen.dsc                                                                      |    4 +
 SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf                                  |    2 +
 UefiPayloadPkg/UefiPayloadPkgIa32.dsc                                                    |    4 +
 UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc                                                 |    4 +
 49 files changed, 8874 insertions(+), 81 deletions(-)
 create mode 100644 MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.c
 create mode 100644 MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.c
 create mode 100644 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitNull.c
 create mode 100644 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitRuntimeDxe.c
 create mode 100644 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c
 create mode 100644 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyUnitTest/VariablePolicyUnitTest.c
 create mode 100644 MdeModulePkg/Test/ShellTest/VariablePolicyFuncTestApp/VariablePolicyFuncTestApp.c
 create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestToLock.c
 create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c
 create mode 100644 MdeModulePkg/Include/Guid/VarCheckPolicyMmi.h
 create mode 100644 MdeModulePkg/Include/Library/VariablePolicyHelperLib.h
 create mode 100644 MdeModulePkg/Include/Library/VariablePolicyLib.h
 create mode 100644 MdeModulePkg/Include/Protocol/VariablePolicy.h
 create mode 100644 MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.inf
 create mode 100644 MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.uni
 create mode 100644 MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.inf
 create mode 100644 MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.uni
 create mode 100644 MdeModulePkg/Library/VariablePolicyLib/ReadMe.md
 create mode 100644 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.inf
 create mode 100644 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.uni
 create mode 100644 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLibRuntimeDxe.inf
 create mode 100644 MdeModulePkg/Library/VariablePolicyLib/VariablePolicyUnitTest/VariablePolicyUnitTest.inf
 create mode 100644 MdeModulePkg/Test/ShellTest/VariablePolicyFuncTestApp/Readme.md
 create mode 100644 MdeModulePkg/Test/ShellTest/VariablePolicyFuncTestApp/VariablePolicyFuncTestApp.inf
 create mode 100644 MdeModulePkg/Test/ShellTest/VariablePolicyFuncTestApp/VariablePolicyTestAuthVar.h

--
2.28.0.windows.1

 


Laszlo Ersek
 

On 09/23/20 08:12, Bret Barkelew via groups.io wrote:
To whom it may concern,
This is as done as it’s going to get.

Thank you all for your help!
Seems like it's been fully reviewed. If that's the case -- do you want
me to merge it?

(Asking because the series modifies multiple packages, so there isn't a
maintainer that's uniquely responsible for merging the series.)

Thanks,
Laszlo


Ard Biesheuvel
 

On 9/23/20 10:45 AM, Laszlo Ersek wrote:
On 09/23/20 08:12, Bret Barkelew via groups.io wrote:
To whom it may concern,
This is as done as it’s going to get.

Thank you all for your help!
Seems like it's been fully reviewed. If that's the case -- do you want
me to merge it?
(Asking because the series modifies multiple packages, so there isn't a
maintainer that's uniquely responsible for merging the series.)
Yes, please. This has been going on long enough.

Only question I have is breakage in edk2-platforms - it seems that most platforms there are broken atm anyway due to the RngLib change having been merged, but it would be good to have an idea what the status is there.


Laszlo Ersek
 

On 09/23/20 11:22, Ard Biesheuvel wrote:
On 9/23/20 10:45 AM, Laszlo Ersek wrote:
On 09/23/20 08:12, Bret Barkelew via groups.io wrote:
To whom it may concern,
This is as done as it’s going to get.

Thank you all for your help!
Seems like it's been fully reviewed. If that's the case -- do you want
me to merge it?

(Asking because the series modifies multiple packages, so there isn't a
maintainer that's uniquely responsible for merging the series.)
Yes, please. This has been going on long enough.

Only question I have is breakage in edk2-platforms - it seems that most
platforms there are broken atm anyway due to the RngLib change having
been merged, but it would be good to have an idea what the status is there.
Judged from patches 05 through 08, the platforms in edk2-platforms are
going to need some new lib class resolutions. Therefore I think we might
have to merge this in two parts:

- patches 01-08 in the first go,
- [update edk2-platforms to mimick patches 05-08],
- patches 09-14 in the second round.

If Bret is OK with that, I can start merging 01-08 soon.

(In theory, we could merge patches 05-08 as a part of the second round,
because technically, edk2-platforms only need 01-04. However, if some
commit messages in edk2-platforms would like to reference *example
platform code* from edk2, then having stable commit hashes for patches
05-08 in edk2 would be useful. Hence my suggestion to include 05-08 in
the first round of edk2 merging.)

Thanks,
Laszlo


Laszlo Ersek
 

On 09/23/20 11:43, Laszlo Ersek wrote:
On 09/23/20 11:22, Ard Biesheuvel wrote:
On 9/23/20 10:45 AM, Laszlo Ersek wrote:
On 09/23/20 08:12, Bret Barkelew via groups.io wrote:
To whom it may concern,
This is as done as it’s going to get.

Thank you all for your help!
Seems like it's been fully reviewed. If that's the case -- do you want
me to merge it?

(Asking because the series modifies multiple packages, so there isn't a
maintainer that's uniquely responsible for merging the series.)
Yes, please. This has been going on long enough.

Only question I have is breakage in edk2-platforms - it seems that most
platforms there are broken atm anyway due to the RngLib change having
been merged, but it would be good to have an idea what the status is there.
Judged from patches 05 through 08, the platforms in edk2-platforms are
going to need some new lib class resolutions. Therefore I think we might
have to merge this in two parts:

- patches 01-08 in the first go,
- [update edk2-platforms to mimick patches 05-08],
- patches 09-14 in the second round.

If Bret is OK with that, I can start merging 01-08 soon.

(In theory, we could merge patches 05-08 as a part of the second round,
because technically, edk2-platforms only need 01-04. However, if some
commit messages in edk2-platforms would like to reference *example
platform code* from edk2, then having stable commit hashes for patches
05-08 in edk2 would be useful. Hence my suggestion to include 05-08 in
the first round of edk2 merging.)
... on a second thought, we could merge this series in a single PR as
well; only edk2-platforms would have to advance its edk2 submodule
reference in two stages:

- first advance the submodule to patch#8,
- then update its own platform DSC files with the new lib instances,
- then advance the edk2 submodule again, to patch#14.

If that works for you, I think we should merge this edk2 set in one go
-- less work for me, and much more intuitive when viewed from the edk2
side. (The series would not be stuck in some half-merged state for any
time at all.)

Thanks
Laszlo


Ard Biesheuvel
 

On 9/23/20 12:04 PM, Laszlo Ersek wrote:
On 09/23/20 11:43, Laszlo Ersek wrote:
On 09/23/20 11:22, Ard Biesheuvel wrote:
On 9/23/20 10:45 AM, Laszlo Ersek wrote:
On 09/23/20 08:12, Bret Barkelew via groups.io wrote:
To whom it may concern,
This is as done as it’s going to get.

Thank you all for your help!
Seems like it's been fully reviewed. If that's the case -- do you want
me to merge it?

(Asking because the series modifies multiple packages, so there isn't a
maintainer that's uniquely responsible for merging the series.)
Yes, please. This has been going on long enough.

Only question I have is breakage in edk2-platforms - it seems that most
platforms there are broken atm anyway due to the RngLib change having
been merged, but it would be good to have an idea what the status is there.
Judged from patches 05 through 08, the platforms in edk2-platforms are
going to need some new lib class resolutions. Therefore I think we might
have to merge this in two parts:

- patches 01-08 in the first go,
- [update edk2-platforms to mimick patches 05-08],
- patches 09-14 in the second round.

If Bret is OK with that, I can start merging 01-08 soon.

(In theory, we could merge patches 05-08 as a part of the second round,
because technically, edk2-platforms only need 01-04. However, if some
commit messages in edk2-platforms would like to reference *example
platform code* from edk2, then having stable commit hashes for patches
05-08 in edk2 would be useful. Hence my suggestion to include 05-08 in
the first round of edk2 merging.)
... on a second thought, we could merge this series in a single PR as
well; only edk2-platforms would have to advance its edk2 submodule
reference in two stages:
- first advance the submodule to patch#8,
- then update its own platform DSC files with the new lib instances,
- then advance the edk2 submodule again, to patch#14.
If that works for you, I think we should merge this edk2 set in one go
-- less work for me, and much more intuitive when viewed from the edk2
side. (The series would not be stuck in some half-merged state for any
time at all.)
We don't actually use git submodules there, so this does not work.

But I am fine with just merging this, as edk2-platforms has been reported to be in broken state anyway.


Laszlo Ersek
 

Hi Bret,

On 09/23/20 08:12, Bret Barkelew via groups.io wrote:
To whom it may concern,
This is as done as it’s going to get.

Thank you all for your help!
I tried to merge this via <https://github.com/tianocore/edk2/pull/954>,
but it failed. Please review the build report, and submit v9 if necessary.

Thanks!
Laszlo


Sean
 

Ard/Laszlo/Samer (since you reported issue with RngLib),

Is now the time to start a discussion about using submodules or at least tracked dependencies in edk2-platforms. The way it stands the only thing you can use to correlate the two are the timestamp of the commit. This means that the history of edk2-platforms will not actually be any different if you merge into edk2 as a two part series or one.

This has been a major pain point as someone who wants to use edk2-platforms downstream.

Another benefit of tracked dependencies is that each platform or group of commonly managed platforms would move their dependencies as they incorporate any edk2 change. This also gives visibility into the maintenance and status of a platform.

Thanks
Sean

On 9/23/2020 3:17 AM, Ard Biesheuvel wrote:
On 9/23/20 12:04 PM, Laszlo Ersek wrote:
On 09/23/20 11:43, Laszlo Ersek wrote:
On 09/23/20 11:22, Ard Biesheuvel wrote:
On 9/23/20 10:45 AM, Laszlo Ersek wrote:
On 09/23/20 08:12, Bret Barkelew via groups.io wrote:
To whom it may concern,
This is as done as it’s going to get.

Thank you all for your help!
Seems like it's been fully reviewed. If that's the case -- do you want
me to merge it?

(Asking because the series modifies multiple packages, so there isn't a
maintainer that's uniquely responsible for merging the series.)
Yes, please. This has been going on long enough.

Only question I have is breakage in edk2-platforms - it seems that most
platforms there are broken atm anyway due to the RngLib change having
been merged, but it would be good to have an idea what the status is there.
Judged from patches 05 through 08, the platforms in edk2-platforms are
going to need some new lib class resolutions. Therefore I think we might
have to merge this in two parts:

- patches 01-08 in the first go,
- [update edk2-platforms to mimick patches 05-08],
- patches 09-14 in the second round.

If Bret is OK with that, I can start merging 01-08 soon.

(In theory, we could merge patches 05-08 as a part of the second round,
because technically, edk2-platforms only need 01-04. However, if some
commit messages in edk2-platforms would like to reference *example
platform code* from edk2, then having stable commit hashes for patches
05-08 in edk2 would be useful. Hence my suggestion to include 05-08 in
the first round of edk2 merging.)
... on a second thought, we could merge this series in a single PR as
well; only edk2-platforms would have to advance its edk2 submodule
reference in two stages:

- first advance the submodule to patch#8,
- then update its own platform DSC files with the new lib instances,
- then advance the edk2 submodule again, to patch#14.

If that works for you, I think we should merge this edk2 set in one go
-- less work for me, and much more intuitive when viewed from the edk2
side. (The series would not be stuck in some half-merged state for any
time at all.)
We don't actually use git submodules there, so this does not work.
But I am fine with just merging this, as edk2-platforms has been reported to be in broken state anyway.


Bret Barkelew
 

As far as I can tell, this is exposing a pre-existing race condition in EndOfDxe. It just so HAPPENS that the old "Disable RequestToLock" callback executed after this TcgMor callback, whereas the new VariablePolicy callback executes first.

I'm going to poke around for a solution, but this seems like an architectural problem with EndOfDxe.

On Wed, Sep 23, 2020 at 6:02 AM Laszlo Ersek <lersek@...> wrote:
Hi Bret,

On 09/23/20 08:12, Bret Barkelew via groups.io wrote:
> To whom it may concern,
> This is as done as it’s going to get.
>
> Thank you all for your help!

I tried to merge this via <https://github.com/tianocore/edk2/pull/954>,
but it failed. Please review the build report, and submit v9 if necessary.

Thanks!
Laszlo


Andrew Fish
 



On Sep 23, 2020, at 2:14 PM, Bret Barkelew <bret@...> wrote:

As far as I can tell, this is exposing a pre-existing race condition in EndOfDxe. It just so HAPPENS that the old "Disable RequestToLock" callback executed after this TcgMor callback, whereas the new VariablePolicy callback executes first.

I'm going to poke around for a solution, but this seems like an architectural problem with EndOfDxe.


The architecture does not guarantee order for the events. So if the events are dependent on the order of other events that is a bug in implementation. These kind of bugs are hard to notice as the DXE Core implementation event dispatch in a fixed order, I think in the order the event was registered. So it looks correct until you add more events. 

Thanks,

Andrew Fish

On Wed, Sep 23, 2020 at 6:02 AM Laszlo Ersek <lersek@...> wrote:
Hi Bret,

On 09/23/20 08:12, Bret Barkelew via groups.io wrote:
> To whom it may concern,
> This is as done as it’s going to get.
>
> Thank you all for your help!

I tried to merge this via <https://github.com/tianocore/edk2/pull/954>,
but it failed. Please review the build report, and submit v9 if necessary.

Thanks!
Laszlo



Bret Barkelew
 

Agreed. It's random that the previous config worked.


On Wed, Sep 23, 2020 at 2:32 PM Andrew Fish <afish@...> wrote:


On Sep 23, 2020, at 2:14 PM, Bret Barkelew <bret@...> wrote:

As far as I can tell, this is exposing a pre-existing race condition in EndOfDxe. It just so HAPPENS that the old "Disable RequestToLock" callback executed after this TcgMor callback, whereas the new VariablePolicy callback executes first.

I'm going to poke around for a solution, but this seems like an architectural problem with EndOfDxe.


The architecture does not guarantee order for the events. So if the events are dependent on the order of other events that is a bug in implementation. These kind of bugs are hard to notice as the DXE Core implementation event dispatch in a fixed order, I think in the order the event was registered. So it looks correct until you add more events. 

Thanks,

Andrew Fish

On Wed, Sep 23, 2020 at 6:02 AM Laszlo Ersek <lersek@...> wrote:
Hi Bret,

On 09/23/20 08:12, Bret Barkelew via groups.io wrote:
> To whom it may concern,
> This is as done as it’s going to get.
>
> Thank you all for your help!

I tried to merge this via <https://github.com/tianocore/edk2/pull/954>,
but it failed. Please review the build report, and submit v9 if necessary.

Thanks!
Laszlo



Laszlo Ersek
 

On 09/23/20 23:34, Bret Barkelew wrote:
Agreed. It's random that the previous config worked.
Here's an idea.

Assume we have two DXE drivers (D1 and D2), each of which registers an
EndOfDxe notification function (each to be queued at TPL_CALLBACK or
TPL_NOTIFY, doesn't matter).

Assume D1 does something in its EndOfDxe handler that breaks if D2's
EndOfDxe handler runs first. Let's call the offending action in D2's
EndOfDxe handler "Nastiness". (Scientific term.)

Further assume that D1 is an optional driver (while D2 is mandatory). D1
may or may not be included in the platform firmware.

Here's what the drivers could do, for ensuring that D1's handler runs
first, *if* D1 is included in the platform firmate:

(1) D2 defines (standardizes) a new, well-known protocol GUID. There is
no need for a protocol structure; it can be a null protocol.

If the protocol is present in the protocol database, that means that
Nastiness must not be performed by D2 in its EndOfDxe handler. For
simplicity, let's call the protocol EDKII_NO_NASTINESS_PLEASE_PROTOCOL.

(2) In its entry point function, D1 produces an instance of
EDKII_NO_NASTINESS_PLEASE_PROTOCOL. The protocol can be the sole
protocol on a new handle, or exist on another long-term handle (such as
gImageHandle).

(3) In its EndOfDxe handler, D1 uninstalls
EDKII_NO_NASTINESS_PLEASE_PROTOCOL (potentially destroying the handle as
well).

(This is safe because the handler is running at TPL_NOTIFY at the
highest, and uninstalling protocols is safe at <= TPL_NOTIFY. Memory
allocation services are also explicitly safe at <= TPL_NOTIFY.)

(4) D2 factors Nastiness out of its EndOfDxe handler to a new function.
This new function -- solely consisting of Nastiness -- has the usual
notification function prototype. Let's call it NastyFun().

(5) In its entry point function, D2 creates a new (private, not GUID-ed)
event, for which NastyFun() is the notification function. The TPL is
TPL_CALLBACK. Let's call the event NastyEvent.

(6) In D2's EndOfDxe handler, the original, open-coded Nastiness is
replaced with the following logic:

- If EDKII_NO_NASTINESS_PLEASE_PROTOCOL exists in the protocol database
(according to gBS->LocateProtocol(), then D2's EndOfDxe handler signals
NastyEvent.

(This is safe because LocateProtocol() is safe to call at up to and
including TPL_NOTIFY, and gBS->SignalEvent is safe even at TPL_HIGH_LEVEL.)

- Otherwise, D2's EndOfDxe calls NastyFun() with a normal function call.


It's supposed to work like this:

- If D1 is not in the firmware, it won't install
EDKII_NO_NASTINESS_PLEASE_PROTOCOL, so D2 performs Nastiness (via a
direct call to NastyFun()) on the call stack of its original EndOfDxe
handler. That is, no change in behavior.

- If D1's EndOfDxe handler completes first, D2's EndOfDxe handler will
again not see EDKII_NO_NASTINESS_PLEASE_PROTOCOL; so goto previous point.

- Multiple drivers similar to D1 may coexist; multiple
EDKII_NO_NASTINESS_PLEASE_PROTOCOL instances may coexist (all NULL
interfaces, but on different handles). If there is at least one,
gBS->LocateProtocol() in D2's EndOfDxe handler will find the first one,
and postpone Nastiness.

- When D2's EndOfDxe handler signals NastyEvent, NastyFun() will be
enqueued at TPL_CALLBACK. Note that, given that D2's EndOfDxe handler is
running at the moment, the EndOfDxe event group has been signaled
previously. This means *all* events in the EndOfDxe event group have
been signaled, and all their notification functions have been enqueued
too (in some unspecified order, except for the specified dispatch
ordering between TPL_NOTIFY and TPL_CALLBACK).

So when NastyEvent is signaled, NastyFun() is enqueued *after* all the
other -- already enqueued -- EndOfDxe notification functions. That's
because (a) NastyEvent uses TPL_CALLBACK, so the already pending
TPL_NOTIFY notifications will be dispatched before it of course, and (b)
regarding notify functions enqueued previously at the same TPL (=
TPL_CALLBACK), functions in any given TPL queue are queued / dispatched
in FIFO order.

By the time NastyFun() runs, it could ASSERT() that
EDKII_NO_NASTINESS_PLEASE_PROTOCOL does not exist.

Thanks
Laszlo

On Wed, Sep 23, 2020 at 2:32 PM Andrew Fish <afish@apple.com> wrote:



On Sep 23, 2020, at 2:14 PM, Bret Barkelew <bret@corthon.com> wrote:

As far as I can tell, this is exposing a pre-existing race condition in
EndOfDxe. It just so HAPPENS that the old "Disable RequestToLock"
callback executed after this TcgMor callback, whereas the new
VariablePolicy callback executes first.

I'm going to poke around for a solution, but this seems like an
architectural problem with EndOfDxe.


The architecture does not guarantee order for the events. So if the events
are dependent on the order of other events that is a bug in implementation.
These kind of bugs are hard to notice as the DXE Core implementation event
dispatch in a fixed order, I think in the order the event was registered.
So it looks correct until you add more events.

Thanks,

Andrew Fish

On Wed, Sep 23, 2020 at 6:02 AM Laszlo Ersek <lersek@redhat.com> wrote:

Hi Bret,

On 09/23/20 08:12, Bret Barkelew via groups.io wrote:
To whom it may concern,
This is as done as it’s going to get.

Thank you all for your help!
I tried to merge this via <https://github.com/tianocore/edk2/pull/954>,
but it failed. Please review the build report, and submit v9 if necessary.

Thanks!
Laszlo