Fix MbedTLS allocations at runtime #163
Merged
Conversation
This file contains 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
Make gmtime() return pointer to a statically allocated storage. This is how it's documented and that's how it gets used. The API isn't thread-safe but given that EDK2 is essentially single-threaded (MP Services Protocol is a special exception). Given that this code gets run at runtime phase, the leak could potentially be used to exhaust memory reserved for the runtime phase. Signed-off-by: Sergii Dmytruk <[email protected]>
This is a follow up for 6ca2060 and 784750e which provide more detailed information on the issue and how this addresses it. The files modified by this commit were chosen based on the list of sources in CryptoPkg/Library/BaseCryptLibMbedTls/RuntimeCryptLib.inf The only source permitted to request memory from boot services is SysCall/RuntimeMemAllocation.c which does it in constructor before ExitBootServices() is called. Trying to update minimal set of files because some of the API which does allocations get used outside of BaseCryptLibMbedTls and can do FreePool(). In the updated files, allocations are of two types: - temporary allocations within a function (they don't get returned or set to some output parameter) - paired alloc/free kind of functions which remain in control of how the memory is treated Signed-off-by: Sergii Dmytruk <[email protected]>
SergiiDmytruk
requested review from
mkopec,
miczyg1,
krystian-hebel and
filipleple
krystian-hebel
approved these changes
Aug 16, 2024
That's great idea, otherwise we will keep maintaining more and more diff in the long run. |
14 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
As was recently reported on Matrix for CachyOS on Nova Custom V56 and in Dasharo/dasharo-issues#1001 for FreeBSD on Protectli V1410, EFI runtime can crash when dealing with SecureBoot EFI variables as in #129 (comment).
I originally fixed it only in a single file which showed up in #129 (comment) and somehow concluded that it should be enough which wasn't true. This attempt should be more successful, making it a PR so that more people can take a look and/or test.
The issue can be reproduced in QEMU and probably anywhere else. I installed Debian, reset SecureBoot keys, downloaded https://github.com/Foxboron/sbctl/releases/tag/0.15.4 binary and tried following https://wiki.cachyos.org/configuration/secure_boot_setup/#setting-up-sbctl.
sbctl enroll-keys -mfailed the same way it was described on Matrix. The build from this branch succeeded.Read commit messages for more details.
P.S. Maybe this should also be sent upstream, which lacks these and some other fixes made previously.