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

[REBASE && FF] Update Stack Cookie Libraries #610

Merged

Conversation

TaylorBeebe
Copy link
Contributor

@TaylorBeebe TaylorBeebe commented Nov 3, 2023

StackCheckLib provides the stack cookie globals for both GCC and MSVC
builds. StackCheckFailureLib provides a function to call when a stack
cookie check fails to give platforms a hook to perform actions
before an interrupt is called. After StackCheckFailure() is called,
an interrupt will be generated at the vector specified by
PcdStackCookieExceptionVector.

This series transitions the core to use the new stack cookie libraries.
The stack cookie value no longer needs to be initialized before
image execution and can instead be initialized in the library
constructor. This update also allows the cookie value to be
randomized for GCC buids which was previously not possible because
GCC does not use the cookie value from the image PE/COFF header.

Because a cross-compiler implementation of stack cookies is available,
stack cookies can be enabled for GCC5 builds.

This implementation is also a net reduction in our EDK2 diff footprint.

  • 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

Tested on GCC and VS builds of Q35

Integration Instructions

StackCheckLib will need to be linked to every module built with /GS for MSVC and -fstack-protector for GCC. An instance of StackCheckFailureLib will need to be added to all platform DSC files which utilize StackCheckLib.

[LibraryClasses.X64]
  NULL|MdePkg/Library/StackCheckLib/StackCheckLib.inf
  StackCheckFailureLib|MdePkg/Library/StackCheckFailureLibNull/StackCheckFailureLibNull.inf

@github-actions github-actions bot added language:python Pull requests that update Python code impact:breaking-change Requires integration attention impact:security Has a security impact type:documentation Improvements or additions to documentation labels Nov 3, 2023
@TaylorBeebe TaylorBeebe force-pushed the stash_stack_cookie_changes_2 branch from db58249 to 5ceab74 Compare November 3, 2023 17:43
@TaylorBeebe TaylorBeebe marked this pull request as ready for review November 3, 2023 17:51
@TaylorBeebe TaylorBeebe changed the title Update Stack Cookie Libraries [REBASE && FF] Update Stack Cookie Libraries Nov 3, 2023
@TaylorBeebe TaylorBeebe force-pushed the stash_stack_cookie_changes_2 branch 5 times, most recently from 8918428 to 694ba6a Compare November 4, 2023 19:40
@TaylorBeebe TaylorBeebe force-pushed the stash_stack_cookie_changes_2 branch from 694ba6a to 11b5c5f Compare November 7, 2023 17:08
@apop5
Copy link
Contributor

apop5 commented Nov 7, 2023

Do the other entry point libraries that need the NO_STACK_COOKIE statement added as well?

MdePkg\Library\PeimEntryPoint\PeimEntryPoint.inf
MdePkg\Library\PeiCoreEntryPoint\PeiCoreEntryPoint.inf
MdePkg\Library\DxeCoreEntryPoint\DxeCoreEntryPoint.inf
MdePkg\Library\UefiDriverEntryPoint\UefiDriverEntryPoint.inf
MdePkg\Library\StandaloneMmDriverEntryPoint\StandaloneMmDriverEntryPoint.inf
MdePkg\Library\UefiApplicationEntryPoint\UefiApplicationEntryPoint.inf

Also, for completeness, do the Include/*EntryPoint.h files also need to have NO_STACK_COOKIE statements added as well?

@TaylorBeebe
Copy link
Contributor Author

Do the other entry point libraries that need the NO_STACK_COOKIE statement added as well?

MdePkg\Library\PeimEntryPoint\PeimEntryPoint.inf MdePkg\Library\PeiCoreEntryPoint\PeiCoreEntryPoint.inf MdePkg\Library\DxeCoreEntryPoint\DxeCoreEntryPoint.inf MdePkg\Library\UefiDriverEntryPoint\UefiDriverEntryPoint.inf MdePkg\Library\StandaloneMmDriverEntryPoint\StandaloneMmDriverEntryPoint.inf MdePkg\Library\UefiApplicationEntryPoint\UefiApplicationEntryPoint.inf

Also, for completeness, do the Include/*EntryPoint.h files also need to have NO_STACK_COOKIE statements added as well?

Yes, they should be there as well.

However, I just looked into it a bit more and it seems like because ProcessLibraryConstructorList() is defined within every module that it's not necessary to put the attributes in the header files at all. We may be able to save a bunch of MU_CHANGEs if that's the case. I'm having a hard time finding a definite ruling on this, though. Do you know off hand?

@TaylorBeebe
Copy link
Contributor Author

FYI: I'm going to delay wrapping up this PR until the week of 11/20 because every other repo will also need an update to use this new stack cookie library.

@TaylorBeebe TaylorBeebe force-pushed the stash_stack_cookie_changes_2 branch from 11b5c5f to ee65ab9 Compare November 20, 2023 17:11
@codecov-commenter
Copy link

codecov-commenter commented Nov 20, 2023

Codecov Report

Attention: 39 lines in your changes are missing coverage. Please review.

Comparison is base (c78e1cc) 1.87% compared to head (ab03f95) 1.87%.

Files Patch % Lines
MdePkg/Library/StackCheckLib/StackCheckLib.c 0.00% 25 Missing ⚠️
...tackCheckFailureLibNull/StackCheckFailureLibNull.c 0.00% 8 Missing ⚠️
...ePkg/Library/StackCheckLibNull/StackCheckLibNull.c 0.00% 6 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           release/202302     #610   +/-   ##
===============================================
  Coverage            1.87%    1.87%           
===============================================
  Files                1340     1341    +1     
  Lines              286506   286427   -79     
  Branches             5756     5754    -2     
===============================================
  Hits                 5359     5359           
+ Misses             281097   281018   -79     
  Partials               50       50           
Flag Coverage Δ
CryptoPkg 7.36% <ø> (ø)
MdeModulePkg 0.72% <ø> (+<0.01%) ⬆️
MdePkg 3.61% <0.00%> (+<0.01%) ⬆️
PolicyServicePkg 30.20% <ø> (ø)
UefiCpuPkg 5.18% <ø> (ø)
UnitTestFrameworkPkg ∅ <ø> (∅)

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.

@TaylorBeebe TaylorBeebe force-pushed the stash_stack_cookie_changes_2 branch from ee65ab9 to 85e8212 Compare November 21, 2023 17:41
Description

StackCheckLib provides the stack cookie globals for both GCC and MSVC
builds. StackCheckFailureLib provides a function to call when a stack
cookie check fails to give platforms a hook to perform actions
before an interrupt is called. After StackCheckFailure() is called,
an interrupt will be generated at the vector specified by
PcdStackCookieExceptionVector.

- [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, ...

How This Was Tested

Tested on GCC and VS builds of Q35

Integration Instructions

N/A
Description

This patch transitions the core to use the new stack cookie libraries.
The stack cookie value no longer needs to be initialized before
image execution and can instead be initialized in the library
constructor. This update also allows the cookie value to be
randomized for GCC buids which was previously not possible because
GCC does not use the cookie value from the image PE/COFF header.

- [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, ...
- [x] 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

Tested on GCC and VS builds of Q35

Integration Instructions

StackCheckLib will need to be linked to every module built with /GS
for MSVC and -fstack-protector for GCC. An instance of
StackCheckFailureLib will need to be added to all platform DSC
files which utilize StackCheckLib.

```
[LibraryClasses.X64]
  NULL|MdePkg/Library/StackCheckLib/StackCheckLib.inf
  StackCheckFailureLib|MdePkg/Library/StackCheckFailureLibNull/StackCheckFailureLibNull.inf
```
Description

Now that a cross-compiler implementation of stack cookies is available,
stack cookies can be enabled for GCC5 builds.

- [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, ...
- [x] 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

Tested on GCC and VS builds of Q35

Integration Instructions

Platforms building with GCC5 and utilize the MdeModulePkg
tools_def.template will need to include a StackCheckLib instance
in their build.

```
[LibraryClasses.X64]
  NULL|MdePkg/Library/StackCheckLib/StackCheckLib.inf
  StackCheckFailureLib|MdePkg/Library/StackCheckFailureLibNull/StackCheckFailureLibNull.inf
```
Description

Delete the old stack cookie logic.

- [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, ...

How This Was Tested

Tested on GCC and VS builds of Q35

Integration Instructions

N/A
@TaylorBeebe TaylorBeebe force-pushed the stash_stack_cookie_changes_2 branch from 85e8212 to ab03f95 Compare November 21, 2023 20:57
@TaylorBeebe TaylorBeebe enabled auto-merge (rebase) November 21, 2023 21:06
@TaylorBeebe TaylorBeebe merged commit 8a980c0 into microsoft:release/202302 Nov 21, 2023
38 checks passed
@TaylorBeebe TaylorBeebe deleted the stash_stack_cookie_changes_2 branch November 21, 2023 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact:breaking-change Requires integration attention impact:security Has a security impact language:python Pull requests that update Python code type:documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants