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

[Feature] Added SharedCrypto (drop-in replacement for BaseCryptLib) #22

Conversation

matthewfcarlson
Copy link
Contributor

This uses a precompiled EFI's to speed up compile times when using OpenSSL and decrease space used. This approach can be used by other crypto Libraries other than OpenSSL as long as they conform to the API of BaseCryptLib.

.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
SharedCryptoPkg/Driver/CommonSharedCryptoFull.c Outdated Show resolved Hide resolved
SharedCryptoPkg/Driver/CommonSharedCryptoMu.c Outdated Show resolved Hide resolved
SharedCryptoPkg/Driver/CommonSharedCryptoShaOnly.c Outdated Show resolved Hide resolved
@corthon
Copy link
Member

corthon commented Apr 5, 2019

Build failing? Looks like we forgot to exclude an incompatible architecture somewhere.

@corthon
Copy link
Member

corthon commented Apr 5, 2019

Build failing? Looks like we forgot to exclude an incompatible architecture somewhere.

Actually, I think it's the removal of /GS, so now we may have extraneous libs.

@matthewfcarlson
Copy link
Contributor Author

I think it's the fact that we're missing BaseSecurityLib and RngLib for X64 and X86

@matthewfcarlson
Copy link
Contributor Author

The last thing to do is investigate the tools_def.conf instead of the DSC override.

@mdkinney
Copy link

The Private/Include/Protocol directory should have one file for the DXE Protocol and another file for the SMM Protocol. The SharedProtocol.h can be removed and the DXE Protocol can contain all the contents to define the protocol. The SMM Protocol file includes the DXE Protocol file and defines the SMM GUID and a typedef for the SMM version of the protocol. See MdeModulePkg/Include/Protocol/SmmFirmwareVolumeBlock.h for an example to follow.

@mdkinney
Copy link

The use of the term 'Shared' 'SHARED' should be removed from all types, variables, functions, library, and module names. The private protocol/ppi/smm protocol are an abstraction for the CryptoPkg/Include/BaseCryptLib.h, so the name of the protocol and the modules should make this relationship obvious. Perhaps using the names listed below for includes.

The use of 'Base' in the name of the library class is unfortunate. The term 'Base' in the name of a lib instance implies module compatibility with all module types.

Private/Protocol/BaseCrypt.h
EDKII_BASE_CRYPT_PROTOCOL_GUID
EDKII_BASE_CRYPT_PROTOCOL
gEdkiiBaseCryptProtocolGuid
Private/Protocol/SmmBaseCrypt.h
EDKII_SMM_BASE_CRYPT_PROTOCOL_GUID
EDKII_SMM_BASE_CRYPT_PROTOCOL
gEdkiiSmmBaseCryptProtocolGuid
Private/Ppi/BaseCrypt.h
EDKII_PEI_BASE_CRYPT_PROTOCOL_GUID
EDKII_PEI_BASE_CRYPT_PROTOCOL
gEdkiiPeiBaseCryptProtocolGuid

Library Instances (not using keyword 'Base')
DxeCryptLibOnProtocol
SmmCryptLibOnProtocol
PeiCryptLibOnPpi

Drivers
CryptDxe
CryptSmm
CryptPei

We could define a few feature flag PCDs that select between a NULL value and function pointer value when filling in the structure of crypto services. This can reduce the number of INFs and C files.

Can we also consider making this a pull request against CryptoPkg in mu_tiano_plus?

@matthewfcarlson
Copy link
Contributor Author

There's been some discussion about moving the PublishDrivers.py, PlatformBuild.py, and the Driver DSC out of Mu_plus. The places that make the most sense for it to move to would be a new repo that would be a submodule of mu_plus (similar to OpenSSL) or in Mu_BASECORE under Basetools since that's where our other nuget publishing tools.

One problem with moving is the eventual goal of moving it upstream to tianocore. Since they don't have anyway to resolve the nuget dependency (as of right now), we should provide the tools for anyone to build their own EFI's to use. If it's in a separate repo, that becomes an extra hurdle for someone to adopt SharedCrypto. Just something we need to be mindful of.

@matthewfcarlson
Copy link
Contributor Author

After some discussion, we've decided to move forward with this PR. Our goal is to land this and deploy it internally for testing. We will stage a PR later that moves this into MU_TIANO_PLUS. At that point, we will restructure the NuGet packaging tools and rename the drivers and libraries to align with BaseCryptLib.

@matthewfcarlson
Copy link
Contributor Author

Add this issue to track the comments Mike K made: microsoft/mu#59

@corthon corthon merged commit ed346e1 into microsoft:release/201903 Apr 23, 2019
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.

4 participants