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

MdePkg: DebugLib: Check Signature in CR in Release Builds #6242

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

os-d
Copy link
Contributor

@os-d os-d commented Sep 26, 2024

Description

The CR macro is used to access an enclosing structure from a pointer within the structure. In DEBUG builds (i.e. when MDEPKG_NDEBUG is not set and debug asserts are enabled), this macro does signature validation checking to ensure that the structure that has been found is the correct structure, based on a signature passed in by the caller.

However, if MDEPKG_NDEBUG is set or debug asserts are disabled, no signature validation is performed, meaning that CR may return an invalid structure that the caller believes is valid and has had signature validation on, causing undefined behavior (memory corruption). We should where at all possible have defined behavior, particularly in RELEASE builds, which are what typical platforms will ship to consumers.

This patch updates CR to do the signature validation in all scenarios to provide defined behavior from the macro. In the event of a signature failure, CR will either 1) assert if !MDEPKG_NDEBUG and debug asserts are enabled (existing behavior) or 2) return NULL to indicate to the caller that signature validation failed.

There exist consumers today who already, erroneously, rely on this behavior, e.g.

Snp = EFI_SIMPLE_NETWORK_DEV_FROM_THIS (This);
OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
if (Snp == NULL) {
return EFI_DEVICE_ERROR;
}
.

Another macro, BASE_CR, exists for callers who do not wish to perform signature validation. Any code that wishes to avoid the signature validation should move to this macro.

This PR also updates EmulatorPkg's graphics stack to add the signature to a structure it is using, a failure that was caught by introducing defined behavior to the CR macro.

  • Breaking change?
    • Breaking change - Does this PR cause a break in build or boot behavior?
    • Examples: Does it add a new library class or move a module to a different repo.
  • Impacts security?
    • Security - Does this PR have a direct security impact?
    • Examples: Crypto algorithm change or buffer overflow fix.
  • Includes tests?
    • Tests - Does this PR include any explicit test code?
    • Examples: Unit tests or integration tests.

How This Was Tested

Tested booting OVMF.

Integration Instructions

This is marked as a breaking change as CR now is expected to return NULL when either MDEPKG_NDEBUG is set or debug asserts are disabled. Consumers should check if CR has returned NULL and handle it appropriately.

In practice, this is not likely to be a breaking change because consumers that are breaking in RELEASE builds here would almost certainly be breaking in DEBUG builds and hitting an assert, although it is possible. This risk is worthwhile, however, because it closes an undefined behavior condition in RELEASE builds. Today's RELEASE build code that breaks here would silently corrupt memory.

@github-actions github-actions bot added the impact:breaking-change This change breaks existing APIs impacting build or boot. label Sep 26, 2024
When updating MdePkg's CR macro to enforce signature checking in
all usages, it was discovered that EmulatorPkg was initializing
a structure without setting the signature for it, causing an error
to be returned when CR now checked the signature.

This commit updates the graphics stack in EmulatorPkg to set the
signature of the data structure and check the return value of
the wrapper for the CR macro.

Signed-off-by: Oliver Smith-Denny <[email protected]>
The CR macro is used to access an enclosing structure from a
pointer within the structure. In DEBUG builds (i.e. when
MDEPKG_NDEBUG is not set and debug asserts are enabled), this
macro does signature validation checking to ensure that the
structure that has been found is the correct structure, based
on a signature passed in by the caller.

However, if MDEPKG_NDEBUG is set or debug asserts are disabled,
no signature validation is performed, meaning that CR may return
an invalid structure that the caller believes is valid and has had
signature validation on, causing undefined behavior (memory
corruption). We should where at all possible have defined behavior,
particularly in RELEASE builds, which are what typical platforms
will ship to consumers.

This patch updates CR to do the signature validation in all scenarios
to provide defined behavior from the macro. In the event of a
signature failure, CR will either 1) assert if !MDEPKG_NDEBUG and
debug asserts are enabled (existing behavior) or 2) return NULL to
indicate to the caller that signature validation failed.

There exist consumers today who already, erroneously, rely on this
behavior.

Another macro, BASE_CR, exists for callers who do not wish to perform
signature validation. Any code that wishes to avoid the signature
validation should move to this macro.

Signed-off-by: Oliver Smith-Denny <[email protected]>
@os-d
Copy link
Contributor Author

os-d commented Sep 30, 2024

@lgao4 I have updated this PR with one additional commit that fixes the CI break. There was a bug in EmulatorPkg where it wasn't setting the signature in a structure it was trying to use the CR macro on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact:breaking-change This change breaks existing APIs impacting build or boot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants