Fix: BaseCryptLibOnOneCrypto#1787
Open
Flickdm wants to merge 4 commits into
Open
Conversation
Contributor
✅ QEMU Validation PassedSource Dependencies
Results
Workflow run: https://github.com/microsoft/mu_basecore/actions/runs/25825075018 This comment was automatically generated by the Mu QEMU PR Validation workflow. |
Flickdm
force-pushed
the
fix/OneCryptoLib
branch
2 times, most recently
from
593536f to
17b6cc9
Compare
apop5
approved these changes
May 13, 2026
os-d
approved these changes
May 13, 2026
Use logical OR in the protocol and function pointer null checks in the dispatch macros. This avoids null dereference risk and correctly handles either missing protocol or missing service function pointers. Signed-off-by: Doug Flick <dougflick@microsoft.com>
Add 8 new unit tests for X509ConstructCertificateStack and X509ConstructCertificateStackV to improve coverage of the X509 certificate stack construction API: - NullInput: NULL X509Stack pointer returns FALSE - SingleCert: construct stack with one CA certificate - Append: build stack incrementally across multiple calls - InvalidCert: truncated DER data fails without losing the stack - MultipleCertsOneCall: three certs in a single variadic call - ZeroSize: cert with size 0 fails gracefully - EmptyList: NULL-only terminator succeeds - VDirect: two certs exercising the VA_LIST code path Also fix the test class string for TestVerifyX509 from "CryptoPkg.BaseCryptLib.Hkdf" to "CryptoPkg.BaseCryptLib.X509". Signed-off-by: Doug Flick <dougflick@microsoft.com>
On AARCH64, VA_LIST is incompatible across GCC5 (ELF/AAPCS64) and CLANGPDB (PE/COFF/MSVC) toolchains. GCC5 uses a 32-byte struct tracking register and stack save areas, while CLANGPDB uses a simple char pointer. Passing VA_LIST through the OneCrypto protocol boundary causes the binary to misinterpret arguments - reading the cert pointer value as the cert size. Replace the direct CALL_CRYPTO_SERVICE dispatch of X509ConstructCertificateStackV with a caller-side loop that unpacks the VA_LIST locally (same toolchain, safe) and calls the non-variadic X509ConstructCertificateStack protocol function once per certificate. Each call passes a single cert/size pair terminated by NULL, avoiding any VA_LIST crossing the cross-toolchain protocol boundary. Signed-off-by: Doug Flick <dougflick@microsoft.com>
Add the missing "id" field to OneCrypto_ext_dep.json. This field is required by the edk2-pytool ext_dep override mechanism to allow downstream platforms to override the OneCrypto binary dependency using "override_id" in a local ext_dep file. Signed-off-by: Doug Flick <dougflick@microsoft.com>
Javagedes
force-pushed
the
fix/OneCryptoLib
branch
from
dacc97a to
c19dcba
Compare
Javagedes
approved these changes
May 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR should be rebased and merged to keep the commit history because it also has a fewer other smaller but nice to have changes that should go in with this.
This is a workaround to not use VA_LIST in
X509ConstructCertificateStackV- which goes against the UEFI specification and is broken today when using a CLANGPDB compiled binary vs a GCC5 DxeCore. This change will unblock our partner teams but the long term fix is to deprecate the (now unused) functionX509ConstructCertificateStackV.This pull request refactors and improves the implementation of the
X509ConstructCertificateStackfunctions in the OneCrypto library, making them more robust and better tested. The main changes include replacing a macro-based implementation with an explicit function, adding comprehensive unit tests for various input scenarios, and fixing a logic bug in the error handling macro. These changes enhance the reliability and maintainability of the certificate stack construction logic.X509 Certificate Stack Construction Refactor and Bug Fixes
Implementation improvements:
X509ConstructCertificateStackVwith an explicit function that iterates through the variadic arguments, validates certificate sizes, and appends certificates to the stack, improving clarity and error handling. (CryptoPkg/Library/BaseCryptLibOnOneCrypto/OneCryptoLib.c)X509ConstructCertificateStackto call the newX509ConstructCertificateStackVfunction and return its result, ensuring consistent logic and error propagation. (CryptoPkg/Library/BaseCryptLibOnOneCrypto/OneCryptoLib.c)Bug fixes:
CryptoServiceNotAvailablemacro: changed the condition from&&to||to correctly handle cases where the protocol pointer isNULL, preventing possible null pointer dereference. (CryptoPkg/Library/BaseCryptLibOnOneCrypto/OneCryptoLib.c)Testing improvements:
X509ConstructCertificateStack, covering scenarios such as null input, single and multiple certificates, appending certificates, invalid certificates, zero-size certificates, empty lists, and direct variadic calls. (CryptoPkg/Test/UnitTest/Library/BaseCryptLib/X509Tests.c)Metadata update:
"id": "onecrypto-bin"field to theOneCrypto_ext_dep.jsonfile, improving package identification. (CryptoPkg/Binaries/OneCrypto_ext_dep.json)For details on how to complete these options and their meaning refer to CONTRIBUTING.md.
How This Was Tested
QemuSbsaPkg && Platform Testing
Integration Instructions
<Describe how these changes should be integrated. Use N/A if nothing is required.>