Move StackCheckLib To a Defined Library Class

[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 avoid build breaks, this PR updates all DSCs and relevant
dsc.incs to remove StackCheckLibNull for the CI build. It also changes
it to the StackCheckLib library class in MdeLibs.dsc.inc. StackCheckLib
is added as a LibraryClass dependency to each of the Entry Point libs.

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

Continuous-integration-options: PatchCheck.ignore-multi-package

• 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:

[LibraryClasses]
  # Provides Stack Cookie Implementation
  StackCheckLib|MdePkg/Library/StackCheckLib/StackCheckLibStaticInit.inf # This can be a different implementation, if desired.

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.

[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
• @bcran
• @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

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