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

OvmfPkg: Add C runtime apis referenced by Boringssl #6539

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

Conversation

leenasoman
Copy link

Description

As part of the effort to get Boringssl to work with EDK2, there are many C runtime apis that get referenced and are currently not implemented in EDK2. This change adds those apis to edk2. In particular, this change adds the following :

Stubs for file functions. These have been implemented to return -1 rather than 0 since there is no file system support. This will ensure that any calls into these apis will return failure.

Implementations of the following api which are referenced and used by Boringssl code :
strdup : string duplication function
bsearch : binary search
getentropy : return entropy of requested length

  • 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

Integration Instructions

NA

@leenasoman leenasoman force-pushed the crt_v1 branch 6 times, most recently from 57ed2aa to 923c0fe Compare December 17, 2024 09:45
@leenasoman leenasoman marked this pull request as ready for review December 17, 2024 12:14
@ardbiesheuvel
Copy link
Member

Please provide some context about what BoringSSL is, why it exists and why it is important for us [Google] to support it in EDK2.

The changes themselves look reasonable to me, but it would be better to create a separate C file for them - that way, even non-LTO builds will only pull bsearch() or strdup() into the build if they are being used.

@leenasoman
Copy link
Author

Thank you @ardbiesheuvel . I have addressed these comments, except that I kept strdup in CrtWrapper.c with other string functions.

Please provide some context about what BoringSSL is, why it exists and why it is important for us [Google] to support it in EDK2.

The changes themselves look reasonable to me, but it would be better to create a separate C file for them - that way, even non-LTO builds will only pull bsearch() or strdup() into the build if they are being used.

@ardbiesheuvel
Copy link
Member

Thank you @ardbiesheuvel . I have addressed these comments, except that I kept strdup in CrtWrapper.c with other string functions.

OK

But please combine these changes into a single patch.

@liyi77
Copy link
Contributor

liyi77 commented Dec 20, 2024

Is this part of the work to bring in BoringSSL? I don't see the benefit of this change at the moment when BoringSSL is not supported in CryptoPkg yet.
I prefer to review it after your entire patch series is ready.

BoringSSL is a fork of OpenSSL that is designed to meet Google's needs.
As documented at https://boringssl.googlesource.com/boringssl,
Boringssl manages Google's patches to openssl and is used in almost all of
Google products that rely on OpenSSL.
A fork of EDK2 forms the basis for the Uefi code in GCP compute and Google
would like to use Boringssl to work with it.
As part of that effort, there are many C runtime apis that get referenced
and are currently not implemented in EDK2.
This change adds those apis to edk2.
In particular, this change adds the following :
- Stubs for file functions added to CrtWrapper.c. These have been
implemented to return -1 rather than 0 since there is no file system
support. This will ensure that any calls into these apis will return
failure.
- Implementation of (strdup : string duplication function)
- Implementations of the following apis in CrtUtils.c, which are
referenced and used by the Boringssl code :
bsearch : binary search
getentropy : return entropy of requested length

Signed-off-by: Leena Soman <[email protected]>
@leenasoman
Copy link
Author

Is this part of the work to bring in BoringSSL? I don't see the benefit of this change at the moment when BoringSSL is not supported in CryptoPkg yet. I prefer to review it after your entire patch series is ready.
Thanks for the review. Yes it is part of the work. There will be more patches, but I think this patch adds stubs and generic utility functions, hence trying to upstream it.

@leenasoman
Copy link
Author

Hi @liyi77 ,
Will it be possible to review and upstream this in parts? As stated earlier, this patch adds stubs and generic utility functions which might be useful for other consumers of CrtWrapper as well.

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