MdePkg: Add CUSTOM_STACK_CHECK_LIB Macro

[os-d]

Description

Commit ac43bba
added StackCheckLibNull to MdeLibs.dsc.inc per review requests on the PR:
#5957 (comment)
#5957 (comment)

The PR was adapted to move specifying StackCheckLibNull in every DSC
to MdeLibs.dsc.inc. However, while this works, it does not allow for
a platform to use one of the other StackCheckLibs (such as
StackCheckLibStaticInit) because we get a linker error by having the
compiler defined stack cookie variables defined more than once (once
from StackCheckLibNull in MdeLibs.dsc.inc and the other in the actual
StackCheckLib implementation). Every platform must include MdeLibs.dsc.inc
and there is no way to override a NULL library class.

In order to support a platform overriding StackCheckLibNull
provided by MdeLibs.dsc.inc, the CUSTOM_STACK_CHECK_LIB macro
is introduced. If this macro is defined, MdeLibs.dsc.inc will
not link StackCheckLibNull and it is expected that the platform
will link the version(s) of StackCheckLib that it requires.

The StackCheckLib README is also updated in this patch to
document the new macro and provide additional information.

• 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

This has been tested that StackCheckLibStaticInit can be used in a package's CI instead of the null version now.

Integration Instructions

If a platform would like to include a real version of the StackCheckLib, it should include:

[Defines]
  DEFINE CUSTOM_STACK_CHECK_LIB = TRUE

[LibraryClasses]
  NULL|MdePkg/Library/StackCheckLib/StackCheckLibStaticInit.inf # e.g., could be different instance
  StackCheckFailureHookLib|MdePkg/Library/StackCheckFailureHookLibNull/StackCheckFailureHookLibNull.inf

For the current details, see https://github.com/tianocore/edk2/blob/HEAD/MdePkg/Library/StackCheckLib/Readme.md.

[tianocore-assign-reviewers]

⚠ WARNING: Cannot add some reviewers: A user specified as a reviewer for this PR is not a collaborator of the repository. Please add them as a collaborator to the repository so they can be requested in the future.

Non-collaborators requested:

• @bibo-mao
• @CorvinK
• @lixianglai

Attn Admins:

• @jljusten
• @lgao4
• @mdkinney
• @vincent-j-zimmer
• @jkmathews
• @miki-intel-work

---

Admin Instructions:

• Add the non-collaborators as collaborators to the appropriate team(s) listed in teams
• If they are no longer needed as reviewers, remove them from Maintainers.txt

[os-d]

@ardbiesheuvel @lgao4, I took the recently merged stack cookies PR and realized why the original commits did not use MdeLibs.dsc.inc to host the null implementation, because we cannot override it with a non-null implementation, as it is a NULL library class. Can you please review this?

[os-d]

@lgao4 @mdkinney can you review, please?

[mdkinney]

Is it possible to use a PCD and #if statement in MdePkgLibs.dsc to conditionally add the NULL lib class? If disabled, it would allow platform DSC to provide an alternate NULL lib instance.

[os-d]

Is it possible to use a PCD and #if statement in MdePkgLibs.dsc to conditionally add the NULL lib class? If disabled, it would allow platform DSC to provide an alternate NULL lib instance.

@mdkinney I don't think we can use a PCD here, based on my tests. When I introduce a PCD to MdePkg.dec and add

[LibraryClasses.common.PEI_CORE, LibraryClasses.common.PEIM, LibraryClasses.common.DXE_CORE, LibraryClasses.common.SMM_CORE, LibraryClasses.common.MM_CORE_STANDALONE, LibraryClasses.common.DXE_DRIVER, LibraryClasses.common.DXE_RUNTIME_DRIVER, LibraryClasses.common.DXE_SMM_DRIVER, LibraryClasses.common.MM_STANDALONE, LibraryClasses.common.UEFI_DRIVER, LibraryClasses.common.UEFI_APPLICATION]
  !if (gEfiMdePkgTokenSpaceGuid.PcdNullStackCheckLib == TRUE)
  NULL|MdePkg/Library/StackCheckLibNull/StackCheckLibNull.inf
  !endif

The build fails because MdeLibs.dsc.inc does not have the PCD set inside of it, looks like the failure comes from here:

edk2/BaseTools/Source/Python/Workspace/DscBuildData.py

Line 961 in 7796422

EdkLogger.error('Parser', FORMAT_INVALID, "PCD (%s) is not defined in DSC file" % Excpt.Pcd,

. Adding the PCD to the DSC that is including MdeLibs.dsc.inc does not work, I have to add the PCD directly to MdeLibs.dsc.inc. When I do that, overriding the PCD in the endpoint DSC doesn't work, it seems that MdeLibs.dsc.inc takes the local PCD value when it processes the if statement.

Using a macro does work:

[LibraryClasses.common.PEI_CORE, LibraryClasses.common.PEIM, LibraryClasses.common.DXE_CORE, LibraryClasses.common.SMM_CORE, LibraryClasses.common.MM_CORE_STANDALONE, LibraryClasses.common.DXE_DRIVER, LibraryClasses.common.DXE_RUNTIME_DRIVER, LibraryClasses.common.DXE_SMM_DRIVER, LibraryClasses.common.MM_STANDALONE, LibraryClasses.common.UEFI_DRIVER, LibraryClasses.common.UEFI_APPLICATION]
  !ifndef $(USE_CUSTOM_STACKCHECKLIB)
    NULL|MdePkg/Library/StackCheckLibNull/StackCheckLibNull.inf
  !endif

When the endpoint DSC does not set this value, the null version is used, if it does include it, then this is not included and the endpoint DSC must include a version of the StackCheckLib. To be honest, I would like to avoid adding more custom macros to the build system as it adds complexity and we might hit other weird scenarios. But if you prefer this scenario, we can go this route.

[mdkinney]

Did you use a Feature PCD? How was the PCD declared in MdePkg.dec?

Here is an example of using a PCD in a DSC file:

https://github.com/tianocore/edk2-platforms/blob/6cfae9dac2ec4d99a10c9efd6d20a521904931fe/Platform/Intel/AlderlakeOpenBoardPkg/AlderlakePRvp/OpenBoardPkg.dsc#L356

!if gMinPlatformPkgTokenSpaceGuid.PcdTpm2Enable == TRUE
$(PLATFORM_PACKAGE)/Tcg/Tcg2PlatformPei/Tcg2PlatformPei.inf
!endif

[os-d]

Did you use a Feature PCD? How was the PCD declared in MdePkg.dec?

@mdkinney I originally used a FixedAtBuild PCD, but I see the same behavior when I use a FeatureFlag PCD. Here is how I defined the PCD in MdePkg.dec:

[PcdsFeatureFlag]
  gEfiMdePkgTokenSpaceGuid.PcdNullStackCheckLib|TRUE|BOOLEAN|0x1000000E

And then if I try to use it in MdeLibs.dsc.inc:

[LibraryClasses.common.PEI_CORE, LibraryClasses.common.PEIM, LibraryClasses.common.DXE_CORE, LibraryClasses.common.SMM_CORE, LibraryClasses.common.MM_CORE_STANDALONE, LibraryClasses.common.DXE_DRIVER, LibraryClasses.common.DXE_RUNTIME_DRIVER, LibraryClasses.common.DXE_SMM_DRIVER, LibraryClasses.common.MM_STANDALONE, LibraryClasses.common.UEFI_DRIVER, LibraryClasses.common.UEFI_APPLICATION]
  !if (gEfiMdePkgTokenSpaceGuid.PcdNullStackCheckLib == TRUE)
    NULL|MdePkg/Library/StackCheckLibNull/StackCheckLibNull.inf
  !endif

The build fails with:

ERROR - Compiler #3000 from /home/osde/edk2/MdePkg/MdeLibs.dsc.inc(41): PCD (gEfiMdePkgTokenSpaceGuid.PcdNullStackCheckLib) is not defined in DSC file

And only if I add this to MdeLibs.dsc.inc does that error go away:

[PcdsFeatureFlag]
  gEfiMdePkgTokenSpaceGuid.PcdNullStackCheckLib|TRUE

But then that is setting the PCD and including that lib, which doesn't let it be overridden.

I can, in the endpoint DSC (I'm using FatPkg.dsc for my testing) do a conditional with the PCD and not have it defined in the DSC (like in your example). Seemingly with the dsc.inc BaseTools is taking a different path and complains when the PCD is not defined in the dsc.inc itself. If I set the PCD in the endpoint DSC (FatPkg.dsc) I still get the build error that MdeLibs.dsc.inc does not include the PCD.

So, from this I think there are only three options:

• Figure out why BaseTools is taking a different path for dsc.inc and resolve that (risky, who knows what else is depending on that behavior)
• Use a macro instead of a PCD. This works, but as mentioned feels much hackier than a PCD.
• The method I have here in this PR, which removes it from MdeLibs.dsc.inc, but then every DSC needs to include a version of StackCheckLib

Unless you have any other ideas. I can push up a test branch to illustrate the issue with the PCD if you want.

[mdkinney]

Thanks for the detailed investigation.

My concern with the current patch is that it requires updates to all downstream DSC files that consume edk2 repo. If we can get the settings into MdeLibs.dsc.inc that preserve the existing behavior, then no downstream DSC files need updates. Only platforms that want to enable new behavior would enable the new features.

[mdkinney]

It also seems that use of a NULL lib instance makes this harder to manage because it can not be overridden in a platform DSC file. Was a formal lib class considered? Could all the entry point lib instances be updated to depend on a new lib class so the lib dependency is only required for all module types other than SEC?

[os-d]

It also seems that use of a NULL lib instance makes this harder to manage because it can not be overridden in a platform DSC file. Was a formal lib class considered? Could all the entry point lib instances be updated to depend on a new lib class so the lib dependency is only required for all module types other than SEC?

This went through a few iterations in Project Mu and I do believe it started as a formal library class. I don't know the full history of why the change was made, I'll follow up with @spbrogan to see if there was an inherent issue with a formal library class. It may just have been trying to not be as high touch as requiring the entry point libs to need a new library class. I agree that having this as a NULL library class adds challenges. My initial testing of using a formal library class tied to the entry point libs looks promising, my simple FatPkg test went fine and the more complex MdePkg and OvmfPkg IA32,X64 builds succeeded. I only tried on GCC, let me code that up more formally and push that up, let's see if all CI passes. My initial concern was on building libraries in a DSC (say MdePkg CI build), since I was assuming that they would not link against the entry point lib until actually included in a PEIM/Driver, but that doesn't seem to be the case, as the build is passing.

[spbrogan]

A formal library class and a dependency added to EntryPointLibs for this seems like a bad design. The library class doesn't have a complete API, is compiler specific, and is controlled not by dependency and usage within code but by compiler flags. Additionally, just leveraging EntryPointLib with the hope that this will catch all modules dependency is an unreliable solution.

While the NULL| in MdeLibs.dsc.inc does have the unfortunate/unacceptable property of not being overridable I don't think anything with dsc.inc design should influence module and library design as the dsc.inc design has so many unfortunate flaws/confusion inducing challenges for real platforms that it shouldn't be used for anything other than a CI crutch.

[os-d]

@mdkinney , after further discussion with @spbrogan , I am going to edit this PR to just be a documentation update to the stack check lib README, to wit that the recommendation is for a platform not to use MdeLibs.dsc.inc and be fully in control of their library classes. This allows a platform to use whatever instance(s) of StackCheckLib they would like to. As you see from my latest attempt, going with linking StackCheckLib against the entry point libs is not enough, there are components that do not link against those libs, but are built with stack cookies. Realistically you'd have to target something like BaseLib, but StackCheckLib (a real implementation) depends on BaseLib so you'd add a circular dependency there. MdeLibs.dsc.inc makes sense for a CI flow and having a copy of the null StackCheckLib there ensures all CI doesn't break. If a platform doesn't care to add real stack cookie checking, it can continue to use MdeLibs.dsc.inc, although in general dsc.incs can obfuscate what is really going on in a platform build, so I would still recommend a platform be extremely explicit in it's DSC.

[mdkinney]

I disagree with limiting use of dsc.inc files to only CI builds. Use of dsc.inc files from platform builds are useful to get the recommended default settings from a package and then override the settings needed by a platform. This allows a package to make changes over time that introduce new features that do not require updates to all platform dsc files.

[leiflindholm]

I disagree with limiting use of dsc.inc files to only CI builds. Use of dsc.inc files from platform builds are useful to get the recommended default settings from a package and then override the settings needed by a platform. This allows a package to make changes over time that introduce new features that do not require updates to all platform dsc files.

Agreed. At least until such time as we implement default implementations otherwise in the build system, we definitely need the .dsc.inc for platforms.

[mdkinney]

One change in #5957 is to unconditionally enable stack checking in VS and GCC compilers.

If we want to think of stack checking as a major feature such as NETWORK_ENABLE and SECURE_BOOT_ENABLE, then should STACK_CHECK_ENABLE define be introduced and only enable stack checking in the compiler if that define is set? That can also provide a define to know what type of stack checking is required. A Null stack check for CI builds, vs default stack checking following recommendations for different module types, vs platform specific stack checking. Perhaps values such as

• OFF -Stack checking is disabled in the compiler and no NULL stack check libs are included
• DEFAULT - Stack checking is enabled in the compiler and the default NULL stack check libs are enabled in MdeLibs.dsc.inc
• CUSTOM - Stack checking is enabled in the compiler and the default NULL stack check libs are not enabled in MdeLibs.dsc.inc

I would also like to point out that enabling stack checking pulls in a dependency on compiler specific intrinsic functions that may change across releases of compilers. #5957 is fragile, because it is implemented against the stack check intrinsics in VS2019 and VS2022 and the implementation does not allow for different intrinsics based on the compiler version.

There have been other discussions about support for compiler instrinics for all compilers across multiple versions, and we may have to work on a general solution to that problem before the support for stack checking can be implemented in a way that can survive future compiler changes.

[os-d]

@mdkinney @leiflindholm, to be clear, I don't mean to advocate that all dsc.inc files be dropped. There is value from some of them. I do maintain that for a physical platform the value is dubious, because as someone who has to own every security and stability aspect of a shipping platform, I would want to know every single aspect of what is getting included there and not have settings be changed or library classes out from under me. But, that is the decision of the platform owner and from the edk2 side, we don't need to prescribe that to them. For platform owners happy with dsc.inc files, great, use them.

My statement is more about MdeLibs.dsc.inc in general, which I think is most appropriate for CI cases. A platform can still use it if they don't want any custom behavior there, but there is not much there and only library classes which never change, so that is easy to add to a platform. Anything that gets added to MdeLibs.dsc.inc is a breaking change (even if it can be swept under the rug by adding to MdeLibs.dsc.inc) and so as a platform owner, I would want to know that some new dependency has been added so I can evaluate it.

To @mdkinney's comments on adding macros: this is possible and is what I tested out earlier in this PR thread and mentioned. I do think that adding new macros feels hacky. I disagree that the current StackCheckLib implementation is fragile in the sense that it is tied to a toolchain, stack cookies will always be tied to a toolchain and that's why the library implementation was done: if a new toolchain changes these functions/variables (which is unlikely, I don't think either the VS or GCC ones have changed in the history of the compilers support of them), a new file gets added to StackCheckLib with those functions/variables. The toolchain being added that changes support could also not add stack cookies if needed while stabilizing it.

I think we should take the opportunity to discuss this topic in the next edk2 CI/Tools meeting (or if a community meeting comes first). What do you think @mdkinney?

[os-d]

@mdkinney I talked further with @spbrogan on this: I think going the macro route makes sense and keeping it constrained to MdeLibs.dsc.inc. As I said, I already tested this out. The one change I would make to your list is that I think that the macro should only have two cases:

• NULL: MdeLibs.dsc.inc includes StackCheckLibNull
• CUSTOM: MdeLibs.dsc.inc does not include StackCheckLibNull and a platform must include one or more implementations

I don't think we should do OFF for two reasons: I don't think we should extend this macro to tools_def.template (I'm not sure if that even works) and NULL is the same thing as OFF. If StackCheckLibNull is linked against, stack check failures are allowed to resume (i.e. same behavior as when stack cookies are not enabled). If a platform wanted to override tools_def.template in an INF, DSC, or their custom tools_def, that still works, they can either set the macro to CUSTOM and not include StackCheckLibNull or leave the macro on NULL and the linker won't link it because the functions/variables are used.

If this approach works for you, I will change this PR to do that. Still happy to discuss further in the CI meeting if you want.

[mdkinney]

You can manage the compiler flags in MdeLibs.dsc.inc by adding a [BuildOptions] section that extends the compiler flags to enable stack checking.

The main reason I wanted to consider OFF was to keep the code generation the same as before introducing this feature, which may have some small size savings over NULL.

[os-d]

@mdkinney Gotcha. I would definitely prefer to keep these compiler flags in tools_def.template. Outside of a module overriding the build options (which ideally should also be pretty few and far between), I think all compiler options should live there. Spreading it out to other files has a great potential to make this more confusing and hard to identify what is occurring and has more potential for weirdness when new compiler flags get added but we are concatenating in MdeLibs.dsc.inc.

If a platform wants to disable stack cookies entirely, they could do that in their platform DSC [BuildOptions], that would be the same thing, but keep the compiler flags centralized.

The other piece is that I believe we as edk2 should be saying we believe stack checking is an important security consideration and platforms should be doing it. The option exists to disable it but you are weakening the security posture of your platform by doing so. It is the same statement we are making with 4k section alignment. That bloats images by a lot more than stack cookies, but is fundamental to our security story.

I will update this PR and put notes in the README and integration section of the PR instructing platforms how to completely disable stack cookies, use the NULL ones, and CUSTOM ones.

[mdkinney]

I agree that the compiler flags to disable can be in platform DSC. That does make more sense. Can document how to do it in README.

I do agree this feature should be enabled by default. Just want an option for platforms to opt-out if they have a reason to do so. Could be for debug or size or use of a compiler that does not support the feature.

[os-d]

@mdkinney I have pushed up this change. Happy to change the name of the macro, I'm not attached to this. I debated still reading a value from the macro, in case it needs to be extended in the future, but decided it probably didn't matter. But I can add that if you prefer.

[os-d]

I just realized I need to update the PR title and description, but I walked away from my computer. I will do that tomorrow.

I did test this for the expected use cases, building as is has no change, defining the macro requires a DSC to provide its own implementation, adding StackCheckLibStaticInit works.

[os-d]

@mdkinney does this match your expectation?

[os-d]

@mdkinney friendly ping to re-review

[os-d]

@mdkinney , I updated the README for clarity. Can you re-review?

[mdkinney]

Is this still considered a "breaking-change"? Can that label be removed?

[os-d]

Is this still considered a "breaking-change"? Can that label be removed?

@mdkinney, this is no longer a breaking-change, thanks. That label can be removed. I do not permission to remove it and I when I previously removed it from the checkbox in the PR description, the GH action doesn't run again to process labels.