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/BaseRngLib: Configure default value for RDRAND state variable #6417

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

philnoh2
Copy link
Contributor

@philnoh2 philnoh2 commented Nov 7, 2024

As a BASE type library, some PEI drivers could link and use it. Tcg2Pei.inf is an example. On edk2-stable202408 version, PEI drivers that link the library include the global variable of mRdRandSupported. The previous commit (c3a8ca7) that refers to the global variable actually is found to influence the link status. Updating the global variable in PEI drivers could affect the following issues.

PEI ROM Boot : Global variable is not updated
PEI RAM Boot : PEI FV integration/security check is failed

To address these issues, based on the library status before the commit, the proposed solution is to configure default value (expected state) for the global variable.

  • 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

The update was verified on some AMD platforms (edk2-stable202408 base) that had the issue.

Integration Instructions

N/A

As a BASE type library, some PEI drivers could link and use it.
Tcg2Pei.inf is an example. On edk2-stable202408 version, PEI drivers
that link the library include the global variable of mRdRandSupported.
The previous commit (c3a8ca7) that refers to the global variable actually
is found to influence the link status. Updating the global variable
in PEI drivers could affect the following issues.

PEI ROM Boot : Global variable is not updated
PEI RAM Boot : PEI FV integration/security check is failed

To address these issues, based on the library status before the commit,
the proposed solution is to configure default value (expected state)
for the global variable.

Signed-off-by: Phil Noh <[email protected]>
@lgao4
Copy link
Contributor

lgao4 commented Nov 14, 2024

This change updates the default value from FALSE to TRUE. Is this the purpose?

@philnoh2
Copy link
Contributor Author

This change updates the default value from FALSE to TRUE. Is this the purpose?

Right. Currently mRdRandSupported is not initialized. On our platform, the value is checked as 0 (FALSE). As the library is base type, it would be best to support a way not to use/update the global variable. Setting the default value (expected state) is a solution for platforms that didn't have a problem with the library version before the commit.

@mdkinney
Copy link
Member

Use of a non-const global variable in a lib of type BASE seems like a bug.

@philnoh2
Copy link
Contributor Author

Use of a non-const global variable in a lib of type BASE seems like a bug.

I have same opinion. To fix the variable usage, I think that moving all code in BaseRngLibConstructor() into ArchIsRngSupported() could be good to remove mRdRandSupported when considering GetRandomNumberxx() is not a high frequency call. For example, I tried to evaluate the update on our platform that uses edk2-stable202408. Then checked that AsmCpuid() and TestRdRand() in ArchIsRngSupported() don't take much time (e.g. TestRdRand () spent 8~9(μs)), and a side effect is not found. Based on that, let me discuss this idea with maintainers. If it is accepted, I would be able to update the PR for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants