Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[202405][Rebase&&FF] Variable Policy Changes #978

Conversation

Flickdm
Copy link
Member

@Flickdm Flickdm commented Jun 26, 2024

Preface

[!NOTE] Depends on #961

Description

Cherry Picks:

  1. 1a63f67 - Picked
  2. aeb50c2 - Picked
  3. 521a898 - Picked
  4. 43530fa - Dropped
  5. 5b46552 - Picked
  6. 7a8dba5 - Picked
  7. dac9330 - Picked
  8. 0549e53 - Picked & splited
  9. 28ec229 - picked
  10. 68670a0 - picked
  11. a8c4697 - picked
  12. 28789f8 - picked
  13. 1fc9ff0 - picked
  14. 8c63697 - picked
  15. 383ec13 - picked

This pull request primarily focuses on modifying the UEFI boot manager library and the variable policy library to improve variable policy enforcement and boot option handling. The most significant changes include the addition of new load option types, the implementation of variable policy locking, and the modification of several methods to ensure better policy enforcement and error handling.

Changes to UEFI Boot Manager Library:

  1. MdeModulePkg/Include/Library/UefiBootManagerLib.h: Expanded the Create method to handle additional load option types like SysPrep#### and PlatformRecovery####. Also, added a lock now variable policy for the PlatformRecovery#### variable if the SetVariable call was successful. [1] [2]
  2. MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c: Added a constructor for UefiBootManagerLib that locates the VariablePolicy protocol and registers a basic variable policy.
  3. MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c: Modified the BmGetFreeOptionNumber method to register a lock now variable policy for the PlatformRecovery#### variable if the SetVariable call was successful. Also, moved RegisterBasicVariablePolicy after SetVariable for OptionName.
  4. MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h: Included Protocol/VariablePolicy.h and Library/VariablePolicyHelperLib.h for variable policy enforcement. [1] [2]

Changes to Variable Policy Library:

  1. MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.c: Fixed character comparison in IsValidVariablePolicyStructure and EvaluatePolicyMatch methods. Also, added a security lock report event when the variable policy locks. [1] [2] [3]
  2. MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.h, MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLibStandaloneMm.c, MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLibTraditional.c: Added a wrapper function to validate the communicate buffer. [1] [2] [3]
  3. MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c: Reported a security lock audit event when the variable policy locks.
  4. MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.inf, MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLibRuntimeDxe.inf: Added SecurityLockAuditLib to report when the variable policy locks. [1] [2]

Additions and Modifications:

  1. MdeModulePkg/Include/Library/VariablePolicyLockingExLib.h: A new file was added that contains definitions necessary for a platform to register a pre- and post-lock callback on the VariablePolicy interface.
  2. MdeModulePkg/Library/VariablePolicyLib/VariablePolicyUnitTest/VariablePolicyUnitTest.inf: A new unit test file was added for business logic for Variable Policy enforcement.
  3. MdeModulePkg/MdeModulePkg.dec: Added VariablePolicyLockingExLib library class and modified PcdAllowVariablePolicyEnforcementDisable PCD to allow policy to be disabled by default. [1] [2]
  4. MdeModulePkg/MdeModulePkg.dsc: Included MemoryProtectionHobLib and added SecurityLockAuditLib to report when the variable policy locks.

For each item, place an "x" in between [ and ] if true. Example: [x].
(you can also check items in the GitHub UI)

  • Impacts functionality?
    • Functionality - Does the change ultimately impact how firmware functions?
    • Examples: Add a new library, publish a new PPI, update an algorithm, ...
  • Impacts security?
    • Security - Does the change have a direct security impact on an application,
      flow, or firmware?
    • Examples: Crypto algorithm change, buffer overflow fix, parameter
      validation improvement, ...
  • Breaking change?
    • Breaking change - Will anyone consuming this change experience a break
      in build or boot behavior?
    • Examples: Add a new library class, move a module to a different repo, call
      a function in a new library class in a pre-existing module, ...
  • Includes tests?
    • Tests - Does the change include any explicit test code?
    • Examples: Unit tests, integration tests, robot tests, ...
  • Includes documentation?
    • Documentation - Does the change contain explicit documentation additions
      outside direct code modifications (and comments)?
    • Examples: Update readme file, add feature readme file, link to documentation
      on an a separate Web page, ...

How This Was Tested

Release/20311

Integration Instructions

N/A

@github-actions github-actions bot added the impact:non-functional Does not have a functional impact label Jun 26, 2024
@Flickdm Flickdm force-pushed the cherry-pick/release/202405/feature/variable-policy branch from e3a4abc to cc33e42 Compare July 3, 2024 22:57
@codecov-commenter
Copy link

codecov-commenter commented Jul 4, 2024

Codecov Report

Attention: Patch coverage is 3.63636% with 106 lines in your changes missing coverage. Please review.

Please upload report for BASE (release/202405@1a3fe3e). Learn more about missing BASE report.

Files Patch % Lines
.../Variable/RuntimeDxe/VariablePolicyLockingCommon.c 0.00% 68 Missing ⚠️
...odulePkg/Library/UefiBootManagerLib/BmLoadOption.c 0.00% 27 Missing ⚠️
...rsal/MonotonicCounterRuntimeDxe/MonotonicCounter.c 0.00% 4 Missing ⚠️
...ModulePkg/Universal/Variable/RuntimeDxe/Variable.c 25.00% 3 Missing ⚠️
...ulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c 0.00% 2 Missing ⚠️
...ePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.c 0.00% 1 Missing ⚠️
...iversal/Variable/RuntimeDxe/VariablePolicySmmDxe.c 0.00% 1 Missing ⚠️
Additional details and impacted files
@@                Coverage Diff                @@
##             release/202405     #978   +/-   ##
=================================================
  Coverage                  ?    0.98%           
=================================================
  Files                     ?      940           
  Lines                     ?   316802           
  Branches                  ?     3630           
=================================================
  Hits                      ?     3128           
  Misses                    ?   313596           
  Partials                  ?       78           
Flag Coverage Δ
MdeModulePkg 0.68% <3.63%> (?)
NetworkPkg 0.55% <ø> (?)
UefiCpuPkg 4.75% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Flickdm Flickdm force-pushed the cherry-pick/release/202405/feature/variable-policy branch 11 times, most recently from 82f8ca0 to ba12a1c Compare July 18, 2024 17:23
@Flickdm Flickdm force-pushed the cherry-pick/release/202405/feature/variable-policy branch 5 times, most recently from 768d8ad to d576ba9 Compare July 19, 2024 21:11
@Flickdm Flickdm marked this pull request as ready for review July 19, 2024 21:16
ShellPkg/Application/Shell/ConsoleLogger.c Outdated Show resolved Hide resolved
ShellPkg/ShellPkg.dsc Outdated Show resolved Hide resolved
@Flickdm Flickdm force-pushed the cherry-pick/release/202405/feature/variable-policy branch 5 times, most recently from bf8561a to 66a4684 Compare July 22, 2024 21:53
@Flickdm Flickdm enabled auto-merge (rebase) July 22, 2024 22:32
@Flickdm Flickdm force-pushed the cherry-pick/release/202405/feature/variable-policy branch from 66a4684 to daf3342 Compare July 22, 2024 22:40
@Flickdm Flickdm force-pushed the cherry-pick/release/202405/feature/variable-policy branch from daf3342 to a5de7ad Compare July 23, 2024 20:15
Bret Barkelew and others added 14 commits July 23, 2024 13:18
When VariablePolicyFuncTestApp ends, the variable policy engine has been
disabled. To accommodate automated testing, reboot the system at the end
of the test suite to reset the variable policy state. The app already
reboots several times during execution to reset the variable policy
enablement/lock state.

- [x] Impacts functionality?
- **Functionality** - Does the change ultimately impact how firmware
functions?
- Examples: Add a new library, publish a new PPI, update an algorithm,
...
- [ ] Impacts security?
- **Security** - Does the change have a direct security impact on an
application,
    flow, or firmware?
  - Examples: Crypto algorithm change, buffer overflow fix, parameter
    validation improvement, ...
- [ ] Breaking change?
- **Breaking change** - Will anyone consuming this change experience a
break
    in build or boot behavior?
- Examples: Add a new library class, move a module to a different repo,
call
    a function in a new library class in a pre-existing module, ...
- [ ] Includes tests?
  - **Tests** - Does the change include any explicit test code?
  - Examples: Unit tests, integration tests, robot tests, ...
- [ ] Includes documentation?
- **Documentation** - Does the change contain explicit documentation
additions
    outside direct code modifications (and comments)?
- Examples: Update readme file, add feature readme file, link to
documentation
    on an a separate Web page, ...

Tested on Q35

N/A
Default to allow Engine disablement. Platform can override in DSC.
Lock interface at ReadyToBoot. Currently -- in Mu -- ReadyToBoot is our TCB, not EndOfDxe.
Don't notify ExitBootServices until after Notify and Callback.
…nterface

Define a NULL library class that can OPTIONALLY be linked against
the DXE VariableServices component to provide platform hooks before and
after VariablePolicy is locked.
This code also dispatches to the callback, if registered.
This change uses newly defined interface to validate communicate buffer.

The new interface is abstracted to Standalone and traditional MM
implementations, respectively to keep the invocation of this interface
the same.
Update Variable Services to allow simple deletion of
auth vars when VarPolicy is disabled.
Adds unit tests to test the following two new APIs introduced:
  1. GetVariablePolicyInfo()
  2. GetLockOnVariableStateVariablePolicyInfo()
...if SetVariable Failed

A variable policy is automatically applied when
EfiBootManagerLoadOptionToVariable() is used to set the
PlatformRecovery#### variable. If the attempt to set the
PlatformRecovery#### fails, we should not register a variable policy.

- [x] Impacts functionality?
- **Functionality** - Does the change ultimately impact how firmware
functions?
- Examples: Add a new library, publish a new PPI, update an algorithm,
...
- [x] Impacts security?
- **Security** - Does the change have a direct security impact on an
application,
    flow, or firmware?
  - Examples: Crypto algorithm change, buffer overflow fix, parameter
    validation improvement, ...
- [ ] Breaking change?
- **Breaking change** - Will anyone consuming this change experience a
break
    in build or boot behavior?
- Examples: Add a new library class, move a module to a different repo,
call
    a function in a new library class in a pre-existing module, ...
- [ ] Includes tests?
  - **Tests** - Does the change include any explicit test code?
  - Examples: Unit tests, integration tests, robot tests, ...
- [ ] Includes documentation?
- **Documentation** - Does the change contain explicit documentation
additions
    outside direct code modifications (and comments)?
- Examples: Update readme file, add feature readme file, link to
documentation
    on an a separate Web page, ...

Tested using the BootAuditTestApp on Q35

N/A
Makes the `#` character used for comparison against wildcard
characters in `CHAR16` strings to be prefixed with `L` so the
character is treated as a wide character constant.

- [ ] Impacts functionality?
- **Functionality** - Does the change ultimately impact how firmware
functions?
- Examples: Add a new library, publish a new PPI, update an algorithm,
...
- [ ] Impacts security?
- **Security** - Does the change have a direct security impact on an
application,
    flow, or firmware?
  - Examples: Crypto algorithm change, buffer overflow fix, parameter
    validation improvement, ...
- [ ] Breaking change?
- **Breaking change** - Will anyone consuming this change experience a
break
    in build or boot behavior?
- Examples: Add a new library class, move a module to a different repo,
call
    a function in a new library class in a pre-existing module, ...
- [x] Includes tests?
  - **Tests** - Does the change include any explicit test code?
  - Examples: Unit tests, integration tests, robot tests, ...
- [ ] Includes documentation?
- **Documentation** - Does the change contain explicit documentation
additions
    outside direct code modifications (and comments)?
- Examples: Update readme file, add feature readme file, link to
documentation
    on an a separate Web page, ...

Verified using C/C++ debugger that the character comparison is tested
and the expected result is returned.

N/A

Signed-off-by: Michael Kubacki <[email protected]>
To enable testing variable policy functionality in the shell, it needs
to be unlocked when the test starts. This PR adds a check to the system
device state and avoids locking if it is in unit test mode.

- [x] Impacts functionality?
- **Functionality** - Does the change ultimately impact how firmware
functions?
- Examples: Add a new library, publish a new PPI, update an algorithm,
...
- [x] Impacts security?
- **Security** - Does the change have a direct security impact on an
application,
    flow, or firmware?
  - Examples: Crypto algorithm change, buffer overflow fix, parameter
    validation improvement, ...
- [ ] Breaking change?
- **Breaking change** - Will anyone consuming this change experience a
break
    in build or boot behavior?
- Examples: Add a new library class, move a module to a different repo,
call
    a function in a new library class in a pre-existing module, ...
- [ ] Includes tests?
  - **Tests** - Does the change include any explicit test code?
  - Examples: Unit tests, integration tests, robot tests, ...
- [ ] Includes documentation?
- **Documentation** - Does the change contain explicit documentation
additions
    outside direct code modifications (and comments)?
- Examples: Update readme file, add feature readme file, link to
documentation
    on an a separate Web page, ...

Tested by running the variable policy test app in Q35.

N/A
@Flickdm Flickdm force-pushed the cherry-pick/release/202405/feature/variable-policy branch from a5de7ad to accf2d8 Compare July 23, 2024 20:18
@Flickdm Flickdm merged commit 94bb414 into microsoft:release/202405 Jul 23, 2024
29 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact:non-functional Does not have a functional impact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants