Date   

Re: [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface

Zurcher, Christopher J
 

Jiewen,

This patch set assumes that EDK2 is linked with OpenSSL for the foreseeable future. The end goal would be to move all applicable Crypto access to the EVP interface and remove the individual modules we maintain for each algorithm. The primary benefit here is reducing complexity and code duplication. Without this end goal, this patch set will not be particularly useful.

If the design goal of BaseCryptLib is to allow OpenSSL to be replaced by other crypto providers, I should abandon this patch set, and we can save the EVP transition for when moving to OpenSSL 3 becomes mandatory. At that time, the CryptX.c family can be modified to call EVP functions.
(This could even be done transparently, by returning UINTN from GetContextSize and using the user-supplied "context" to store the OpenSSL-supplied context pointer.)
Delaying EVP adoption in this case would also allow more time for platforms to adopt the Crypto Services model, which should help offset the size impact.

Please let me know which path should be taken.

Thanks,
Christopher Zurcher

-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com>
Sent: Wednesday, September 16, 2020 09:05
To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io; Zurcher, Christopher J
<christopher.j.zurcher@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>
Subject: Re: [edk2-devel] [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface

On 09/16/20 16:17, Yao, Jiewen wrote:
Thank you Laszlo.
You forced me to read the code again and more carefully. :-)

I believe I understand why you use NULL to free the context now - to support error path.

First, I did have some new thought for EVP API. Let’s consider 3 cases, 1) Hash all data one time, 2) Hash update
data multiple times, 3) Error during update.

A. In current design, the API sequence is:
1) EvpMdHashAll (Digest)
2) EvpMdInit, EvpMdUpdate, EvpMdUpdate, ... , EvpMdFinal (Digest)
3) EvpMdInit, EvpMdUpdate->ERROR, EvpMdFinal (NULL)

B. We can auto free context in EvpMdUpdate in error path - the API sequence is:
1) EvpMdHashAll (Digest)
2) EvpMdInit, EvpMdUpdate, EvpMdUpdate, ... , EvpMdFinal (Digest)
3) EvpMdInit, EvpMdUpdate->ERROR
I don't like "B" because in the loop body where you call EvpMdUpdate(),
you may encounter an error from a different source than EvpMdUpdate()
itself. For example, you may be reading data from the network or a disk
file. If fetching the next chunk fails, we'd still want to clean up the
EVP MD context. Therefore, if EvpMdUpdate() would itself invalidate the
context, if it failed, then that would *complicate* the error path. (=
Clean up the context after the loop body *only* if something different
from EvpMdUpdate() failed.)


C: We can use New/Free style - similar to HMAC
1) EvpMdHashAll (Digest)
2) EvpMdNew, EvpMdInit, EvpMdUpdate, EvpMdUpdate, ... , EvpMdFinal (Digest), EvpMdFree
3) EvpMdNew, EvpMdInit, EvpMdUpdate->ERROR, EvpMdFree
Yes, this was the first pattern, the one that caused me to say "please
don't do this". In this pattern, the context may exist between "New" and
"Init", and also between "Final" and "Free". Those two states are
useless and only good for causing confusion.

For example, are you allowed to Duplicate a context in those states?
It's best to prevent even the *asking* of that question.


I checked below APIs:
1) openssl (EVP_MD_CTX_new, EVP_DigestInit_ex, EVP_DigestUpdate, EVP_DigestFinal_ex, EVP_MD_CTX_free)
2) mbedtls (mbedtls_sha256_init, mbedtls_sha256_starts_ret, mbedtls_sha256_update_ret, mbedtls_sha256_finish_ret,
mbedtls_sha256_free)
3) BaseCryptoLib HMAC (HmacSha256New, HmacSha256SetKey, HmacSha256Update, HmacSha256Final, HmacSha256Free)

All APIs include free function to free the context, I don’t think it is a bad idea to have EvpMdFree() API.
I would like to propose option - C.
- I cannot comment on mbedtls (2).

- I think the current BaseCryptLib HMAC APIs (3) are not great, and we
should use this opportunity to improve upon them.

- Regarding openssl (1), as I understand it, the situation is different
from edk2. In OpenSSL, EVP_MD_CTX is *not* an opaque type (expressed in
C language terms, it is not an "incomplete" type, but a "complete"
type). Therefore, an expression such as sizeof(EVP_MD_CTX) works.

The consequence is that OpenSSL code itself can use the following style:

void func(void)
{
EVP_MD_CTX ctx;

EVP_DigestInit_ex(&ctx, ...);
...
EVP_DigestFinal_ex(&ctx, ...)
}

In other words, OpenSSL-native code is permitted to know the internals
of EVP_MD_CTX, therefore the OpenSSL-internal code may allocate
EVP_MD_CTX with means that are *different* from EVP_MD_CTX_new() and
EVP_MD_CTX_free().

The caller of (Init, Final) may use (New, Free) for memory management,
or may use something completely different (for example, local variables).

Therefore, offering the (Init, Final) APIs separately from (New, Free)
is meaningful.

But the situation in edk2 -- or any other OpenSSL *consumer* -- is
different. In edk2, EVP_MD_CTX is an opaque type -- in C language terms:
an "incomplete" type. OpenSSL deliberately hides the internals of
EVP_MD_CTX.

See "CryptoPkg/Library/OpensslLib/openssl/include/openssl/ossl_typ.h":

typedef struct evp_md_ctx_st EVP_MD_CTX;
and the "evp_md_ctx_st" structure type is only defined in
"CryptoPkg/Library/OpensslLib/openssl/crypto/evp/evp_local.h", whose
contents -- I think! -- OpenSSL client code cannot, or *should not*,
refer to.

This means that OpenSSL consumer code can *only* rely on
EVP_MD_CTX_new() and EVP_MD_CTX_free(), for allocating and releasing the
context, respectively. "sizeof(EVP_MD_CTX)" is not permitted, and
defining a local or global variable of type EVP_MD_CTX is also not valid.

This means that the only reason for separating (Init, Final) from (New,
Free) falls away, in OpenSSL consumer code. In OpenSSL consumer code,
there is no reason to keep *both* pairs of APIs.


Please note that, this (very prudent) information hiding / encapsulation
in OpenSSL used to be violated by edk2 in the past, intentionally.
That's what the HmacXxxGetContextSize() APIs were about -- those APIs
forcefully leaked the context sizes to client code, so that client code
in edk2 could perform its own allocation.

But in <https://bugzilla.tianocore.org/show_bug.cgi?id=1792>, we finally
eliminated HmacXxxGetContextSize(). As a part of that, we also
simplified the HMAC API -- note that Init was replaced by SetKey. And
because we were reworking an existent API, I didn't propose very
intrusive changes -- I didn't propose fusing Final and Free, for example.

Now, the EVP MD APIs are just being introduced to edk2. So we have a
chance at getting them right, and making them minimal.

Second, I believe EVP is just something in openssl. It does not mean that we *have to* design API to follow
openssl.
After rethink the size impact, I do have concern to merge all Hash implementation together in one function.
It might means nothing if the crypto library is based upon openssl.
But if the cryptolib implementation is based upon other crypto, such as Intel IPP
(https://github.com/slimbootloader/slimbootloader/tree/master/BootloaderCommonPkg/Library/IppCryptoLib) or mbedTls
(https://github.com/jyao1/edk2/tree/DeviceSecurity/CryptoMbedTlsPkg), then we can NOT get size benefit by separating
the hash algorithm.


I would like to propose separate EvpMdxxx.
EvpMdNew -> Sha256New, Sha384New
EvpMdInit -> Sha256Init, Sha384Init
EvpMdUpdate -> Sha256Update, Sha384Update
EvpMdFinal -> Sha256Final, Sha384Final
EvpMdFree -> Sha256Free, Sha384Free
I have no comment on this.

We can do similar thing with Hmac, to deprecate Sha256GetContextSize() API,
Yes, good idea.

and replace caller with Sha256New() / Sha384Free()
Indeed.

But then -- there's no reason left for keeping (New, Free) separate from
(Init, Final).

Thanks
Laszlo


Re: [RFC] Request for the new package "RedfishPkg" under edk2 repo

Felix Polyudov
 

Abner,

 

Are there RedFish dependencies in the REST EX Driver from your package?

If not, perhaps it should be in the NetworkPkg, not in the RedFishPkg.

 

From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Abner Chang
Sent: Monday, September 14, 2020 11:12 PM
To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist); rfc@edk2.groups.io
Cc: Wang, Nickle (HPS SW); Chen, Aaron; siyuan.fu@...; Wang, Fan; Wu, Jiaxin; Ni, Ray; Michael D Kinney
Subject: [EXTERNAL] Re: [edk2-devel] [RFC] Request for the new package "RedfishPkg" under edk2 repo

 

Seems no one has comment on this topic. Let’s just go through the code review process.

Thanks

 

Abner

 

From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Abner Chang
Sent: Wednesday, September 9, 2020 11:02 AM
To: devel@edk2.groups.io; rfc@edk2.groups.io
Cc: Wang, Nickle (HPS SW) <nickle.wang@...>; Chen, Aaron <aaron.chen@...>; siyuan.fu@...; Wang, Fan <fan.wang@...>; Wu, Jiaxin <jiaxin.wu@...>; Ni, Ray <ray.ni@...>; Michael D Kinney <michael.d.kinney@...>
Subject: Re: [edk2-devel] [RFC] Request for the new package "RedfishPkg" under edk2 repo

 

Add [RFC] to the subject, add Ray and Mike to the loop.

 

From: Chang, Abner (HPS SW/FW Technologist)
Sent: Tuesday, September 8, 2020 12:06 PM
To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist) <abner.chang@...>; rfc@edk2.groups.io
Cc: Wang, Nickle (HPS SW) <nickle.wang@...>; Chen, Aaron <aaron.chen@...>; siyuan.fu@...; Wang, Fan <fan.wang@...>; Wu, Jiaxin <jiaxin.wu@...>
Subject: RE: Request for the new package "RedfishPkg" under edk2 repo

 

This is the RFC for the new package "RedfishPkg" introduced to edk2 repo, I thought mailing system will add [RFC] prefix to the subject. Sorry for the inconvenience.

 

From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Abner Chang
Sent: Tuesday, September 8, 2020 11:48 AM
To: rfc@edk2.groups.io
Cc: Wang, Nickle (HPS SW) <nickle.wang@...>; Chen, Aaron <aaron.chen@...>; siyuan.fu@...; Wang, Fan <fan.wang@...>; Wu, Jiaxin <jiaxin.wu@...>; devel@edk2.groups.io
Subject: [edk2-devel] Request for the new package "RedfishPkg" under edk2 repo

 

Hi everyone,

Given that we are going to contribute code of UEFI Redfish edk2 solution, a new package “RedfishPkg” under edk2 repo is necessary for accommodating the UEFI Redfish driver stacks, that includes

-          EFI Redfish Host Interface DXE Driver

-          EFI Refish Credential DXE Driver

-          EFI REST EX UEFI Driver for Redfish service

-          EFI Redfish Discover UEFI Driver

-          EFI Redfish Discover Protocol

-          EFI Redfish Config UEFI Driver

-          EFI BIOS Config To Redfish Dxe Driver

-          EFI REST JSON Structure DXE Driver

-          EFI Source Coding DXE Driver

-          EFI BIOS Resource Provision Generation Protocol

-          EFI BIOS Resource Provision Transport Layer Protocol

 

The architecture have been discussing in TianoCore Design meeting and the corresponding BZ were created as well.

The code we will start to contribute includes

-          Contribute to edk2 repo for those drivers already have the corresponding definitions in UEFI spec.

-          Contribute code to edk2-staging/UEFI _Redfish for those drivers do not have the corresponding definitions in UEFI spec. This is for the evaluation and require ECR to USWG if community agree with having this driver for Redfish edk2 solution.

 

Please refer to below link for the details, https://github.com/tianocore/edk2-staging/blob/UEFI_Redfish/Readme.md

 

Thanks

Abner

P Please consider the environment before printing this email

The information contained in this message may be confidential and proprietary to American Megatrends (AMI). This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.


Re: 回复: [edk2-devel] [PATCH v10 0/5] Use RngLib instead of TimerLib for OpensslLib

Matthew Carlson
 

Hey Liming!
I added the two reviewed by and pushed to the PR here: https://github.com/tianocore/edk2/pull/933

Feel free to use that PR to merge in

--
- Matthew Carlson


Re: more development process failure [was: UefiPayloadPkg: Runtime MMCONF]

Guo Dong
 

Thanks Laszlo for all these information.
It looks it is not well communicated on the merge requirements.
This is the first time for me to merge a patch set and I had thought
it is same with merging a single patch (no cover letter).
As UefiPayloadPkg maintainer, most time I worked on bootloaders.
It would be great if we could have a single page to documents the
rule and steps for maintainers.

And it would be great for maintainers if EDK2 could move to github
code review, and merge from the pull request directly once it passed
the review. We don't need do any extra things for the patch merge.

Thanks,
Guo

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
Ersek
Sent: Wednesday, September 16, 2020 11:14 AM
To: Dong, Guo <guo.dong@intel.com>; devel@edk2.groups.io
Cc: marcello.bauer@9elements.com; Kinney, Michael D
<michael.d.kinney@intel.com>; Leif Lindholm (Nuvia address)
<leif@nuviainc.com>; Doran, Mark <mark.doran@intel.com>; Andrew Fish
<afish@apple.com>; Guptha, Soumya K <soumya.k.guptha@intel.com>
Subject: Re: [edk2-devel] more development process failure [was:
UefiPayloadPkg: Runtime MMCONF]

On 09/16/20 19:30, Dong, Guo wrote:

Hi Laszlo,

The patchset includes 3 patches, and all of them had been reviewed by
package owners.
The patch submitter has a pull request
https://github.com/tianocore/edk2/pull/885, I rebased the patch to latest
master, and merged it by adding reviewed-by found from emails.
I also make sure it passed all the checks before I put "push" button there. then
retrigger a new build with "push" button.

I am not sure what is missing. If there is any other requirements, should they
be captured during code review or tool check?

- The description field of <https://github.com/tianocore/edk2/pull/932/>
is empty. It's difficult to tell where the patches come from -- where
they were posted and reviewed. A copy of the cover letter should have
been included here, plus preferably a link to the v5 mailing list thread
(the one that got merged in the end).
- It was not confirmed in the v5 mailing list thread that the series had
been merged. The confirmation should have included at least one of: (a)
the github PR link, (b) the git commit range. (Preferably: both.)

It's not the eventual git commits that I'm complaining about, but the
lack of communication with the community, and the lack of record for
posterity.

Myself, I used to consider github PRs a means merely for replacing our
earlier direct "git push" commands -- with a CI build + mergify. So, as
a maintainer, I would myself queue up several patch sets in a single
"batch" PR, add some links to BZs and the mailing list, and let it fly.
But then Mike told me this was really wrong, and we should clearly
associate any given PR with a specific patch set on the list.

This meant an *immense* workload increase for me, in particular because
I tend to merge patch sets for *other* people and subsystems too (after
they pass review), that is, for such subsystems that I do not
co-maintain. In particular during the feature freeze periods.

So what really rubs me the wrong way is that, if I am expected to keep
all of this meta-data nice and tidy, why aren't some other maintainers?
It's a double standard.

I can live with either *all of us* ignoring PR tidiness, or *all of us*
doing our best to keep everything nicely cross-referenced.

But right now I spend significant time and effort on keeping
communication and records complete and clean in *all three of* bugzilla,
github, and mailing list, whereas a good subset of the maintainers
couldn't care less in *either* of those communication channels.

For your reference, here's a random PR I submitted and merged for others:

https://github.com/tianocore/edk2/pull/904

Observe in PR#904:

- title carries cover letter subject
- description carries cover letter body
- description has a pointer to the BZ, and a link to the cover letter in
the mailing list archive (two links in fact, in different archives)

And then here's my report back on the list:

https://edk2.groups.io/g/devel/message/64644

And my BZ comment to the same effect (also closing the BZ as
RESOLVED|FIXED):

https://bugzilla.tianocore.org/show_bug.cgi?id=2376#c9
https://edk2.groups.io/g/bugs/message/12777


I don't insist on the particular information content of github PRs, as
-- at this stage -- they really are not more than just a way to set off
CI, before pushing/merging a series.

What I do insist on is that all of us maintainers (people with
permission to set the "push" label) be subject to the same expectations
when it comes to creating pull requests.

(Please note also that I absolutely don't need a BZ for every
contribution. My request is only that *if* there is a BZ, then handle it
thoroughly.)

Laszlo



Thanks,
Guo

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
Ersek
Sent: Wednesday, September 16, 2020 1:57 AM
To: Dong, Guo <guo.dong@intel.com>
Cc: devel@edk2.groups.io; marcello.bauer@9elements.com; Kinney,
Michael D
<michael.d.kinney@intel.com>; Leif Lindholm (Nuvia address)
<leif@nuviainc.com>; Doran, Mark <mark.doran@intel.com>; Andrew Fish
<afish@apple.com>; Guptha, Soumya K <soumya.k.guptha@intel.com>
Subject: [edk2-devel] more development process failure [was:
UefiPayloadPkg:
Runtime MMCONF]

Guo,

On 08/18/20 10:24, Marcello Sylvester Bauer wrote:
Support arbitrary platforms with different or even no MMCONF space.
Fixes crash on platforms not exposing 256 buses.

Tested on:
* AMD Stoney Ridge

Branch: https://github.com/9elements/edk2-1/tree/UefiPayloadPkg-
MMCONF
PR: https://github.com/tianocore/edk2/pull/885

v5:
* MdePkg
- support variable size MMCONF in all PciExpressLibs
- use (UINTX)-1 as return values for invalid Pci addresses
Okay, so we got more of the same development process violations here, as
I've just reported at <https://edk2.groups.io/g/devel/message/65313>.

See this new pull request:

https://github.com/tianocore/edk2/pull/932/

"No description provided."

You should be embarrassed.

Laszlo








Re: [PATCH v7 11/14] SecurityPkg: Allow VariablePolicy state to delete authenticated variables

Bret Barkelew
 

These are both great points and I’ll make both changes.

 

Thanks!

 

- Bret

 

From: Yao, Jiewen via groups.io
Sent: Tuesday, September 15, 2020 7:52 PM
To: Yao, Jiewen; gaoliming; devel@edk2.groups.io; Wang, Jian J; bret@...; Bi, Dandan
Cc: Wu, Hao A; liming.gao; Justen, Jordan L; 'Laszlo Ersek'; 'Ard Biesheuvel'; 'Andrew Fish'; Ni, Ray
Subject: [EXTERNAL] Re: [edk2-devel] [PATCH v7 11/14] SecurityPkg: Allow VariablePolicy state to delete authenticated variables

 

Hi Bret
I have minor comment below. Please let me know your thought.

 
> -----件原件-----
> 件人: bounce+27952+64723+4905953+8761045@groups.io
> <bounce+27952+64723+4905953+8761045@groups.io> 代表 Bret Barkelew
> 时间: 2020828 13:51
> 收件人: devel@edk2.groups.io
> 抄送: Jiewen Yao <jiewen.yao@...>; Jian J Wang
> <jian.j.wang@...>; Chao Zhang <chao.b.zhang@...>
> : [edk2-devel] [PATCH v7 11/14] SecurityPkg: Allow VariablePolicy state
> to delete authenticated variables
>
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2522&amp;data=02%7C01%7Cbret.barkelew%40microsoft.com%7C8f1aee9f15a14900a3d508d859eb7222%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637358215630854973&amp;sdata=JyDdXC9JRdBdqfmhAc6fI4R5xhh70wcD0NnNIgwfF3w%3D&amp;reserved=0
>
> Causes AuthService to check
> IsVariablePolicyEnabled() before enforcing
> write protections to allow variable deletion
> when policy engine is disabled.
>
> Only allows deletion, not modification.
>
> Cc: Jiewen Yao <jiewen.yao@...>
> Cc: Jian J Wang <jian.j.wang@...>
> Cc: Chao Zhang <chao.b.zhang@...>
> Cc: Bret Barkelew <brbarkel@...>
> Signed-off-by: Bret Barkelew <brbarkel@...>
> ---
>  SecurityPkg/Library/AuthVariableLib/AuthService.c       | 22
> ++++++++++++++++----
>  SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf |  2 ++
>  2 files changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/SecurityPkg/Library/AuthVariableLib/AuthService.c
> b/SecurityPkg/Library/AuthVariableLib/AuthService.c
> index 2f60331f2c04..aca9a5620c28 100644
> --- a/SecurityPkg/Library/AuthVariableLib/AuthService.c
> +++ b/SecurityPkg/Library/AuthVariableLib/AuthService.c
> @@ -19,12 +19,16 @@
>    to verify the signature.
>
>
>
>  Copyright (c) 2009 - 2019, Intel Corporation. All rights reserved.<BR>
>
> +Copyright (c) Microsoft Corporation.
>
>  SPDX-License-Identifier: BSD-2-Clause-Patent
>
>
>
>  **/
>
>
>
>  #include "AuthServiceInternal.h"
>
>
>
> +#include <Protocol/VariablePolicy.h>
>
> +#include <Library/VariablePolicyLib.h>
>
> +
>
>  //
>
>  // Public Exponent of RSA Key.
>
>  //
>
> @@ -217,9 +221,12 @@ NeedPhysicallyPresent(
>    IN     EFI_GUID       *VendorGuid
>
>    )
>
>  {
>
> -  if ((CompareGuid (VendorGuid, &gEfiSecureBootEnableDisableGuid) &&
> (StrCmp (VariableName, EFI_SECURE_BOOT_ENABLE_NAME) == 0))
>
> -    || (CompareGuid (VendorGuid, &gEfiCustomModeEnableGuid) && (StrCmp
> (VariableName, EFI_CUSTOM_MODE_NAME) == 0))) {
>
> -    return TRUE;
>
> +  // If the VariablePolicy engine is disabled, allow deletion of any
> authenticated variables.
>
> +  if (IsVariablePolicyEnabled()) {
>
> +    if ((CompareGuid (VendorGuid, &gEfiSecureBootEnableDisableGuid) &&
> (StrCmp (VariableName, EFI_SECURE_BOOT_ENABLE_NAME) == 0))
>
> +      || (CompareGuid (VendorGuid, &gEfiCustomModeEnableGuid) && (StrCmp
> (VariableName, EFI_CUSTOM_MODE_NAME) == 0))) {
>
> +      return TRUE;
>
> +    }
>
>    }

[Jiewen] Looks good.

>
>
>    return FALSE;
>
> @@ -842,7 +849,8 @@ ProcessVariable (
>               &OrgVariableInfo
>
>               );
>
>
>
> -  if ((!EFI_ERROR (Status)) && IsDeleteAuthVariable
> (OrgVariableInfo.Attributes, Data, DataSize, Attributes) &&
> UserPhysicalPresent()) {
>
> +  // If the VariablePolicy engine is disabled, allow deletion of any
> authenticated variables.
>
> +  if ((!EFI_ERROR (Status)) && IsDeleteAuthVariable
> (OrgVariableInfo.Attributes, Data, DataSize, Attributes) &&
> (UserPhysicalPresent() || !IsVariablePolicyEnabled())) {
>

[Jiewen] Looks good.

>      //
>
>      // Allow the delete operation of common authenticated variable(AT or
> AW) at user physical presence.
>
>      //
>
> @@ -1960,6 +1968,12 @@ VerifyTimeBasedPayload (
>
>
>    CopyMem (Buffer, PayloadPtr, PayloadSize);
>
>
>
> +  // If the VariablePolicy engine is disabled, allow deletion of any
> authenticated variables.
>
> +  if (PayloadSize == 0 && (Attributes & EFI_VARIABLE_APPEND_WRITE) == 0 &&
> !IsVariablePolicyEnabled()) {
>
> +    VerifyStatus = TRUE;
>
> +    goto Exit;
>
> +  }
>

[Jiewen] I checked the programming context.
If we are going to skip the check, I feel the GetScratchBuffer() and CopyMem () may be avoided.
Also, I do not find any those data are used at Exit.

How about we move the check just after getting PayloadSize?
  //
  // Find out the new data payload which follows Pkcs7 SignedData directly.
  //
  PayloadPtr = SigData + SigDataSize;
  PayloadSize = DataSize - OFFSET_OF_AUTHINFO2_CERT_DATA - (UINTN) SigDataSize;
I hope it can make logic clearer.


One more thing is about below action at Exit.
Pkcs7FreeSigners (TopLevelCert);
Pkcs7FreeSigners (SignerCerts);

With new short path, we can come here with NULL point for Pkcs7FreeSigners().
I don't know the result if we pass a NULL pointer according to Pkcs7FreeSigners() API definition.
/**
  Wrap function to use free() to free allocated memory for certificates.
  If this interface is not supported, then ASSERT().
  @param[in]  Certs        Pointer to the certificates to be freed.
**/
VOID
EFIAPI
Pkcs7FreeSigners (
  IN  UINT8        *Certs
  );

I notice the current openssl version BaseCryptoLib implementation will check NULL and return.
We are safe in the default one. But I am not sure about other implementation.

I recommend we either document NULL pointer behavior in Pkcs7FreeSigners(), or add NULL pointer check at Exit to avoid calling Pkcs7FreeSigners().

With above two update, reviewed-by: Jiewen Yao <Jiewen.yao@...>


> +
>
>    if (AuthVarType == AuthVarTypePk) {
>
>      //
>
>      // Verify that the signature has been made with the current Platform
> Key (no chaining for PK).
>
> diff --git a/SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf
> b/SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf
> index 8d4ce14df494..8eadeebcebd7 100644
> --- a/SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf
> +++ b/SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf
> @@ -3,6 +3,7 @@
>  #
>
>  #  Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.<BR>
>
>  #  Copyright (c) 2018, ARM Limited. All rights reserved.<BR>
>
> +#  Copyright (c) Microsoft Corporation.
>
>  #
>
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
>
>  #
>
> @@ -41,6 +42,7 @@ [LibraryClasses]
>    MemoryAllocationLib
>
>    BaseCryptLib
>
>    PlatformSecureLib
>
> +  VariablePolicyLib
>
>
>
>  [Guids]
>
>    ## CONSUMES            ## Variable:L"SetupMode"
>
> --
> 2.28.0.windows.1
>
>
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
>
> View/Reply Online (#64723): https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F64723&amp;data=02%7C01%7Cbret.barkelew%40microsoft.com%7C8f1aee9f15a14900a3d508d859eb7222%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637358215630864971&amp;sdata=JpxigghyweSgGoxS3lF6P6giUqI6WkIkDfX6%2FTqcog4%3D&amp;reserved=0
> Mute This Topic: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.io%2Fmt%2F76468137%2F4905953&amp;data=02%7C01%7Cbret.barkelew%40microsoft.com%7C8f1aee9f15a14900a3d508d859eb7222%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637358215630864971&amp;sdata=FUAT8j4ic9AokVjXWRNAgC2GRTKg581rra%2BHcnJ%2F%2BBc%3D&amp;reserved=0
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Funsub&amp;data=02%7C01%7Cbret.barkelew%40microsoft.com%7C8f1aee9f15a14900a3d508d859eb7222%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637358215630864971&amp;sdata=kTY8I2s1WrJMc0r43wy0d1LwHJfnVe3WuYnx%2BCMuT4k%3D&amp;reserved=0
> [gaoliming@...]
> -=-=-=-=-=-=
>
>


 


Re: more development process failure [was: UefiPayloadPkg: Runtime MMCONF]

Laszlo Ersek
 

On 09/16/20 19:30, Dong, Guo wrote:

Hi Laszlo,

The patchset includes 3 patches, and all of them had been reviewed by package owners.
The patch submitter has a pull request https://github.com/tianocore/edk2/pull/885, I rebased the patch to latest master, and merged it by adding reviewed-by found from emails.
I also make sure it passed all the checks before I put "push" button there. then retrigger a new build with "push" button.

I am not sure what is missing. If there is any other requirements, should they be captured during code review or tool check?
- The description field of <https://github.com/tianocore/edk2/pull/932/>
is empty. It's difficult to tell where the patches come from -- where
they were posted and reviewed. A copy of the cover letter should have
been included here, plus preferably a link to the v5 mailing list thread
(the one that got merged in the end).

- It was not confirmed in the v5 mailing list thread that the series had
been merged. The confirmation should have included at least one of: (a)
the github PR link, (b) the git commit range. (Preferably: both.)

It's not the eventual git commits that I'm complaining about, but the
lack of communication with the community, and the lack of record for
posterity.

Myself, I used to consider github PRs a means merely for replacing our
earlier direct "git push" commands -- with a CI build + mergify. So, as
a maintainer, I would myself queue up several patch sets in a single
"batch" PR, add some links to BZs and the mailing list, and let it fly.
But then Mike told me this was really wrong, and we should clearly
associate any given PR with a specific patch set on the list.

This meant an *immense* workload increase for me, in particular because
I tend to merge patch sets for *other* people and subsystems too (after
they pass review), that is, for such subsystems that I do not
co-maintain. In particular during the feature freeze periods.

So what really rubs me the wrong way is that, if I am expected to keep
all of this meta-data nice and tidy, why aren't some other maintainers?
It's a double standard.

I can live with either *all of us* ignoring PR tidiness, or *all of us*
doing our best to keep everything nicely cross-referenced.

But right now I spend significant time and effort on keeping
communication and records complete and clean in *all three of* bugzilla,
github, and mailing list, whereas a good subset of the maintainers
couldn't care less in *either* of those communication channels.

For your reference, here's a random PR I submitted and merged for others:

https://github.com/tianocore/edk2/pull/904

Observe in PR#904:

- title carries cover letter subject
- description carries cover letter body
- description has a pointer to the BZ, and a link to the cover letter in
the mailing list archive (two links in fact, in different archives)

And then here's my report back on the list:

https://edk2.groups.io/g/devel/message/64644

And my BZ comment to the same effect (also closing the BZ as
RESOLVED|FIXED):

https://bugzilla.tianocore.org/show_bug.cgi?id=2376#c9
https://edk2.groups.io/g/bugs/message/12777


I don't insist on the particular information content of github PRs, as
-- at this stage -- they really are not more than just a way to set off
CI, before pushing/merging a series.

What I do insist on is that all of us maintainers (people with
permission to set the "push" label) be subject to the same expectations
when it comes to creating pull requests.

(Please note also that I absolutely don't need a BZ for every
contribution. My request is only that *if* there is a BZ, then handle it
thoroughly.)

Laszlo



Thanks,
Guo

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
Ersek
Sent: Wednesday, September 16, 2020 1:57 AM
To: Dong, Guo <guo.dong@intel.com>
Cc: devel@edk2.groups.io; marcello.bauer@9elements.com; Kinney, Michael D
<michael.d.kinney@intel.com>; Leif Lindholm (Nuvia address)
<leif@nuviainc.com>; Doran, Mark <mark.doran@intel.com>; Andrew Fish
<afish@apple.com>; Guptha, Soumya K <soumya.k.guptha@intel.com>
Subject: [edk2-devel] more development process failure [was: UefiPayloadPkg:
Runtime MMCONF]

Guo,

On 08/18/20 10:24, Marcello Sylvester Bauer wrote:
Support arbitrary platforms with different or even no MMCONF space.
Fixes crash on platforms not exposing 256 buses.

Tested on:
* AMD Stoney Ridge

Branch: https://github.com/9elements/edk2-1/tree/UefiPayloadPkg-
MMCONF
PR: https://github.com/tianocore/edk2/pull/885

v5:
* MdePkg
- support variable size MMCONF in all PciExpressLibs
- use (UINTX)-1 as return values for invalid Pci addresses
Okay, so we got more of the same development process violations here, as
I've just reported at <https://edk2.groups.io/g/devel/message/65313>.

See this new pull request:

https://github.com/tianocore/edk2/pull/932/

"No description provided."

You should be embarrassed.

Laszlo





update edk2-platforms Vlv2TbltDevicePkg

Kilian Kegel
 

Hi Mike,

 

recently I have updated my CdePkg project from https://github.com/tianocore/edk2-staging.git.

 

It seems that the ValleyView Package (MinnowBoard) in edk2-platforms/Platform/Intel/Vlv2TbltDevicePkg

needs an update to support latest FmpDevicePkg from edk2-stable202008.

 

diff --git a/Platform/Intel/Vlv2TbltDevicePkg/FmpBlueSampleDevice.dsc b/Platform/Intel/Vlv2TbltDevicePkg/FmpBlueSampleDevice.dsc

index 3bd9f150b3..1bf943cf0e 100644

--- a/Platform/Intel/Vlv2TbltDevicePkg/FmpBlueSampleDevice.dsc

+++ b/Platform/Intel/Vlv2TbltDevicePkg/FmpBlueSampleDevice.dsc

@@ -52,4 +52,7 @@

       # Device specific library that processes a capsule and updates the FW storage device

       #

       FmpDeviceLib|Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLibSample/FmpDeviceLib.inf

+      FmpDependencyLib|FmpDevicePkg\Library\FmpDependencyLib\FmpDependencyLib.inf

+      FmpDependencyCheckLib|FmpDevicePkg\Library\FmpDependencyCheckLibNull\FmpDependencyCheckLibNull.inf

+      FmpDependencyDeviceLib|FmpDevicePkg\Library\FmpDependencyDeviceLibNull\FmpDependencyDeviceLibNull.inf

   }

diff --git a/Platform/Intel/Vlv2TbltDevicePkg/FmpGreenSampleDevice.dsc b/Platform/Intel/Vlv2TbltDevicePkg/FmpGreenSampleDevice.dsc

index 61bdd36a96..0e6c10e23f 100644

--- a/Platform/Intel/Vlv2TbltDevicePkg/FmpGreenSampleDevice.dsc

+++ b/Platform/Intel/Vlv2TbltDevicePkg/FmpGreenSampleDevice.dsc

@@ -52,4 +52,7 @@

       # Device specific library that processes a capsule and updates the FW storage device

       #

       FmpDeviceLib|Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLibSample/FmpDeviceLib.inf

+      FmpDependencyLib|FmpDevicePkg\Library\FmpDependencyLib\FmpDependencyLib.inf

+      FmpDependencyCheckLib|FmpDevicePkg\Library\FmpDependencyCheckLibNull\FmpDependencyCheckLibNull.inf

+      FmpDependencyDeviceLib|FmpDevicePkg\Library\FmpDependencyDeviceLibNull\FmpDependencyDeviceLibNull.inf

   }

diff --git a/Platform/Intel/Vlv2TbltDevicePkg/FmpMinnowMaxSystem.dsc b/Platform/Intel/Vlv2TbltDevicePkg/FmpMinnowMaxSystem.dsc

index 304519b294..eea73c0f06 100644

--- a/Platform/Intel/Vlv2TbltDevicePkg/FmpMinnowMaxSystem.dsc

+++ b/Platform/Intel/Vlv2TbltDevicePkg/FmpMinnowMaxSystem.dsc

@@ -56,4 +56,7 @@

       # Device specific library that processes a capsule and updates the FW storage device

       #

       FmpDeviceLib|Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLib/FmpDeviceLib.inf

+      FmpDependencyLib|FmpDevicePkg\Library\FmpDependencyLib\FmpDependencyLib.inf

+      FmpDependencyCheckLib|FmpDevicePkg\Library\FmpDependencyCheckLibNull\FmpDependencyCheckLibNull.inf

+      FmpDependencyDeviceLib|FmpDevicePkg\Library\FmpDependencyDeviceLibNull\FmpDependencyDeviceLibNull.inf

   }

diff --git a/Platform/Intel/Vlv2TbltDevicePkg/FmpRedSampleDevice.dsc b/Platform/Intel/Vlv2TbltDevicePkg/FmpRedSampleDevice.dsc

index 59851f2b41..d37974f9d4 100644

--- a/Platform/Intel/Vlv2TbltDevicePkg/FmpRedSampleDevice.dsc

+++ b/Platform/Intel/Vlv2TbltDevicePkg/FmpRedSampleDevice.dsc

@@ -52,4 +52,7 @@

       # Device specific library that processes a capsule and updates the FW storage device

       #

       FmpDeviceLib|Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLibSample/FmpDeviceLib.inf

+      FmpDependencyLib|FmpDevicePkg\Library\FmpDependencyLib\FmpDependencyLib.inf

+      FmpDependencyCheckLib|FmpDevicePkg\Library\FmpDependencyCheckLibNull\FmpDependencyCheckLibNull.inf

+      FmpDependencyDeviceLib|FmpDevicePkg\Library\FmpDependencyDeviceLibNull\FmpDependencyDeviceLibNull.inf

   }

 

Best Reagrds,

Kilian


Re: more development process failure [was: UefiPayloadPkg: Runtime MMCONF]

Guo Dong
 

Hi Laszlo,

The patchset includes 3 patches, and all of them had been reviewed by package owners.
The patch submitter has a pull request https://github.com/tianocore/edk2/pull/885, I rebased the patch to latest master, and merged it by adding reviewed-by found from emails.
I also make sure it passed all the checks before I put "push" button there. then retrigger a new build with "push" button.

I am not sure what is missing. If there is any other requirements, should they be captured during code review or tool check?

Thanks,
Guo

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
Ersek
Sent: Wednesday, September 16, 2020 1:57 AM
To: Dong, Guo <guo.dong@intel.com>
Cc: devel@edk2.groups.io; marcello.bauer@9elements.com; Kinney, Michael D
<michael.d.kinney@intel.com>; Leif Lindholm (Nuvia address)
<leif@nuviainc.com>; Doran, Mark <mark.doran@intel.com>; Andrew Fish
<afish@apple.com>; Guptha, Soumya K <soumya.k.guptha@intel.com>
Subject: [edk2-devel] more development process failure [was: UefiPayloadPkg:
Runtime MMCONF]

Guo,

On 08/18/20 10:24, Marcello Sylvester Bauer wrote:
Support arbitrary platforms with different or even no MMCONF space.
Fixes crash on platforms not exposing 256 buses.

Tested on:
* AMD Stoney Ridge

Branch: https://github.com/9elements/edk2-1/tree/UefiPayloadPkg-
MMCONF
PR: https://github.com/tianocore/edk2/pull/885

v5:
* MdePkg
- support variable size MMCONF in all PciExpressLibs
- use (UINTX)-1 as return values for invalid Pci addresses
Okay, so we got more of the same development process violations here, as
I've just reported at <https://edk2.groups.io/g/devel/message/65313>.

See this new pull request:

https://github.com/tianocore/edk2/pull/932/

"No description provided."

You should be embarrassed.

Laszlo





Re: [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface

Laszlo Ersek
 

On 09/16/20 16:17, Yao, Jiewen wrote:
Thank you Laszlo.
You forced me to read the code again and more carefully. :-)

I believe I understand why you use NULL to free the context now - to support error path.

First, I did have some new thought for EVP API. Let’s consider 3 cases, 1) Hash all data one time, 2) Hash update data multiple times, 3) Error during update.

A. In current design, the API sequence is:
1) EvpMdHashAll (Digest)
2) EvpMdInit, EvpMdUpdate, EvpMdUpdate, ... , EvpMdFinal (Digest)
3) EvpMdInit, EvpMdUpdate->ERROR, EvpMdFinal (NULL)

B. We can auto free context in EvpMdUpdate in error path - the API sequence is:
1) EvpMdHashAll (Digest)
2) EvpMdInit, EvpMdUpdate, EvpMdUpdate, ... , EvpMdFinal (Digest)
3) EvpMdInit, EvpMdUpdate->ERROR
I don't like "B" because in the loop body where you call EvpMdUpdate(),
you may encounter an error from a different source than EvpMdUpdate()
itself. For example, you may be reading data from the network or a disk
file. If fetching the next chunk fails, we'd still want to clean up the
EVP MD context. Therefore, if EvpMdUpdate() would itself invalidate the
context, if it failed, then that would *complicate* the error path. (=
Clean up the context after the loop body *only* if something different
from EvpMdUpdate() failed.)


C: We can use New/Free style - similar to HMAC
1) EvpMdHashAll (Digest)
2) EvpMdNew, EvpMdInit, EvpMdUpdate, EvpMdUpdate, ... , EvpMdFinal (Digest), EvpMdFree
3) EvpMdNew, EvpMdInit, EvpMdUpdate->ERROR, EvpMdFree
Yes, this was the first pattern, the one that caused me to say "please
don't do this". In this pattern, the context may exist between "New" and
"Init", and also between "Final" and "Free". Those two states are
useless and only good for causing confusion.

For example, are you allowed to Duplicate a context in those states?
It's best to prevent even the *asking* of that question.


I checked below APIs:
1) openssl (EVP_MD_CTX_new, EVP_DigestInit_ex, EVP_DigestUpdate, EVP_DigestFinal_ex, EVP_MD_CTX_free)
2) mbedtls (mbedtls_sha256_init, mbedtls_sha256_starts_ret, mbedtls_sha256_update_ret, mbedtls_sha256_finish_ret, mbedtls_sha256_free)
3) BaseCryptoLib HMAC (HmacSha256New, HmacSha256SetKey, HmacSha256Update, HmacSha256Final, HmacSha256Free)

All APIs include free function to free the context, I don’t think it is a bad idea to have EvpMdFree() API.
I would like to propose option - C.
- I cannot comment on mbedtls (2).

- I think the current BaseCryptLib HMAC APIs (3) are not great, and we
should use this opportunity to improve upon them.

- Regarding openssl (1), as I understand it, the situation is different
from edk2. In OpenSSL, EVP_MD_CTX is *not* an opaque type (expressed in
C language terms, it is not an "incomplete" type, but a "complete"
type). Therefore, an expression such as sizeof(EVP_MD_CTX) works.

The consequence is that OpenSSL code itself can use the following style:

void func(void)
{
EVP_MD_CTX ctx;

EVP_DigestInit_ex(&ctx, ...);
...
EVP_DigestFinal_ex(&ctx, ...)
}

In other words, OpenSSL-native code is permitted to know the internals
of EVP_MD_CTX, therefore the OpenSSL-internal code may allocate
EVP_MD_CTX with means that are *different* from EVP_MD_CTX_new() and
EVP_MD_CTX_free().

The caller of (Init, Final) may use (New, Free) for memory management,
or may use something completely different (for example, local variables).

Therefore, offering the (Init, Final) APIs separately from (New, Free)
is meaningful.

But the situation in edk2 -- or any other OpenSSL *consumer* -- is
different. In edk2, EVP_MD_CTX is an opaque type -- in C language terms:
an "incomplete" type. OpenSSL deliberately hides the internals of
EVP_MD_CTX.

See "CryptoPkg/Library/OpensslLib/openssl/include/openssl/ossl_typ.h":

typedef struct evp_md_ctx_st EVP_MD_CTX;
and the "evp_md_ctx_st" structure type is only defined in
"CryptoPkg/Library/OpensslLib/openssl/crypto/evp/evp_local.h", whose
contents -- I think! -- OpenSSL client code cannot, or *should not*,
refer to.

This means that OpenSSL consumer code can *only* rely on
EVP_MD_CTX_new() and EVP_MD_CTX_free(), for allocating and releasing the
context, respectively. "sizeof(EVP_MD_CTX)" is not permitted, and
defining a local or global variable of type EVP_MD_CTX is also not valid.

This means that the only reason for separating (Init, Final) from (New,
Free) falls away, in OpenSSL consumer code. In OpenSSL consumer code,
there is no reason to keep *both* pairs of APIs.


Please note that, this (very prudent) information hiding / encapsulation
in OpenSSL used to be violated by edk2 in the past, intentionally.
That's what the HmacXxxGetContextSize() APIs were about -- those APIs
forcefully leaked the context sizes to client code, so that client code
in edk2 could perform its own allocation.

But in <https://bugzilla.tianocore.org/show_bug.cgi?id=1792>, we finally
eliminated HmacXxxGetContextSize(). As a part of that, we also
simplified the HMAC API -- note that Init was replaced by SetKey. And
because we were reworking an existent API, I didn't propose very
intrusive changes -- I didn't propose fusing Final and Free, for example.

Now, the EVP MD APIs are just being introduced to edk2. So we have a
chance at getting them right, and making them minimal.

Second, I believe EVP is just something in openssl. It does not mean that we *have to* design API to follow openssl.
After rethink the size impact, I do have concern to merge all Hash implementation together in one function.
It might means nothing if the crypto library is based upon openssl.
But if the cryptolib implementation is based upon other crypto, such as Intel IPP (https://github.com/slimbootloader/slimbootloader/tree/master/BootloaderCommonPkg/Library/IppCryptoLib) or mbedTls (https://github.com/jyao1/edk2/tree/DeviceSecurity/CryptoMbedTlsPkg), then we can NOT get size benefit by separating the hash algorithm.


I would like to propose separate EvpMdxxx.
EvpMdNew -> Sha256New, Sha384New
EvpMdInit -> Sha256Init, Sha384Init
EvpMdUpdate -> Sha256Update, Sha384Update
EvpMdFinal -> Sha256Final, Sha384Final
EvpMdFree -> Sha256Free, Sha384Free
I have no comment on this.

We can do similar thing with Hmac, to deprecate Sha256GetContextSize() API,
Yes, good idea.

and replace caller with Sha256New() / Sha384Free()
Indeed.

But then -- there's no reason left for keeping (New, Free) separate from
(Init, Final).

Thanks
Laszlo


[PATCH v2] EmulatorPkg: Enable support for Secure Boot

Wadhawan, Divneil R
 

SECURE_BOOT_ENABLE feature flag is introduced to enable Secure Boot.

The following gets enabled with this patch:

o Secure Boot Menu in "Device Manager" for enrolling keys

o Storage space for Authenticated Variables

o Authenticated execution of 3rd party images

 

Signed-off-by: Divneil Rai Wadhawan <divneil.r.wadhawan@...>

---

EmulatorPkg/EmulatorPkg.dsc | 37 +++++++++++++++++++++++++++++++++++--

EmulatorPkg/EmulatorPkg.fdf | 14 ++++++++++++++

2 files changed, 49 insertions(+), 2 deletions(-)

 

diff --git a/EmulatorPkg/EmulatorPkg.dsc b/EmulatorPkg/EmulatorPkg.dsc

index 86a6271735..c6e25c745e 100644

--- a/EmulatorPkg/EmulatorPkg.dsc

+++ b/EmulatorPkg/EmulatorPkg.dsc

@@ -32,6 +32,7 @@

   DEFINE NETWORK_TLS_ENABLE       = FALSE

   DEFINE NETWORK_HTTP_BOOT_ENABLE = FALSE

   DEFINE NETWORK_ISCSI_ENABLE     = FALSE

+  DEFINE SECURE_BOOT_ENABLE       = FALSE

 [SkuIds]

   0|DEFAULT

@@ -106,12 +107,20 @@

   LockBoxLib|MdeModulePkg/Library/LockBoxNullLib/LockBoxNullLib.inf

   CpuExceptionHandlerLib|MdeModulePkg/Library/CpuExceptionHandlerLibNull/CpuExceptionHandlerLibNull.inf

   TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf

-  AuthVariableLib|MdeModulePkg/Library/AuthVariableLibNull/AuthVariableLibNull.inf

   VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf

   SortLib|MdeModulePkg/Library/BaseSortLib/BaseSortLib.inf

   ShellLib|ShellPkg/Library/UefiShellLib/UefiShellLib.inf

   FileHandleLib|MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf

+!if $(SECURE_BOOT_ENABLE) == TRUE

+  IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf

+  OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf

+  PlatformSecureLib|SecurityPkg/Library/PlatformSecureLibNull/PlatformSecureLibNull.inf

+  AuthVariableLib|SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf

+!else

+  AuthVariableLib|MdeModulePkg/Library/AuthVariableLibNull/AuthVariableLibNull.inf

+!endif

+

[LibraryClasses.common.SEC]

   PeiServicesLib|EmulatorPkg/Library/SecPeiServicesLib/SecPeiServicesLib.inf

   PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf

@@ -162,6 +171,16 @@

   TimerLib|EmulatorPkg/Library/DxeCoreTimerLib/DxeCoreTimerLib.inf

  EmuThunkLib|EmulatorPkg/Library/DxeEmuLib/DxeEmuLib.inf

+[LibraryClasses.common.DXE_DRIVER, LibraryClasses.common.UEFI_DRIVER, LibraryClasses.common.UEFI_APPLICATION]

+!if $(SECURE_BOOT_ENABLE) == TRUE

+  BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf

+!endif

+

+[LibraryClasses.common.DXE_RUNTIME_DRIVER]

+!if $(SECURE_BOOT_ENABLE) == TRUE

+  BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf

+!endif

+

[LibraryClasses.common.DXE_RUNTIME_DRIVER, LibraryClasses.common.UEFI_DRIVER, LibraryClasses.common.DXE_DRIVER, LibraryClasses.common.UEFI_APPLICATION]

   HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf

   PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf

@@ -190,6 +209,10 @@

   gEmulatorPkgTokenSpaceGuid.PcdEmuFirmwareFdSize|0x002a0000

   gEmulatorPkgTokenSpaceGuid.PcdEmuFirmwareBlockSize|0x10000

   gEmulatorPkgTokenSpaceGuid.PcdEmuFirmwareVolume|L"../FV/FV_RECOVERY.fd"

+!if $(SECURE_BOOT_ENABLE) == TRUE

+  gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800

+  gEfiSecurityPkgTokenSpaceGuid.PcdUserPhysicalPresence|TRUE

+!endif

   gEmulatorPkgTokenSpaceGuid.PcdEmuMemorySize|L"64!64"

@@ -306,7 +329,14 @@

   EmulatorPkg/ResetRuntimeDxe/Reset.inf

   MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf

   EmulatorPkg/FvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf

-  MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf

+

+  MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf {

+    <LibraryClasses>

+!if $(SECURE_BOOT_ENABLE) == TRUE

+      NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf

+!endif

+  }

+

   MdeModulePkg/Universal/EbcDxe/EbcDxe.inf

   MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf

   EmulatorPkg/EmuThunkDxe/EmuThunk.inf

@@ -315,6 +345,9 @@

   EmulatorPkg/PlatformSmbiosDxe/PlatformSmbiosDxe.inf

   EmulatorPkg/TimerDxe/Timer.inf

+!if $(SECURE_BOOT_ENABLE) == TRUE

+  SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf

+!endif

   MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {

     <LibraryClasses>

diff --git a/EmulatorPkg/EmulatorPkg.fdf b/EmulatorPkg/EmulatorPkg.fdf

index 295f6f1db8..b256aa9397 100644

--- a/EmulatorPkg/EmulatorPkg.fdf

+++ b/EmulatorPkg/EmulatorPkg.fdf

@@ -46,10 +46,17 @@ DATA = {

   # Blockmap[1]: End

   0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,

   ## This is the VARIABLE_STORE_HEADER

+!if $(SECURE_BOOT_ENABLE) == FALSE

   #Signature: gEfiVariableGuid =

   #  { 0xddcf3616, 0x3275, 0x4164, { 0x98, 0xb6, 0xfe, 0x85, 0x70, 0x7f, 0xfe, 0x7d }}

   0x16, 0x36, 0xcf, 0xdd, 0x75, 0x32, 0x64, 0x41,

   0x98, 0xb6, 0xfe, 0x85, 0x70, 0x7f, 0xfe, 0x7d,

+!else

+  # Signature: gEfiAuthenticatedVariableGuid =

+  #  { 0xaaf32c78, 0x947b, 0x439a, { 0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92 }}

+  0x78, 0x2c, 0xf3, 0xaa, 0x7b, 0x94, 0x9a, 0x43,

+  0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92,

+!endif

   #Size: 0xc000 (gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize) - 0x48 (size of EFI_FIRMWARE_VOLUME_HEADER) = 0xBFB8

   # This can speed up the Variable Dispatch a bit.

   0xB8, 0xBF, 0x00, 0x00,

@@ -186,6 +193,13 @@ INF  RuleOverride = UI MdeModulePkg/Application/UiApp/UiApp.inf

INF  MdeModulePkg/Application/BootManagerMenuApp/BootManagerMenuApp.inf

INF  MdeModulePkg/Universal/DriverSampleDxe/DriverSampleDxe.inf

+#

+# Secure Boot Key Enroll

+#

+!if $(SECURE_BOOT_ENABLE) == TRUE

+INF SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf

+!endif

+

#

# Network stack drivers

#

--

2.24.1.windows.2


Re: [PATCH] EmulatorPkg: Enable support for Secure Boot

Wadhawan, Divneil R
 

Hi Ray,

I have fixed the review comments.
I will push a v2 of the patch.

Regards,
Divneil

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Wednesday, September 16, 2020 2:16 PM
To: gaoliming <gaoliming@byosoft.com.cn>; devel@edk2.groups.io; Wadhawan, Divneil R <divneil.r.wadhawan@intel.com>
Cc: 'Andrew Fish' <afish@apple.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [PATCH] EmulatorPkg: Enable support for Secure Boot

1. I prefer to not duplicate the HobLib/PcdLib/.../TimerLib in DSC for runtime drivers just because they need to link a different CryptLib.
2. Why the DSC requires UEFI_DRIVER and UEFI_APPLICATION modules use RuntimeCryptLib? It should cause build failures because RuntimeCryptLib only can support DXE_RUNTIME_DRIVER.
3. SecurityStubDxe is already in DSC file. Why did you add another one?

Thanks,
Ray

-----Original Message-----
From: gaoliming <gaoliming@byosoft.com.cn>
Sent: Wednesday, September 16, 2020 9:49 AM
To: devel@edk2.groups.io; Wadhawan, Divneil R
<divneil.r.wadhawan@intel.com>
Cc: Ni, Ray <ray.ni@intel.com>; 'Andrew Fish' <afish@apple.com>;
Justen, Jordan L <jordan.l.justen@intel.com>; Kinney, Michael D
<michael.d.kinney@intel.com>
Subject: 回复: [edk2-devel] [PATCH] EmulatorPkg: Enable support for
Secure Boot

I think SECURE_BOOT_ENABLE flag is fine. It controls more security
related features. And, this flag is also used in OVMF DSC.

So, this change is good to me. Reviewed-by: Liming Gao
<gaoliming@byosoft.com.cn>

Ray, Andrew: have you any other comment?

Thanks
Liming
-----邮件原件-----
发件人: bounce+27952+65013+4905953+8761045@groups.io
<bounce+27952+65013+4905953+8761045@groups.io> 代表 Wadhawan, Divneil
R
发送时间: 2020年9月4日 2:17
收件人: devel@edk2.groups.io
抄送: Ni, Ray <ray.ni@intel.com>; Andrew Fish (afish@apple.com)
<afish@apple.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
Kinney, Michael D <michael.d.kinney@intel.com>; Wadhawan, Divneil R
<divneil.r.wadhawan@intel.com>
主题: [edk2-devel] [PATCH] EmulatorPkg: Enable support for Secure Boot

SECURE_BOOT_ENABLE feature flag is introduced to enable Secure Boot.
The following gets enabled with this patch:
o Secure Boot Menu in "Device Manager" for enrolling keys o Storage
space for Authenticated Variables o Authenticated execution of 3rd
party images

Signed-off-by: Divneil Rai Wadhawan <divneil.r.wadhawan@intel.com>
---
EmulatorPkg/EmulatorPkg.dsc | 40
+++++++++++++++++++++++++++++++++++--
EmulatorPkg/EmulatorPkg.fdf | 21 +++++++++++++++----
2 files changed, 55 insertions(+), 6 deletions(-)

diff --git a/EmulatorPkg/EmulatorPkg.dsc
b/EmulatorPkg/EmulatorPkg.dsc index 86a6271735..6591c3e824 100644
--- a/EmulatorPkg/EmulatorPkg.dsc
+++ b/EmulatorPkg/EmulatorPkg.dsc
@@ -32,6 +32,7 @@
DEFINE NETWORK_TLS_ENABLE = FALSE
DEFINE NETWORK_HTTP_BOOT_ENABLE = FALSE
DEFINE NETWORK_ISCSI_ENABLE = FALSE
+ DEFINE SECURE_BOOT_ENABLE = FALSE

[SkuIds]
0|DEFAULT
@@ -106,12 +107,20 @@
LockBoxLib|MdeModulePkg/Library/LockBoxNullLib/LockBoxNullLib.inf

CpuExceptionHandlerLib|MdeModulePkg/Library/CpuExceptionHandlerLibNu
ll/CpuExceptionHandlerLibNull.inf

TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/Tpm
MeasurementLibNull.inf
-
AuthVariableLib|MdeModulePkg/Library/AuthVariableLibNull/AuthVariabl
AuthVariableLib|eLi
bNull.inf
VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf
SortLib|MdeModulePkg/Library/BaseSortLib/BaseSortLib.inf
ShellLib|ShellPkg/Library/UefiShellLib/UefiShellLib.inf

FileHandleLib|MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf

+ !if $(SECURE_BOOT_ENABLE) == TRUE
+ IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
+ OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
+
PlatformSecureLib|SecurityPkg/Library/PlatformSecureLibNull/Platform
PlatformSecureLib|Secur
eLibNull.inf
+
AuthVariableLib|SecurityPkg/Library/AuthVariableLib/AuthVariableLib.
AuthVariableLib|inf
+ !else
+
AuthVariableLib|MdeModulePkg/Library/AuthVariableLibNull/AuthVariabl
AuthVariableLib|eLi
bNull.inf
+ !endif
+
[LibraryClasses.common.SEC]

PeiServicesLib|EmulatorPkg/Library/SecPeiServicesLib/SecPeiServicesL
PeiServicesLib|ib.inf
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
@@ -162,7 +171,20 @@
TimerLib|EmulatorPkg/Library/DxeCoreTimerLib/DxeCoreTimerLib.inf
EmuThunkLib|EmulatorPkg/Library/DxeEmuLib/DxeEmuLib.inf

-[LibraryClasses.common.DXE_RUNTIME_DRIVER,
LibraryClasses.common.UEFI_DRIVER, LibraryClasses.common.DXE_DRIVER,
LibraryClasses.common.UEFI_APPLICATION]
+[LibraryClasses.common.DXE_DRIVER]
+ HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
+ PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
+
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemor
yAllocationLib.inf
+
ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeR
eportStatusCodeLib.inf
+ EmuThunkLib|EmulatorPkg/Library/DxeEmuLib/DxeEmuLib.inf
+
PeCoffExtraActionLib|EmulatorPkg/Library/DxeEmuPeCoffExtraActionLib/
PeCoffExtraActionLib|Dxe
EmuPeCoffExtraActionLib.inf
+
ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeR
eportStatusCodeLib.inf
+ TimerLib|EmulatorPkg/Library/DxeTimerLib/DxeTimerLib.inf
+ !if $(SECURE_BOOT_ENABLE) == TRUE
+ BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
+ !endif
+
+[LibraryClasses.common.DXE_RUNTIME_DRIVER,
LibraryClasses.common.UEFI_DRIVER,
LibraryClasses.common.UEFI_APPLICATION]
HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf

MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemor
yAllocationLib.inf
@@ -171,6 +193,9 @@

PeCoffExtraActionLib|EmulatorPkg/Library/DxeEmuPeCoffExtraActionLib/
PeCoffExtraActionLib|Dxe
EmuPeCoffExtraActionLib.inf

ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeR
eportStatusCodeLib.inf
TimerLib|EmulatorPkg/Library/DxeTimerLib/DxeTimerLib.inf
+ !if $(SECURE_BOOT_ENABLE) == TRUE
+ BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
+ !endif

[PcdsFeatureFlag]
gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode|FALSE
@@ -190,6 +215,10 @@
gEmulatorPkgTokenSpaceGuid.PcdEmuFirmwareFdSize|0x002a0000
gEmulatorPkgTokenSpaceGuid.PcdEmuFirmwareBlockSize|0x10000

gEmulatorPkgTokenSpaceGuid.PcdEmuFirmwareVolume|L"../FV/FV_RECOVE
RY.fd"
+ !if $(SECURE_BOOT_ENABLE) == TRUE
+ gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
+ gEfiSecurityPkgTokenSpaceGuid.PcdUserPhysicalPresence|TRUE
+ !endif

gEmulatorPkgTokenSpaceGuid.PcdEmuMemorySize|L"64!64"

@@ -315,6 +344,13 @@
EmulatorPkg/PlatformSmbiosDxe/PlatformSmbiosDxe.inf
EmulatorPkg/TimerDxe/Timer.inf

+ !if $(SECURE_BOOT_ENABLE) == TRUE
+
SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
igD
xe.inf
+ MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf {
+ <LibraryClasses>
+
NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
NULL|ib.i
nf
+ }
+ !endif

MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
{
<LibraryClasses>
diff --git a/EmulatorPkg/EmulatorPkg.fdf
b/EmulatorPkg/EmulatorPkg.fdf index 295f6f1db8..4bf592e778 100644
--- a/EmulatorPkg/EmulatorPkg.fdf
+++ b/EmulatorPkg/EmulatorPkg.fdf
@@ -46,10 +46,16 @@ DATA = {
# Blockmap[1]: End
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
## This is the VARIABLE_STORE_HEADER
- #Signature: gEfiVariableGuid =
- # { 0xddcf3616, 0x3275, 0x4164, { 0x98, 0xb6, 0xfe, 0x85, 0x70,
0x7f, 0xfe, 0x7d }}
- 0x16, 0x36, 0xcf, 0xdd, 0x75, 0x32, 0x64, 0x41,
- 0x98, 0xb6, 0xfe, 0x85, 0x70, 0x7f, 0xfe, 0x7d,
+ !if $(SECURE_BOOT_ENABLE) == FALSE
+ #Signature: gEfiVariableGuid =
+ # { 0xddcf3616, 0x3275, 0x4164, { 0x98, 0xb6, 0xfe, 0x85,
+ 0x70,
0x7f,
0xfe, 0x7d }}
+ 0x16, 0x36, 0xcf, 0xdd, 0x75, 0x32, 0x64, 0x41,
+ 0x98, 0xb6, 0xfe, 0x85, 0x70, 0x7f, 0xfe, 0x7d, !else
+ # Signature: gEfiAuthenticatedVariableGuid = { 0xaaf32c78,
+ 0x947b,
0x439a, { 0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92 } }
+ 0x78, 0x2c, 0xf3, 0xaa, 0x7b, 0x94, 0x9a, 0x43,
+ 0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92, !endif
#Size: 0xc000
(gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize) -
0x48 (size of EFI_FIRMWARE_VOLUME_HEADER) = 0xBFB8
# This can speed up the Variable Dispatch a bit.
0xB8, 0xBF, 0x00, 0x00,
@@ -186,6 +192,13 @@ INF RuleOverride = UI
MdeModulePkg/Application/UiApp/UiApp.inf
INF
MdeModulePkg/Application/BootManagerMenuApp/BootManagerMenuApp.i
nf
INF MdeModulePkg/Universal/DriverSampleDxe/DriverSampleDxe.inf

+#
+# Secure Boot Key Enroll
+#
+!if $(SECURE_BOOT_ENABLE) == TRUE
+INF
SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
igD
xe.inf
+!endif
+
#
# Network stack drivers
#
--
2.24.1.windows.2


Re: [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface

Laszlo Ersek
 

On 09/16/20 14:56, Yao, Jiewen wrote:
I try to understand what is the use case to EvpMdFinal without DigestValue and only Free the EvpMdContext.
Error paths. You got some hashing going, detect an error condition
(related to the hashing itself, or to something else), and want to abort
/ clean up the hashing. You need to release the context, but don't care
about the digest calculated thus far.

A normal usage will be:
1) EvpMdHashAll (Digest) - get digest.
2) EvpMdInit, EvpMdUpdate, EvpMdUpdate, ... , EvpMdFinal (Digest) - get digest

Why we need support the case to use EvpMdFinal (NULL) in the end to free context?
We don't strictly need it. In the above (error path) use case, we can
still pass a pointer to an actual digest-receiving object, and then go
on to ignore the contents of that object. Passing NULL in that case
improves readability though.

Thanks
Laszlo



Thank you
Yao Jiewen

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
Sent: Wednesday, September 16, 2020 8:40 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io; Zurcher,
Christopher J <christopher.j.zurcher@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>
Subject: Re: [edk2-devel] [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP
(Envelope) Digest interface

On 09/16/20 14:07, Yao, Jiewen wrote:
I overlooked the behavior that NULL DigestValue will take side effect to free
Context.

Hi Christopher
May I understand why we need free the context?
This does not align with other existing HASH or HMAC functions.
I requested that.

It makes no sense to keep a finalized context.

In other words, it makes no sense to separate finalization from freeing.
A context that has been finalized is unusable for anything except freeing.

Thanks
Laszlo


Thank you
Yao Jiewen

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao,
Jiewen
Sent: Wednesday, September 16, 2020 7:45 PM
To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Zurcher,
Christopher J <christopher.j.zurcher@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
<xiaoyux.lu@intel.com>
Subject: Re: [edk2-devel] [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP
(Envelope) Digest interface

Hi Laszlo
I disagree to put OPTIONAL for DigestValue, just because NULL is checked.
If we need follow this, then we need add OPTIONAL to almost all pointer,
which
is unnecessary.

+BOOLEAN
+EFIAPI
+EvpMdFinal (
+ IN OUT VOID *EvpMdContext,
+ OUT UINT8 *DigestValue
(3) DigestValue should be marked OPTIONAL in my opinion, as NULL is
deliberately permitted (for just freeing the context).




-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com>
Sent: Wednesday, September 16, 2020 7:07 PM
To: devel@edk2.groups.io; Zurcher, Christopher J
<christopher.j.zurcher@intel.com>
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
<jian.j.wang@intel.com>;
Lu, XiaoyuX <xiaoyux.lu@intel.com>
Subject: Re: [edk2-devel] [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP
(Envelope) Digest interface

Hello Christopher,

On 09/16/20 02:59, Zurcher, Christopher J wrote:
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2545

The EVP interface should be used in place of discrete digest function
calls.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
Signed-off-by: Christopher J Zurcher <christopher.j.zurcher@intel.com>
---
CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf | 1 +
CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf | 1 +
CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf | 1 +
CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf | 1 +
CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf | 1 +
CryptoPkg/Include/Library/BaseCryptLib.h | 129 ++++++++++
CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c | 257
++++++++++++++++++++
CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c | 128
++++++++++
CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c | 128
++++++++++
9 files changed, 647 insertions(+)
I agree that "CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c"
is necessary. (If I understand correctly, that file was missing from
your v2 posting.)

But "CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c" seems
superfluous. This file is never referenced in the INF files.

The point of this file would be to allow *some* of the Base / Pei /
Runtime / Smm instances to "stub out" the EVP MD functions (i.e. provide
only stub implementations). But this isn't what's happening -- all of
the Base / Pei / Runtime / Smm instances are getting the real deal
("CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c").

(1) So I think "CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c"
should be dropped. Only the Null instance of the library needs null
versions of the new functions. The Base / Pei / Runtime / Smm instances
don't.


diff --git a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
index 4aae2aba95..3968f29412 100644
--- a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
@@ -50,6 +50,7 @@
Pk/CryptAuthenticode.c
Pk/CryptTs.c
Pem/CryptPem.c
+ Evp/CryptEvpMd.c

SysCall/CrtWrapper.c
SysCall/TimerWrapper.c
diff --git a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
index dc28e3a11d..d0b91716d0 100644
--- a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
@@ -57,6 +57,7 @@
Pk/CryptTsNull.c
Pem/CryptPemNull.c
Rand/CryptRandNull.c
+ Evp/CryptEvpMd.c

SysCall/CrtWrapper.c
SysCall/ConstantTimeClock.c
diff --git a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
index 5005beed02..9f3accd35b 100644
--- a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
@@ -56,6 +56,7 @@
Pk/CryptAuthenticodeNull.c
Pk/CryptTsNull.c
Pem/CryptPem.c
+ Evp/CryptEvpMd.c

SysCall/CrtWrapper.c
SysCall/TimerWrapper.c
diff --git a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
index 91ec3e03bf..420623cdc6 100644
--- a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
@@ -54,6 +54,7 @@
Pk/CryptAuthenticodeNull.c
Pk/CryptTsNull.c
Pem/CryptPem.c
+ Evp/CryptEvpMd.c

SysCall/CrtWrapper.c
SysCall/ConstantTimeClock.c
diff --git a/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
b/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
index 689af4fedd..542ac2e2e1 100644
--- a/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
+++ b/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
@@ -50,6 +50,7 @@
Pk/CryptTsNull.c
Pem/CryptPemNull.c
Rand/CryptRandNull.c
+ Evp/CryptEvpMdNull.c

[Packages]
MdePkg/MdePkg.dec
diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h
b/CryptoPkg/Include/Library/BaseCryptLib.h
index ae9bde9e37..5e1b408b54 100644
--- a/CryptoPkg/Include/Library/BaseCryptLib.h
+++ b/CryptoPkg/Include/Library/BaseCryptLib.h
@@ -1012,6 +1012,135 @@ HmacSha256Final (
OUT UINT8 *HmacValue
);

+//==============================================================
=======================
+// EVP (Envelope) Primitive
+//==============================================================
=======================
+
+/**
+ Allocates and initializes one EVP_MD_CTX context for subsequent
EVP_MD
use.
+
+ If DigestName is NULL, then return FALSE.
+
+ @param[in] DigestName Pointer to the digest name as a NULL-
terminated ASCII string.
+ Valid digest names are:
+ MD5, SHA1, SHA224, SHA256, SHA384, SHA512
+ SHA3-224, SHA3-256, SHA3-384, SHA3-512
+ SM3
+
+ @return Pointer to the EVP_MD_CTX context that has been allocated
and
initialized.
+ If DigestName is invalid, returns NULL.
+ If the allocations fails, returns NULL.
+ If initialization fails, returns NULL.
+
+**/
+VOID *
+EFIAPI
+EvpMdInit (
+ IN CONST CHAR8 *DigestName
+ );
+
+/**
+ Makes a copy of an existing EVP_MD context.
+
+ If EvpMdContext is NULL, then return FALSE.
+ If NewEvpMdContext is NULL, then return FALSE.
+
+ @param[in] EvpMdContext Pointer to EVP_MD context being copied.
+ @param[out] NewEvpMdContext Pointer to new EVP_MD context.
+
+ @retval TRUE EVP_MD context copy succeeded.
+ @retval FALSE EVP_MD context copy failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdDuplicate (
+ IN CONST VOID *EvpMdContext,
+ OUT VOID *NewEvpMdContext
+ );
+
+/**
+ Digests the input data and updates EVP_MD context.
+
+ This function performs EVP digest on a data buffer of the specified size.
+ It can be called multiple times to compute the digest of long or
discontinuous data streams.
+ EVP_MD context should be already correctly initialized by EvpMdInit(),
and
should not
+ be finalized by EvpMdFinal(). Behavior with invalid context is undefined.
+
+ If EvpMdContext is NULL, then return FALSE.
+ If Data is NULL and DataSize is not zero, return FALSE.
+
+ @param[in, out] EvpMdContext Pointer to the EVP_MD context.
+ @param[in] Data Pointer to the buffer containing the data to
be
digested.
+ @param[in] DataSize Size of Data buffer in bytes.
+
+ @retval TRUE EVP data digest succeeded.
+ @retval FALSE EVP data digest failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdUpdate (
+ IN OUT VOID *EvpMdContext,
+ IN CONST VOID *Data,
+ IN UINTN DataSize
+ );
+
+/**
+ Completes computation of the EVP digest value.
+ Releases the specified EVP_MD_CTX context.
+
+ This function completes EVP hash computation and retrieves the digest
value into
+ the specified memory. After this function has been called, the EVP
context
cannot
+ be used again.
+ EVP context should be already correctly initialized by EvpMdInit(), and
should
+ not be finalized by EvpMdFinal(). Behavior with invalid EVP context is
undefined.
+
+ If EvpMdContext is NULL, then return FALSE.
+ If DigestValue is NULL, free the Context then return FALSE.
+
+ @param[in, out] EvpMdContext Pointer to the EVP context.
+ @param[out] Digest Pointer to a buffer that receives the EVP
digest
value.

(2) Please extend the comment on Digest with the following:

The caller is responsible for providing enough storage for the digest
algorithm selected with EvpMdInit(). Providing EVP_MAX_MD_SIZE bytes
will suffice for storing the digest regardless of the algorithm chosen
in EvpMdInit().

(EVP_MAX_MD_SIZE is a public OpenSSL macro and I think we should openly
advertise it to consumers in edk2.)

+
+ @retval TRUE EVP digest computation succeeded.
+ @retval FALSE EVP digest computation failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdFinal (
+ IN OUT VOID *EvpMdContext,
+ OUT UINT8 *DigestValue
(3) DigestValue should be marked OPTIONAL in my opinion, as NULL is
deliberately permitted (for just freeing the context).

+ );
+
+/**
+ Computes the message digest of an input data buffer.
+
+ This function performs the message digest of a given data buffer, and
places
+ the digest value into the specified memory.
+
+ If DigestName is NULL, return FALSE.
+ If Data is NULL and DataSize is not zero, return FALSE.
+ If HashValue is NULL, return FALSE.
+
+ @param[in] DigestName Pointer to the digest name.
+ @param[in] Data Pointer to the buffer containing the data to be
hashed.
+ @param[in] DataSize Size of Data buffer in bytes.
+ @param[out] HashValue Pointer to a buffer that receives the digest
value.
+
+ @retval TRUE Digest computation succeeded.
+ @retval FALSE Digest computation failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdHashAll (
+ IN CONST CHAR8 *DigestName,
+ IN CONST VOID *Data,
+ IN UINTN DataSize,
+ OUT UINT8 *HashValue
+ );
+
//===============================================================
======================
// Symmetric Cryptography Primitive
//===============================================================
======================
diff --git a/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
new file mode 100644
index 0000000000..b2770a9186
--- /dev/null
+++ b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
@@ -0,0 +1,257 @@
+/** @file
+ EVP MD Wrapper Implementation for OpenSSL.
+
+Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include "InternalCryptLib.h"
+#include <openssl/evp.h>
+
+/**
+ Allocates and initializes one EVP_MD_CTX context for subsequent
EVP_MD
use.
+
+ If DigestName is NULL, then return FALSE.
+
+ @param[in] DigestName Pointer to the digest name as a NULL-
terminated ASCII string.
+ Valid digest names are:
+ MD5, SHA1, SHA224, SHA256, SHA384, SHA512
+ SHA3-224, SHA3-256, SHA3-384, SHA3-512
+ SM3
+
+ @return Pointer to the EVP_MD_CTX context that has been allocated
and
initialized.
+ If DigestName is invalid, returns NULL.
+ If the allocations fails, returns NULL.
+ If initialization fails, returns NULL.
+
+**/
+VOID *
+EFIAPI
+EvpMdInit (
+ IN CONST CHAR8 *DigestName
+ )
+{
+ EVP_MD *Digest;
+ VOID *EvpMdContext;
+
+ //
+ // Check input parameters.
+ //
+ if (DigestName == NULL) {
+ return NULL;
+ }
+
+ //
+ // Allocate EVP_MD_CTX Context
+ //
+ EvpMdContext = EVP_MD_CTX_new ();
+ if (EvpMdContext == NULL) {
+ return NULL;
+ }
+
+ Digest = EVP_get_digestbyname (DigestName);
I think this may not compile with gcc (and correctly so). The pointer
returned by EVP_get_digestbyname() is const-qualified, but with the
assignment, we're throwing away the const-ness.

(4) Please const-qualify the "Digest" local pointer.

+ if (Digest == NULL) {
+ return NULL;
+ }
(5) This is a memory leak I believe; "EvpMdContext" is leaked.

For keeping the control flow simple, consider moving
EVP_get_digestbyname() above EVP_MD_CTX_new().

+
+ //
+ // Initialize Context
+ //
+ if (EVP_DigestInit_ex (EvpMdContext, Digest, NULL) != 1) {
+ EVP_MD_CTX_free (EvpMdContext);
+ return NULL;
+ }
+
+ return EvpMdContext;
+}
+
+/**
+ Makes a copy of an existing EVP_MD context.
+
+ If EvpMdContext is NULL, then return FALSE.
+ If NewEvpMdContext is NULL, then return FALSE.
+
+ @param[in] EvpMdContext Pointer to EVP_MD context being copied.
+ @param[out] NewEvpMdContext Pointer to new EVP_MD context.
+
+ @retval TRUE EVP_MD context copy succeeded.
+ @retval FALSE EVP_MD context copy failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdDuplicate (
+ IN CONST VOID *EvpMdContext,
+ OUT VOID *NewEvpMdContext
+ )
+{
+ //
+ // Check input parameters.
+ //
+ if (EvpMdContext == NULL || NewEvpMdContext == NULL) {
+ return FALSE;
+ }
+
+ if (EVP_MD_CTX_copy (NewEvpMdContext, EvpMdContext) != 1) {
+ return FALSE;
+ }
+
+ return TRUE;
+}
(6) Can you please confirm that the caller is supposed to initialize
"NewEvpMdContext" with EvpMdInit() first, before calling EvpMdDuplicate()?

+
+/**
+ Digests the input data and updates EVP_MD context.
+
+ This function performs EVP digest on a data buffer of the specified size.
+ It can be called multiple times to compute the digest of long or
discontinuous data streams.
+ EVP_MD context should be already correctly initialized by EvpMdInit(),
and
should not
+ be finalized by EvpMdFinal(). Behavior with invalid context is undefined.
+
+ If EvpMdContext is NULL, then return FALSE.
+ If Data is NULL and DataSize is not zero, return FALSE.
+
+ @param[in, out] EvpMdContext Pointer to the EVP_MD context.
+ @param[in] Data Pointer to the buffer containing the data to
be
digested.
+ @param[in] DataSize Size of Data buffer in bytes.
+
+ @retval TRUE EVP data digest succeeded.
+ @retval FALSE EVP data digest failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdUpdate (
+ IN OUT VOID *EvpMdContext,
+ IN CONST VOID *Data,
+ IN UINTN DataSize
+ )
+{
+ //
+ // Check input parameters.
+ //
+ if (EvpMdContext == NULL) {
+ return FALSE;
+ }
+
+ //
+ // Check invalid parameters, in case only DataLength was checked in
OpenSSL
+ //
+ if (Data == NULL && DataSize != 0) {
+ return FALSE;
+ }
+
+ //
+ // OpenSSL EVP digest update
+ //
+ if (EVP_DigestUpdate (EvpMdContext, Data, DataSize) != 1) {
+ return FALSE;
+ }
+
+ return TRUE;
+}
+
+/**
+ Completes computation of the EVP digest value.
+ Releases the specified EVP_MD_CTX context.
+
+ This function completes EVP hash computation and retrieves the digest
value into
+ the specified memory. After this function has been called, the EVP
context
cannot
+ be used again.
+ EVP context should be already correctly initialized by EvpMdInit(), and
should
+ not be finalized by EvpMdFinal(). Behavior with invalid EVP context is
undefined.
+
+ If EvpMdContext is NULL, then return FALSE.
+ If DigestValue is NULL, free the Context then return FALSE.
+
+ @param[in, out] EvpMdContext Pointer to the EVP context.
+ @param[out] Digest Pointer to a buffer that receives the EVP
digest
value.
+
+ @retval TRUE EVP digest computation succeeded.
+ @retval FALSE EVP digest computation failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdFinal (
+ IN OUT VOID *EvpMdContext,
+ OUT UINT8 *DigestValue
+ )
+{
+ UINT32 Length;
+ BOOLEAN ReturnValue;
+
+ ReturnValue = TRUE;
+
+ //
+ // Check input parameters.
+ //
+ if (EvpMdContext == NULL) {
+ return FALSE;
+ }
+ if (DigestValue == NULL) {
+ EVP_MD_CTX_free (EvpMdContext);
+ return FALSE;
+ }
+
+ //
+ // OpenSSL EVP digest finalization
+ //
+ if (EVP_DigestFinal_ex (EvpMdContext, DigestValue, &Length) != 1) {
+ ReturnValue = FALSE;
+ }

(7) I suggest dropping the "Length" local variable. EVP_DigestFinal_ex()
deals fine with the third parameter being NULL, according to the docs
(and the code).


+
+ //
+ // Free OpenSSL EVP_MD_CTX Context
+ //
+ EVP_MD_CTX_free (EvpMdContext);
+
+ return ReturnValue;
+}
+
+/**
+ Computes the message digest of an input data buffer.
+
+ This function performs the message digest of a given data buffer, and
places
+ the digest value into the specified memory.
+
+ If DigestName is NULL, return FALSE.
+ If Data is NULL and DataSize is not zero, return FALSE.
+ If HashValue is NULL, return FALSE.
+
+ @param[in] DigestName Pointer to the digest name.
+ @param[in] Data Pointer to the buffer containing the data to be
hashed.
+ @param[in] DataSize Size of Data buffer in bytes.
+ @param[out] HashValue Pointer to a buffer that receives the digest
value.
+
+ @retval TRUE Digest computation succeeded.
+ @retval FALSE Digest computation failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdHashAll (
+ IN CONST CHAR8 *DigestName,
+ IN CONST VOID *Data,
+ IN UINTN DataSize,
+ OUT UINT8 *HashValue
+ )
+{
+ BOOLEAN Result;
+ VOID *EvpMdContext;
+
+ EvpMdContext = EvpMdInit (DigestName);
+ if (EvpMdContext == NULL) {
+ return FALSE;
+ }
+
+ Result = EvpMdUpdate (EvpMdContext, Data, DataSize);
+ if (Result == FALSE) {
(8) Style: please write (!Result).


+ EvpMdFinal (EvpMdContext, NULL);
+ return FALSE;
+ }
+
+ Result = EvpMdFinal (EvpMdContext, HashValue);
+
+ return Result;
+}
diff --git a/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c
b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c
new file mode 100644
index 0000000000..038f63801f
--- /dev/null
+++ b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c
@@ -0,0 +1,128 @@
+/** @file
+ EVP MD Wrapper Null Library.
+
+Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include "InternalCryptLib.h"
+
+/**
+ Allocates and initializes one EVP_MD_CTX context for subsequent
EVP_MD
use.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in] DigestName Pointer to the digest name as a NULL-
terminated ASCII string.
+ Valid digest names are:
+ MD5, SHA1, SHA224, SHA256, SHA384, SHA512
+ SHA3-224, SHA3-256, SHA3-384, SHA3-512
+ SM3
+
+ @return NULL This interface is not supported.
+
+**/
+VOID *
+EFIAPI
+EvpMdInit (
+ IN CONST CHAR8 *DigestName
+ )
+{
+ ASSERT (FALSE);
+ return NULL;
+}
+
+/**
+ Makes a copy of an existing EVP_MD context.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in] EvpMdContext Pointer to EVP_MD context being copied.
+ @param[out] NewEvpMdContext Pointer to new EVP_MD context.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdDuplicate (
+ IN CONST VOID *EvpMdContext,
+ OUT VOID *NewEvpMdContext
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
+
+/**
+ Digests the input data and updates EVP_MD context.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in, out] EvpMdContext Pointer to the EVP_MD context.
+ @param[in] Data Pointer to the buffer containing the data to
be
digested.
+ @param[in] DataSize Size of Data buffer in bytes.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdUpdate (
+ IN OUT VOID *EvpMdContext,
+ IN CONST VOID *Data,
+ IN UINTN DataSize
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
+
+/**
+ Completes computation of the EVP digest value.
+ Releases the specified EVP_MD_CTX context.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in, out] EvpMdContext Pointer to the EVP context.
+ @param[out] Digest Pointer to a buffer that receives the EVP
digest
value.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdFinal (
+ IN OUT VOID *EvpMdContext,
+ OUT UINT8 *DigestValue
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
+
+/**
+ Computes the message digest of an input data buffer.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in] DigestName Pointer to the digest name.
+ @param[in] Data Pointer to the buffer containing the data to be
hashed.
+ @param[in] DataSize Size of Data buffer in bytes.
+ @param[out] HashValue Pointer to a buffer that receives the digest
value.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdHashAll (
+ IN CONST CHAR8 *DigestName,
+ IN CONST VOID *Data,
+ IN UINTN DataSize,
+ OUT UINT8 *HashValue
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
diff --git a/CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c
b/CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c
new file mode 100644
index 0000000000..038f63801f
--- /dev/null
+++ b/CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c
@@ -0,0 +1,128 @@
+/** @file
+ EVP MD Wrapper Null Library.
+
+Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include "InternalCryptLib.h"
+
+/**
+ Allocates and initializes one EVP_MD_CTX context for subsequent
EVP_MD
use.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in] DigestName Pointer to the digest name as a NULL-
terminated ASCII string.
+ Valid digest names are:
+ MD5, SHA1, SHA224, SHA256, SHA384, SHA512
+ SHA3-224, SHA3-256, SHA3-384, SHA3-512
+ SM3
+
+ @return NULL This interface is not supported.
+
+**/
+VOID *
+EFIAPI
+EvpMdInit (
+ IN CONST CHAR8 *DigestName
+ )
+{
+ ASSERT (FALSE);
+ return NULL;
+}
+
+/**
+ Makes a copy of an existing EVP_MD context.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in] EvpMdContext Pointer to EVP_MD context being copied.
+ @param[out] NewEvpMdContext Pointer to new EVP_MD context.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdDuplicate (
+ IN CONST VOID *EvpMdContext,
+ OUT VOID *NewEvpMdContext
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
+
+/**
+ Digests the input data and updates EVP_MD context.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in, out] EvpMdContext Pointer to the EVP_MD context.
+ @param[in] Data Pointer to the buffer containing the data to
be
digested.
+ @param[in] DataSize Size of Data buffer in bytes.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdUpdate (
+ IN OUT VOID *EvpMdContext,
+ IN CONST VOID *Data,
+ IN UINTN DataSize
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
+
+/**
+ Completes computation of the EVP digest value.
+ Releases the specified EVP_MD_CTX context.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in, out] EvpMdContext Pointer to the EVP context.
+ @param[out] Digest Pointer to a buffer that receives the EVP
digest
value.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdFinal (
+ IN OUT VOID *EvpMdContext,
+ OUT UINT8 *DigestValue
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
+
+/**
+ Computes the message digest of an input data buffer.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in] DigestName Pointer to the digest name.
+ @param[in] Data Pointer to the buffer containing the data to be
hashed.
+ @param[in] DataSize Size of Data buffer in bytes.
+ @param[out] HashValue Pointer to a buffer that receives the digest
value.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdHashAll (
+ IN CONST CHAR8 *DigestName,
+ IN CONST VOID *Data,
+ IN UINTN DataSize,
+ OUT UINT8 *HashValue
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
Thanks,
Laszlo



Re: [PATCH] IntelFsp2Pkg GenCfgOpt.py: Initialize IncLines as empty list

Zeng, Star
 

Reviewed-by: Star Zeng <star.zeng@intel.com>

-----Original Message-----
From: Chiu, Chasel <chasel.chiu@intel.com>
Sent: Wednesday, September 16, 2020 8:21 PM
To: devel@edk2.groups.io; gaoliming@byosoft.com.cn
Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: RE: [edk2-devel] [PATCH] IntelFsp2Pkg GenCfgOpt.py: Initialize IncLines as empty list


Reviewed-by: Chasel Chiu <chasel.chiu@intel.com>

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
gaoliming
Sent: Wednesday, September 16, 2020 5:58 PM
To: devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L
<nathaniel.l.desimone@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: [edk2-devel] [PATCH] IntelFsp2Pkg GenCfgOpt.py: Initialize
IncLines as empty list

IncLines as empty list for the case when InputHeaderFile is not specified.

Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Signed-off-by: Liming Gao <gaoliming@byosoft.com.cn>
---
IntelFsp2Pkg/Tools/GenCfgOpt.py | 1 +
1 file changed, 1 insertion(+)

diff --git a/IntelFsp2Pkg/Tools/GenCfgOpt.py
b/IntelFsp2Pkg/Tools/GenCfgOpt.py index e9de128e..bcced590 100644
--- a/IntelFsp2Pkg/Tools/GenCfgOpt.py
+++ b/IntelFsp2Pkg/Tools/GenCfgOpt.py
@@ -1177,6 +1177,7 @@ EndList
UpdSignatureCheck = ['FSPT_UPD_SIGNATURE',
'FSPM_UPD_SIGNATURE', 'FSPS_UPD_SIGNATURE']
ExcludedSpecificUpd = ['FSPT_ARCH_UPD', 'FSPM_ARCH_UPD',
'FSPS_ARCH_UPD']

+ IncLines = []
if InputHeaderFile != '':
if not os.path.exists(InputHeaderFile):
self.Error = "Input header file '%s' does not exist"
% InputHeaderFile
--
2.27.0.windows.1




Re: [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface

Yao, Jiewen
 

Thank you Laszlo.
You forced me to read the code again and more carefully. :-)

I believe I understand why you use NULL to free the context now - to support error path.

First, I did have some new thought for EVP API. Let’s consider 3 cases, 1) Hash all data one time, 2) Hash update data multiple times, 3) Error during update.

A. In current design, the API sequence is:
1) EvpMdHashAll (Digest)
2) EvpMdInit, EvpMdUpdate, EvpMdUpdate, ... , EvpMdFinal (Digest)
3) EvpMdInit, EvpMdUpdate->ERROR, EvpMdFinal (NULL)

B. We can auto free context in EvpMdUpdate in error path - the API sequence is:
1) EvpMdHashAll (Digest)
2) EvpMdInit, EvpMdUpdate, EvpMdUpdate, ... , EvpMdFinal (Digest)
3) EvpMdInit, EvpMdUpdate->ERROR

C: We can use New/Free style - similar to HMAC
1) EvpMdHashAll (Digest)
2) EvpMdNew, EvpMdInit, EvpMdUpdate, EvpMdUpdate, ... , EvpMdFinal (Digest), EvpMdFree
3) EvpMdNew, EvpMdInit, EvpMdUpdate->ERROR, EvpMdFree

I checked below APIs:
1) openssl (EVP_MD_CTX_new, EVP_DigestInit_ex, EVP_DigestUpdate, EVP_DigestFinal_ex, EVP_MD_CTX_free)
2) mbedtls (mbedtls_sha256_init, mbedtls_sha256_starts_ret, mbedtls_sha256_update_ret, mbedtls_sha256_finish_ret, mbedtls_sha256_free)
3) BaseCryptoLib HMAC (HmacSha256New, HmacSha256SetKey, HmacSha256Update, HmacSha256Final, HmacSha256Free)

All APIs include free function to free the context, I don’t think it is a bad idea to have EvpMdFree() API.
I would like to propose option - C.


Second, I believe EVP is just something in openssl. It does not mean that we *have to* design API to follow openssl.
After rethink the size impact, I do have concern to merge all Hash implementation together in one function.
It might means nothing if the crypto library is based upon openssl.
But if the cryptolib implementation is based upon other crypto, such as Intel IPP (https://github.com/slimbootloader/slimbootloader/tree/master/BootloaderCommonPkg/Library/IppCryptoLib) or mbedTls (https://github.com/jyao1/edk2/tree/DeviceSecurity/CryptoMbedTlsPkg), then we can NOT get size benefit by separating the hash algorithm.


I would like to propose separate EvpMdxxx.
EvpMdNew -> Sha256New, Sha384New
EvpMdInit -> Sha256Init, Sha384Init
EvpMdUpdate -> Sha256Update, Sha384Update
EvpMdFinal -> Sha256Final, Sha384Final
EvpMdFree -> Sha256Free, Sha384Free

We can do similar thing with Hmac, to deprecate Sha256GetContextSize() API, and replace caller with Sha256New() / Sha384Free()

Thank you
Yao Jiewen

-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com>
Sent: Wednesday, September 16, 2020 9:36 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io; Zurcher,
Christopher J <christopher.j.zurcher@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>
Subject: Re: [edk2-devel] [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP
(Envelope) Digest interface

On 09/16/20 13:44, Yao, Jiewen wrote:
Hi Laszlo
I disagree to put OPTIONAL for DigestValue, just because NULL is checked.
If we need follow this, then we need add OPTIONAL to almost all pointer,
which is unnecessary.

I'm not suggesting OPTIONAL *only* because NULL is checked. I'm
suggesting OPTIONAL because there is a specific use case related to
that. Please see the comments on the function:

"If DigestValue is NULL, free the Context then return FALSE."

But, anyway, I don't insist. It's not a huge deal. Feel free to ignore
my comment regarding OPTIONAL.

Thanks
Laszlo



+BOOLEAN
+EFIAPI
+EvpMdFinal (
+ IN OUT VOID *EvpMdContext,
+ OUT UINT8 *DigestValue
(3) DigestValue should be marked OPTIONAL in my opinion, as NULL is
deliberately permitted (for just freeing the context).




-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com>
Sent: Wednesday, September 16, 2020 7:07 PM
To: devel@edk2.groups.io; Zurcher, Christopher J
<christopher.j.zurcher@intel.com>
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
<jian.j.wang@intel.com>;
Lu, XiaoyuX <xiaoyux.lu@intel.com>
Subject: Re: [edk2-devel] [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP
(Envelope) Digest interface

Hello Christopher,

On 09/16/20 02:59, Zurcher, Christopher J wrote:
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2545

The EVP interface should be used in place of discrete digest function
calls.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
Signed-off-by: Christopher J Zurcher <christopher.j.zurcher@intel.com>
---
CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf | 1 +
CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf | 1 +
CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf | 1 +
CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf | 1 +
CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf | 1 +
CryptoPkg/Include/Library/BaseCryptLib.h | 129 ++++++++++
CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c | 257
++++++++++++++++++++
CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c | 128
++++++++++
CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c | 128
++++++++++
9 files changed, 647 insertions(+)
I agree that "CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c"
is necessary. (If I understand correctly, that file was missing from
your v2 posting.)

But "CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c" seems
superfluous. This file is never referenced in the INF files.

The point of this file would be to allow *some* of the Base / Pei /
Runtime / Smm instances to "stub out" the EVP MD functions (i.e. provide
only stub implementations). But this isn't what's happening -- all of
the Base / Pei / Runtime / Smm instances are getting the real deal
("CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c").

(1) So I think "CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c"
should be dropped. Only the Null instance of the library needs null
versions of the new functions. The Base / Pei / Runtime / Smm instances
don't.


diff --git a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
index 4aae2aba95..3968f29412 100644
--- a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
@@ -50,6 +50,7 @@
Pk/CryptAuthenticode.c
Pk/CryptTs.c
Pem/CryptPem.c
+ Evp/CryptEvpMd.c

SysCall/CrtWrapper.c
SysCall/TimerWrapper.c
diff --git a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
index dc28e3a11d..d0b91716d0 100644
--- a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
@@ -57,6 +57,7 @@
Pk/CryptTsNull.c
Pem/CryptPemNull.c
Rand/CryptRandNull.c
+ Evp/CryptEvpMd.c

SysCall/CrtWrapper.c
SysCall/ConstantTimeClock.c
diff --git a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
index 5005beed02..9f3accd35b 100644
--- a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
@@ -56,6 +56,7 @@
Pk/CryptAuthenticodeNull.c
Pk/CryptTsNull.c
Pem/CryptPem.c
+ Evp/CryptEvpMd.c

SysCall/CrtWrapper.c
SysCall/TimerWrapper.c
diff --git a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
index 91ec3e03bf..420623cdc6 100644
--- a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
@@ -54,6 +54,7 @@
Pk/CryptAuthenticodeNull.c
Pk/CryptTsNull.c
Pem/CryptPem.c
+ Evp/CryptEvpMd.c

SysCall/CrtWrapper.c
SysCall/ConstantTimeClock.c
diff --git a/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
b/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
index 689af4fedd..542ac2e2e1 100644
--- a/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
+++ b/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
@@ -50,6 +50,7 @@
Pk/CryptTsNull.c
Pem/CryptPemNull.c
Rand/CryptRandNull.c
+ Evp/CryptEvpMdNull.c

[Packages]
MdePkg/MdePkg.dec
diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h
b/CryptoPkg/Include/Library/BaseCryptLib.h
index ae9bde9e37..5e1b408b54 100644
--- a/CryptoPkg/Include/Library/BaseCryptLib.h
+++ b/CryptoPkg/Include/Library/BaseCryptLib.h
@@ -1012,6 +1012,135 @@ HmacSha256Final (
OUT UINT8 *HmacValue
);

+//==============================================================
=======================
+// EVP (Envelope) Primitive
+//==============================================================
=======================
+
+/**
+ Allocates and initializes one EVP_MD_CTX context for subsequent
EVP_MD
use.
+
+ If DigestName is NULL, then return FALSE.
+
+ @param[in] DigestName Pointer to the digest name as a NULL-
terminated ASCII string.
+ Valid digest names are:
+ MD5, SHA1, SHA224, SHA256, SHA384, SHA512
+ SHA3-224, SHA3-256, SHA3-384, SHA3-512
+ SM3
+
+ @return Pointer to the EVP_MD_CTX context that has been allocated and
initialized.
+ If DigestName is invalid, returns NULL.
+ If the allocations fails, returns NULL.
+ If initialization fails, returns NULL.
+
+**/
+VOID *
+EFIAPI
+EvpMdInit (
+ IN CONST CHAR8 *DigestName
+ );
+
+/**
+ Makes a copy of an existing EVP_MD context.
+
+ If EvpMdContext is NULL, then return FALSE.
+ If NewEvpMdContext is NULL, then return FALSE.
+
+ @param[in] EvpMdContext Pointer to EVP_MD context being copied.
+ @param[out] NewEvpMdContext Pointer to new EVP_MD context.
+
+ @retval TRUE EVP_MD context copy succeeded.
+ @retval FALSE EVP_MD context copy failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdDuplicate (
+ IN CONST VOID *EvpMdContext,
+ OUT VOID *NewEvpMdContext
+ );
+
+/**
+ Digests the input data and updates EVP_MD context.
+
+ This function performs EVP digest on a data buffer of the specified size.
+ It can be called multiple times to compute the digest of long or
discontinuous data streams.
+ EVP_MD context should be already correctly initialized by EvpMdInit(),
and
should not
+ be finalized by EvpMdFinal(). Behavior with invalid context is undefined.
+
+ If EvpMdContext is NULL, then return FALSE.
+ If Data is NULL and DataSize is not zero, return FALSE.
+
+ @param[in, out] EvpMdContext Pointer to the EVP_MD context.
+ @param[in] Data Pointer to the buffer containing the data to
be
digested.
+ @param[in] DataSize Size of Data buffer in bytes.
+
+ @retval TRUE EVP data digest succeeded.
+ @retval FALSE EVP data digest failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdUpdate (
+ IN OUT VOID *EvpMdContext,
+ IN CONST VOID *Data,
+ IN UINTN DataSize
+ );
+
+/**
+ Completes computation of the EVP digest value.
+ Releases the specified EVP_MD_CTX context.
+
+ This function completes EVP hash computation and retrieves the digest
value into
+ the specified memory. After this function has been called, the EVP context
cannot
+ be used again.
+ EVP context should be already correctly initialized by EvpMdInit(), and
should
+ not be finalized by EvpMdFinal(). Behavior with invalid EVP context is
undefined.
+
+ If EvpMdContext is NULL, then return FALSE.
+ If DigestValue is NULL, free the Context then return FALSE.
+
+ @param[in, out] EvpMdContext Pointer to the EVP context.
+ @param[out] Digest Pointer to a buffer that receives the EVP
digest
value.

(2) Please extend the comment on Digest with the following:

The caller is responsible for providing enough storage for the digest
algorithm selected with EvpMdInit(). Providing EVP_MAX_MD_SIZE bytes
will suffice for storing the digest regardless of the algorithm chosen
in EvpMdInit().

(EVP_MAX_MD_SIZE is a public OpenSSL macro and I think we should openly
advertise it to consumers in edk2.)

+
+ @retval TRUE EVP digest computation succeeded.
+ @retval FALSE EVP digest computation failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdFinal (
+ IN OUT VOID *EvpMdContext,
+ OUT UINT8 *DigestValue
(3) DigestValue should be marked OPTIONAL in my opinion, as NULL is
deliberately permitted (for just freeing the context).

+ );
+
+/**
+ Computes the message digest of an input data buffer.
+
+ This function performs the message digest of a given data buffer, and
places
+ the digest value into the specified memory.
+
+ If DigestName is NULL, return FALSE.
+ If Data is NULL and DataSize is not zero, return FALSE.
+ If HashValue is NULL, return FALSE.
+
+ @param[in] DigestName Pointer to the digest name.
+ @param[in] Data Pointer to the buffer containing the data to be
hashed.
+ @param[in] DataSize Size of Data buffer in bytes.
+ @param[out] HashValue Pointer to a buffer that receives the digest
value.
+
+ @retval TRUE Digest computation succeeded.
+ @retval FALSE Digest computation failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdHashAll (
+ IN CONST CHAR8 *DigestName,
+ IN CONST VOID *Data,
+ IN UINTN DataSize,
+ OUT UINT8 *HashValue
+ );
+
//===============================================================
======================
// Symmetric Cryptography Primitive
//===============================================================
======================
diff --git a/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
new file mode 100644
index 0000000000..b2770a9186
--- /dev/null
+++ b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
@@ -0,0 +1,257 @@
+/** @file
+ EVP MD Wrapper Implementation for OpenSSL.
+
+Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include "InternalCryptLib.h"
+#include <openssl/evp.h>
+
+/**
+ Allocates and initializes one EVP_MD_CTX context for subsequent
EVP_MD
use.
+
+ If DigestName is NULL, then return FALSE.
+
+ @param[in] DigestName Pointer to the digest name as a NULL-
terminated ASCII string.
+ Valid digest names are:
+ MD5, SHA1, SHA224, SHA256, SHA384, SHA512
+ SHA3-224, SHA3-256, SHA3-384, SHA3-512
+ SM3
+
+ @return Pointer to the EVP_MD_CTX context that has been allocated and
initialized.
+ If DigestName is invalid, returns NULL.
+ If the allocations fails, returns NULL.
+ If initialization fails, returns NULL.
+
+**/
+VOID *
+EFIAPI
+EvpMdInit (
+ IN CONST CHAR8 *DigestName
+ )
+{
+ EVP_MD *Digest;
+ VOID *EvpMdContext;
+
+ //
+ // Check input parameters.
+ //
+ if (DigestName == NULL) {
+ return NULL;
+ }
+
+ //
+ // Allocate EVP_MD_CTX Context
+ //
+ EvpMdContext = EVP_MD_CTX_new ();
+ if (EvpMdContext == NULL) {
+ return NULL;
+ }
+
+ Digest = EVP_get_digestbyname (DigestName);
I think this may not compile with gcc (and correctly so). The pointer
returned by EVP_get_digestbyname() is const-qualified, but with the
assignment, we're throwing away the const-ness.

(4) Please const-qualify the "Digest" local pointer.

+ if (Digest == NULL) {
+ return NULL;
+ }
(5) This is a memory leak I believe; "EvpMdContext" is leaked.

For keeping the control flow simple, consider moving
EVP_get_digestbyname() above EVP_MD_CTX_new().

+
+ //
+ // Initialize Context
+ //
+ if (EVP_DigestInit_ex (EvpMdContext, Digest, NULL) != 1) {
+ EVP_MD_CTX_free (EvpMdContext);
+ return NULL;
+ }
+
+ return EvpMdContext;
+}
+
+/**
+ Makes a copy of an existing EVP_MD context.
+
+ If EvpMdContext is NULL, then return FALSE.
+ If NewEvpMdContext is NULL, then return FALSE.
+
+ @param[in] EvpMdContext Pointer to EVP_MD context being copied.
+ @param[out] NewEvpMdContext Pointer to new EVP_MD context.
+
+ @retval TRUE EVP_MD context copy succeeded.
+ @retval FALSE EVP_MD context copy failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdDuplicate (
+ IN CONST VOID *EvpMdContext,
+ OUT VOID *NewEvpMdContext
+ )
+{
+ //
+ // Check input parameters.
+ //
+ if (EvpMdContext == NULL || NewEvpMdContext == NULL) {
+ return FALSE;
+ }
+
+ if (EVP_MD_CTX_copy (NewEvpMdContext, EvpMdContext) != 1) {
+ return FALSE;
+ }
+
+ return TRUE;
+}
(6) Can you please confirm that the caller is supposed to initialize
"NewEvpMdContext" with EvpMdInit() first, before calling EvpMdDuplicate()?

+
+/**
+ Digests the input data and updates EVP_MD context.
+
+ This function performs EVP digest on a data buffer of the specified size.
+ It can be called multiple times to compute the digest of long or
discontinuous data streams.
+ EVP_MD context should be already correctly initialized by EvpMdInit(),
and
should not
+ be finalized by EvpMdFinal(). Behavior with invalid context is undefined.
+
+ If EvpMdContext is NULL, then return FALSE.
+ If Data is NULL and DataSize is not zero, return FALSE.
+
+ @param[in, out] EvpMdContext Pointer to the EVP_MD context.
+ @param[in] Data Pointer to the buffer containing the data to
be
digested.
+ @param[in] DataSize Size of Data buffer in bytes.
+
+ @retval TRUE EVP data digest succeeded.
+ @retval FALSE EVP data digest failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdUpdate (
+ IN OUT VOID *EvpMdContext,
+ IN CONST VOID *Data,
+ IN UINTN DataSize
+ )
+{
+ //
+ // Check input parameters.
+ //
+ if (EvpMdContext == NULL) {
+ return FALSE;
+ }
+
+ //
+ // Check invalid parameters, in case only DataLength was checked in
OpenSSL
+ //
+ if (Data == NULL && DataSize != 0) {
+ return FALSE;
+ }
+
+ //
+ // OpenSSL EVP digest update
+ //
+ if (EVP_DigestUpdate (EvpMdContext, Data, DataSize) != 1) {
+ return FALSE;
+ }
+
+ return TRUE;
+}
+
+/**
+ Completes computation of the EVP digest value.
+ Releases the specified EVP_MD_CTX context.
+
+ This function completes EVP hash computation and retrieves the digest
value into
+ the specified memory. After this function has been called, the EVP context
cannot
+ be used again.
+ EVP context should be already correctly initialized by EvpMdInit(), and
should
+ not be finalized by EvpMdFinal(). Behavior with invalid EVP context is
undefined.
+
+ If EvpMdContext is NULL, then return FALSE.
+ If DigestValue is NULL, free the Context then return FALSE.
+
+ @param[in, out] EvpMdContext Pointer to the EVP context.
+ @param[out] Digest Pointer to a buffer that receives the EVP
digest
value.
+
+ @retval TRUE EVP digest computation succeeded.
+ @retval FALSE EVP digest computation failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdFinal (
+ IN OUT VOID *EvpMdContext,
+ OUT UINT8 *DigestValue
+ )
+{
+ UINT32 Length;
+ BOOLEAN ReturnValue;
+
+ ReturnValue = TRUE;
+
+ //
+ // Check input parameters.
+ //
+ if (EvpMdContext == NULL) {
+ return FALSE;
+ }
+ if (DigestValue == NULL) {
+ EVP_MD_CTX_free (EvpMdContext);
+ return FALSE;
+ }
+
+ //
+ // OpenSSL EVP digest finalization
+ //
+ if (EVP_DigestFinal_ex (EvpMdContext, DigestValue, &Length) != 1) {
+ ReturnValue = FALSE;
+ }

(7) I suggest dropping the "Length" local variable. EVP_DigestFinal_ex()
deals fine with the third parameter being NULL, according to the docs
(and the code).


+
+ //
+ // Free OpenSSL EVP_MD_CTX Context
+ //
+ EVP_MD_CTX_free (EvpMdContext);
+
+ return ReturnValue;
+}
+
+/**
+ Computes the message digest of an input data buffer.
+
+ This function performs the message digest of a given data buffer, and
places
+ the digest value into the specified memory.
+
+ If DigestName is NULL, return FALSE.
+ If Data is NULL and DataSize is not zero, return FALSE.
+ If HashValue is NULL, return FALSE.
+
+ @param[in] DigestName Pointer to the digest name.
+ @param[in] Data Pointer to the buffer containing the data to be
hashed.
+ @param[in] DataSize Size of Data buffer in bytes.
+ @param[out] HashValue Pointer to a buffer that receives the digest
value.
+
+ @retval TRUE Digest computation succeeded.
+ @retval FALSE Digest computation failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdHashAll (
+ IN CONST CHAR8 *DigestName,
+ IN CONST VOID *Data,
+ IN UINTN DataSize,
+ OUT UINT8 *HashValue
+ )
+{
+ BOOLEAN Result;
+ VOID *EvpMdContext;
+
+ EvpMdContext = EvpMdInit (DigestName);
+ if (EvpMdContext == NULL) {
+ return FALSE;
+ }
+
+ Result = EvpMdUpdate (EvpMdContext, Data, DataSize);
+ if (Result == FALSE) {
(8) Style: please write (!Result).


+ EvpMdFinal (EvpMdContext, NULL);
+ return FALSE;
+ }
+
+ Result = EvpMdFinal (EvpMdContext, HashValue);
+
+ return Result;
+}
diff --git a/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c
b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c
new file mode 100644
index 0000000000..038f63801f
--- /dev/null
+++ b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c
@@ -0,0 +1,128 @@
+/** @file
+ EVP MD Wrapper Null Library.
+
+Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include "InternalCryptLib.h"
+
+/**
+ Allocates and initializes one EVP_MD_CTX context for subsequent
EVP_MD
use.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in] DigestName Pointer to the digest name as a NULL-
terminated ASCII string.
+ Valid digest names are:
+ MD5, SHA1, SHA224, SHA256, SHA384, SHA512
+ SHA3-224, SHA3-256, SHA3-384, SHA3-512
+ SM3
+
+ @return NULL This interface is not supported.
+
+**/
+VOID *
+EFIAPI
+EvpMdInit (
+ IN CONST CHAR8 *DigestName
+ )
+{
+ ASSERT (FALSE);
+ return NULL;
+}
+
+/**
+ Makes a copy of an existing EVP_MD context.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in] EvpMdContext Pointer to EVP_MD context being copied.
+ @param[out] NewEvpMdContext Pointer to new EVP_MD context.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdDuplicate (
+ IN CONST VOID *EvpMdContext,
+ OUT VOID *NewEvpMdContext
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
+
+/**
+ Digests the input data and updates EVP_MD context.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in, out] EvpMdContext Pointer to the EVP_MD context.
+ @param[in] Data Pointer to the buffer containing the data to
be
digested.
+ @param[in] DataSize Size of Data buffer in bytes.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdUpdate (
+ IN OUT VOID *EvpMdContext,
+ IN CONST VOID *Data,
+ IN UINTN DataSize
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
+
+/**
+ Completes computation of the EVP digest value.
+ Releases the specified EVP_MD_CTX context.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in, out] EvpMdContext Pointer to the EVP context.
+ @param[out] Digest Pointer to a buffer that receives the EVP
digest
value.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdFinal (
+ IN OUT VOID *EvpMdContext,
+ OUT UINT8 *DigestValue
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
+
+/**
+ Computes the message digest of an input data buffer.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in] DigestName Pointer to the digest name.
+ @param[in] Data Pointer to the buffer containing the data to be
hashed.
+ @param[in] DataSize Size of Data buffer in bytes.
+ @param[out] HashValue Pointer to a buffer that receives the digest
value.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdHashAll (
+ IN CONST CHAR8 *DigestName,
+ IN CONST VOID *Data,
+ IN UINTN DataSize,
+ OUT UINT8 *HashValue
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
diff --git a/CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c
b/CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c
new file mode 100644
index 0000000000..038f63801f
--- /dev/null
+++ b/CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c
@@ -0,0 +1,128 @@
+/** @file
+ EVP MD Wrapper Null Library.
+
+Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include "InternalCryptLib.h"
+
+/**
+ Allocates and initializes one EVP_MD_CTX context for subsequent
EVP_MD
use.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in] DigestName Pointer to the digest name as a NULL-
terminated ASCII string.
+ Valid digest names are:
+ MD5, SHA1, SHA224, SHA256, SHA384, SHA512
+ SHA3-224, SHA3-256, SHA3-384, SHA3-512
+ SM3
+
+ @return NULL This interface is not supported.
+
+**/
+VOID *
+EFIAPI
+EvpMdInit (
+ IN CONST CHAR8 *DigestName
+ )
+{
+ ASSERT (FALSE);
+ return NULL;
+}
+
+/**
+ Makes a copy of an existing EVP_MD context.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in] EvpMdContext Pointer to EVP_MD context being copied.
+ @param[out] NewEvpMdContext Pointer to new EVP_MD context.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdDuplicate (
+ IN CONST VOID *EvpMdContext,
+ OUT VOID *NewEvpMdContext
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
+
+/**
+ Digests the input data and updates EVP_MD context.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in, out] EvpMdContext Pointer to the EVP_MD context.
+ @param[in] Data Pointer to the buffer containing the data to
be
digested.
+ @param[in] DataSize Size of Data buffer in bytes.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdUpdate (
+ IN OUT VOID *EvpMdContext,
+ IN CONST VOID *Data,
+ IN UINTN DataSize
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
+
+/**
+ Completes computation of the EVP digest value.
+ Releases the specified EVP_MD_CTX context.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in, out] EvpMdContext Pointer to the EVP context.
+ @param[out] Digest Pointer to a buffer that receives the EVP
digest
value.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdFinal (
+ IN OUT VOID *EvpMdContext,
+ OUT UINT8 *DigestValue
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
+
+/**
+ Computes the message digest of an input data buffer.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in] DigestName Pointer to the digest name.
+ @param[in] Data Pointer to the buffer containing the data to be
hashed.
+ @param[in] DataSize Size of Data buffer in bytes.
+ @param[out] HashValue Pointer to a buffer that receives the digest
value.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdHashAll (
+ IN CONST CHAR8 *DigestName,
+ IN CONST VOID *Data,
+ IN UINTN DataSize,
+ OUT UINT8 *HashValue
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
Thanks,
Laszlo


Re: [PATCH v1 1/1] ShellPkg/AcpiView: PCCT Parser

Sami Mujawar
 

Hi Zhichao,

Thank you for the feedback.

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gao, Zhichao via groups.io
Sent: 16 September 2020 03:44 AM
To: devel@edk2.groups.io; Sami Mujawar <Sami.Mujawar@arm.com>
Cc: Ni, Ray <ray.ni@intel.com>; Marc Moisson-Franckhauser <Marc.Moisson-Franckhauser@arm.com>; Guillaume Letellier <Guillaume.Letellier@arm.com>; Matteo Carlini <Matteo.Carlini@arm.com>; Ben Adderson <Ben.Adderson@arm.com>; nd <nd@arm.com>
Subject: Re: [edk2-devel] [PATCH v1 1/1] ShellPkg/AcpiView: PCCT Parser

Hi Sami,

Sorry for the delay review. Please see below.

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sami
Mujawar
Sent: Monday, August 24, 2020 6:23 PM
To: devel@edk2.groups.io
Cc: Sami Mujawar <sami.mujawar@arm.com>; Ni, Ray <ray.ni@intel.com>; Gao,
Zhichao <zhichao.gao@intel.com>; marc.moisson-franckhauser@arm.com;
Guillaume.Letellier@arm.com; Matteo.Carlini@arm.com;
Ben.Adderson@arm.com; nd@arm.com
Subject: [edk2-devel] [PATCH v1 1/1] ShellPkg/AcpiView: PCCT Parser

From: Marc Moisson-Franckhauser <marc.moisson-franckhauser@arm.com>

Create a new parser for the PCCT Table.

The PCCT Table is used to describe how the OSPM can communicate with entities
outside the platform. It describes which memory spaces correspond to which
entity as well as a few of the needed information to handle the communications.

This new PCCT parser dumps the values and names of the table fields. It also
performs some validation on the table's fields.

Signed-off-by: Marc Moisson-Franckhauser <marc.moisson-
franckhauser@arm.com>
Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---

The changes can be seen at:
https://github.com/samimujawar/edk2/tree/840_pcct_parser_v1

ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h | 24 +-
ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.h | 4
+-
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pcct/PcctParser.c |
494 ++++++++++++++++++++
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pcct/PcctParser.h |
33 ++

ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c
| 4 +-

ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.i
nf | 4 +-
6 files changed, 558 insertions(+), 5 deletions(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
index
f81ccac7e118378aa185db4b625e5bcd75f78347..051fdf807abb1067a264c136364
bb6d145b38dab 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
@@ -1,7 +1,7 @@
/** @file
Header file for ACPI parser

- Copyright (c) 2016 - 2020, ARM Limited. All rights reserved.
+ Copyright (c) 2016 - 2020, Arm Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent **/

@@ -671,6 +671,28 @@ ParseAcpiMcfg (
);

/**
+ This function parses the ACPI PCCT table including its sub-structures
+ of type 0 through 4.
+ When trace is enabled this function parses the PCCT table and traces
+ the ACPI table fields.
+
+ This function also performs validation of the ACPI table fields.
+
+ @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] Ptr Pointer to the start of the buffer.
+ @param [in] AcpiTableLength Length of the ACPI table.
+ @param [in] AcpiTableRevision Revision of the ACPI table.
+**/
+VOID
+EFIAPI
+ParseAcpiPcct (
+ IN BOOLEAN Trace,
+ IN UINT8* Ptr,
+ IN UINT32 AcpiTableLength,
+ IN UINT8 AcpiTableRevision
+ );
+
+/**
This function parses the ACPI PPTT table.
When trace is enabled this function parses the PPTT table and
traces the ACPI table fields.
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.h
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.h
index
4f92596b90a6ee422d8d0959881015ffd3de4da0..19265d0b763f8a810759a2cef0
9ce2cc2d7bec03 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.h
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiTableParser.h
@@ -1,7 +1,7 @@
/** @file
Header file for ACPI table parser

- Copyright (c) 2016 - 2018, ARM Limited. All rights reserved.
+ Copyright (c) 2016 - 2020, Arm Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent **/

@@ -11,7 +11,7 @@
/**
The maximum number of ACPI table parsers.
*/
-#define MAX_ACPI_TABLE_PARSERS 16
+#define MAX_ACPI_TABLE_PARSERS 17

/** An invalid/NULL signature value.
*/
diff --git
a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pcct/PcctParser.c
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pcct/PcctParser.c
new file mode 100644
index
0000000000000000000000000000000000000000..526cb7b79aa7aa6eee098246
00b6c7eac0ab67e2
--- /dev/null
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pcct/PcctPars
+++ er.c
@@ -0,0 +1,494 @@
+/** @file
+ PCCT table parser
+
+ Copyright (c) 2020, Arm Limited.
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+ @par Reference(s):
+ - ACPI 6.3 Specification - January 2019 **/
+
+#include <Library/PrintLib.h>
+#include <Library/UefiLib.h>
+#include "AcpiParser.h"
+#include "AcpiView.h"
+#include "AcpiViewConfig.h"
+#include "PcctParser.h"
+
+// Local variables
+STATIC ACPI_DESCRIPTION_HEADER_INFO AcpiHdrInfo;
+
+STATIC UINT8* PccSubspaceLength;
+STATIC UINT8* PccSubspaceType;
+
+/**
+ This function validates the length coded on 4 bytes of a shared
+memory range
+
+ @param [in] Ptr Pointer to the start of the field data.
+ @param [in] Context Pointer to context specific information e.g. this
+ could be a pointer to the ACPI table header.
+**/
+STATIC
+VOID
+EFIAPI
+ValidateRangeLength4 (
+ IN UINT8* Ptr,
+ IN VOID* Context
+ )
+{
+ if (*(UINT32*)Ptr < MIN_EXT_PCC_SUBSPACE_MEM_RANGE_LEN) {
+ IncrementErrorCount ();
+ Print (
+ L"\nError: Shared memory range length is too short.\n"
+ L"Length is %u when it should be greater than or equal to %u",
+ *(UINT32*)Ptr,
+ MIN_EXT_PCC_SUBSPACE_MEM_RANGE_LEN
+ );
+ }
+}
+
+/**
+ This function validates the length coded on 8 bytes of a shared
+memory range
+
+ @param [in] Ptr Pointer to the start of the field data.
+ @param [in] Context Pointer to context specific information e.g. this
+ could be a pointer to the ACPI table header.
+**/
+STATIC
+VOID
+EFIAPI
+ValidateRangeLength8 (
+ IN UINT8* Ptr,
+ IN VOID* Context
+ )
+{
+ if (*(UINT64*)Ptr <= MIN_MEMORY_RANGE_LENGTH) {
+ IncrementErrorCount ();
+ Print (
+ L"\nError: Shared memory range length is too short.\n"
+ L"Length is %u when it should be greater than %u",
+ *(UINT64*)Ptr,
+ MIN_MEMORY_RANGE_LENGTH
+ );
+ }
+}
+
+/**
+ This function validates address space for type 0 structure.
+
+ @param [in] Ptr Pointer to the start of the field data.
+ @param [in] Context Pointer to context specific information e.g. this
+ could be a pointer to the ACPI table header.
+**/
+STATIC
+VOID
+EFIAPI
+ValidatePccType0Gas (
+ IN UINT8* Ptr,
+ IN VOID* Context
+ )
+{
+ switch (*(UINT8*)Ptr) {
+#if !(defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64))
+ case EFI_ACPI_6_3_SYSTEM_IO:
+#endif //if not (defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64))
Why doesn't ARM arch need this check? Is there any doc that descripts this? Just curious.

[SAMI] ARM architecture does not have an I/O address space. The ARM Architecture Reference Manual (https://developer.arm.com/documentation/102105/0101/?search=5eec7399e24a5e02d07b2754) is the best place to get the latest information on Arm Architecture.
The 'Application Note AN 274 Migrating from IA-32 to ARM' (https://developer.arm.com/documentation/dai0274/b?lang=en) may be of interest here. See Section 3.1.1, point 2 for information on I/O address space.
[/SAMI]

+ case EFI_ACPI_6_3_SYSTEM_MEMORY:
+ return;
+ default:
+ IncrementErrorCount ();
+ Print (L"\nError: Invalid address space");
+ }
+}
+
+/**
+ This function validates address space for structures of types other than 0.
+
+ @param [in] Ptr Pointer to the start of the field data.
+ @param [in] Context Pointer to context specific information e.g. this
+ could be a pointer to the ACPI table header.
+**/
+STATIC
+VOID
+EFIAPI
+ValidatePccGas (
+ IN UINT8* Ptr,
+ IN VOID* Context
+ )
+{
+ switch (*(UINT8*)Ptr) {
+#if !(defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64))
+ case EFI_ACPI_6_3_SYSTEM_IO:
+#endif //if not (defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64))
+ case EFI_ACPI_6_3_FUNCTIONAL_FIXED_HARDWARE:
+ case EFI_ACPI_6_3_SYSTEM_MEMORY:
+ return;
+ default:
+ IncrementErrorCount ();
+ Print (L"\nError: Invalid address space");
+ }
+}
This function is used for subspace type1, 2, 3 and 4. But refer the ACPI 6.3, the field for type4 is optional. If it is not supported, the field would be filled with 0x0 value. So I think we should put the type into consideration.
[SAMI] I think this can be fixed with a few additional checks. I will send out a v2 patch with this changed. [/SAMI]

+
+/**
+ An ACPI_PARSER array describing the ACPI PCCT Table.
+*/
+STATIC CONST ACPI_PARSER PcctParser[] = {
+ PARSE_ACPI_HEADER (&AcpiHdrInfo),
+ {L"Flags", 4, 36, NULL, NULL, NULL, NULL, NULL},
+ {L"Reserved", 8, 40, NULL, NULL, NULL, NULL, NULL} };
+
+/**
+ An ACPI_PARSER array describing the platform communications channel
+subspace
+ structure header.
+*/
+STATIC CONST ACPI_PARSER PccSubspaceHeaderParser[] = {
+ PCC_SUBSPACE_HEADER ()
+ // ... Type Specific Fields ...
+};
+
+/**
+ An ACPI_PARSER array describing the Generic Communications Subspace -
+Type 0 */ STATIC CONST ACPI_PARSER PccSubspaceType0Parser[] = {
+ PCC_SUBSPACE_HEADER (),
+ {L"Reserved", 6, 2, L"%x %x %x %x %x %x", Dump6Chars, NULL, NULL,
+NULL},
+ {L"Base Address", 8, 8, L"0x%lx", NULL, NULL, NULL, NULL},
+ {L"Memory Range Length", 8, 16, L"0x%lx", NULL, NULL,
ValidateRangeLength8,
+ NULL},
+ {L"Doorbell Register", 12, 24, NULL, DumpGas, NULL, ValidatePccType0Gas,
+ NULL},
+ {L"Doorbell Preserve", 8, 36, L"0x%lx", NULL, NULL, NULL, NULL},
+ {L"Doorbell Write", 8, 44, L"0x%lx", NULL, NULL, NULL, NULL},
+ {L"Nominal Latency", 4, 52, L"%u", NULL, NULL, NULL, NULL},
+ {L"Maximum Periodic Access Rate", 4, 56, L"%u", NULL, NULL, NULL,
+NULL},
+ {L"Minimum Request Turnaround Time", 2, 60, L"%u", NULL, NULL, NULL,
+NULL} };
+
+/**
+ An ACPI_PARSER array describing the HW-Reduced Communications
+Subspace
+ - Type 1
+*/
+STATIC CONST ACPI_PARSER PccSubspaceType1Parser[] = {
+ PCC_SUBSPACE_HEADER (),
+ {L"Platform Interrupt", 4, 2, L"0x%x", NULL, NULL, NULL, NULL},
+ {L"Platform Interrupt Flags", 1, 6, L"0x%x", NULL, NULL, NULL, NULL},
+ {L"Reserved", 1, 7, L"0x%x", NULL, NULL, NULL, NULL},
+ {L"Base Address", 8, 8, L"0x%lx", NULL, NULL, NULL, NULL},
+ {L"Memory Range Length", 8, 16, L"0x%lx", NULL, NULL,
ValidateRangeLength8,
+ NULL},
+ {L"Doorbell Register", 12, 24, NULL, DumpGas, NULL,
+ ValidatePccGas, NULL},
+ {L"Doorbell Preserve", 8, 36, L"0x%lx", NULL, NULL, NULL, NULL},
+ {L"Doorbell Write", 8, 44, L"0x%lx", NULL, NULL, NULL, NULL},
+ {L"Nominal Latency", 4, 52, L"%u", NULL, NULL, NULL, NULL},
+ {L"Maximum Periodic Access Rate", 4, 56, L"%u", NULL, NULL, NULL,
+NULL},
+ {L"Minimum Request Turnaround Time", 2, 60, L"%u", NULL, NULL, NULL,
+NULL} };
+
+/**
+ An ACPI_PARSER array describing the HW-Reduced Communications
+Subspace
+ - Type 2
+*/
+STATIC CONST ACPI_PARSER PccSubspaceType2Parser[] = {
+ PCC_SUBSPACE_HEADER (),
+ {L"Platform Interrupt", 4, 2, L"0x%x", NULL, NULL, NULL, NULL},
+ {L"Platform Interrupt Flags", 1, 6, L"0x%x", NULL, NULL, NULL, NULL},
+ {L"Reserved", 1, 7, L"0x%x", NULL, NULL, NULL, NULL},
+ {L"Base Address", 8, 8, L"0x%lx", NULL, NULL, NULL, NULL},
+ {L"Memory Range Length", 8, 16, L"0x%lx", NULL, NULL,
ValidateRangeLength8,
+ NULL},
+ {L"Doorbell Register", 12, 24, NULL, DumpGas, NULL,
+ ValidatePccGas, NULL},
+ {L"Doorbell Preserve", 8, 36, L"0x%lx", NULL, NULL, NULL, NULL},
+ {L"Doorbell Write", 8, 44, L"0x%lx", NULL, NULL, NULL, NULL},
+ {L"Nominal Latency", 4, 52, L"%u", NULL, NULL, NULL, NULL},
+ {L"Maximum Periodic Access Rate", 4, 56, L"%u", NULL, NULL, NULL,
+NULL},
+ {L"Minimum Request Turnaround Time", 2, 60, L"%u", NULL, NULL, NULL,
+NULL},
+ {L"Platform Interrupt Ack Register", 12, 62, NULL, DumpGas, NULL,
+ ValidatePccGas, NULL},
+ {L"Platform Interrupt Ack Preserve", 8, 74, L"0x%lx", NULL, NULL,
+NULL, NULL},
+ {L"Platform Interrupt Ack Write", 8, 82, L"0x%lx", NULL, NULL,
+ NULL, NULL},
+};
+
+/**
+ An ACPI_PARSER array describing the Extended PCC Subspaces - Type 3/4
+*/ STATIC CONST ACPI_PARSER PccSubspaceType3Parser[] = {
+ PCC_SUBSPACE_HEADER (),
+ {L"Platform Interrupt", 4, 2, L"0x%x", NULL, NULL, NULL, NULL},
+ {L"Platform Interrupt Flags", 1, 6, L"0x%x", NULL, NULL, NULL, NULL},
+ {L"Reserved", 1, 7, L"0x%x", NULL, NULL, NULL, NULL},
+ {L"Base Address", 8, 8, L"0x%lx", NULL, NULL, NULL, NULL},
The offset definition in APCI 6.3 Table 14-368 is different. Seems it is a spec mistake.
[SAMI] We have reported this to ASWG and there is an ECR to fix this in the spec. [/SAMI]

Others are OK to me.

Thanks,
Zhichao

+ {L"Memory Range Length", 4, 16, L"0x%x", NULL, NULL,
ValidateRangeLength4,
+ NULL},
+ {L"Doorbell Register", 12, 20, NULL, DumpGas, NULL,
+ ValidatePccGas, NULL},
+ {L"Doorbell Preserve", 8, 32, L"0x%lx", NULL, NULL, NULL, NULL},
+ {L"Doorbell Write", 8, 40, L"0x%lx", NULL, NULL, NULL, NULL},
+ {L"Nominal Latency", 4, 48, L"%u", NULL, NULL, NULL, NULL},
+ {L"Maximum Periodic Access Rate", 4, 52, L"%u", NULL, NULL, NULL,
+NULL},
+ {L"Minimum Request Turnaround Time", 4, 56, L"%u", NULL, NULL, NULL,
+NULL},
+ {L"Platform Interrupt Ack Register", 12, 60, NULL, DumpGas, NULL,
+ ValidatePccGas, NULL},
+ {L"Platform Interrupt Ack Preserve", 8, 72, L"0x%lx", NULL, NULL,
+NULL, NULL},
+ {L"Platform Interrupt Ack Set", 8, 80, L"0x%lx", NULL, NULL, NULL,
+NULL},
+ {L"Reserved", 8, 88, L"0x%lx", NULL, NULL, NULL, NULL},
+ {L"Cmd Complete Check Reg Addr", 12, 96, NULL, DumpGas, NULL,
+ ValidatePccGas, NULL},
+ {L"Cmd Complete Check Mask", 8, 108, L"0x%lx", NULL, NULL, NULL,
+NULL},
+ {L"Cmd Update Reg Addr", 12, 116, NULL, DumpGas, NULL,
+ ValidatePccGas, NULL},
+ {L"Cmd Update Preserve mask", 8, 128, L"0x%lx", NULL, NULL, NULL,
+NULL},
+ {L"Cmd Update Set mask", 8, 136, L"0x%lx", NULL, NULL, NULL, NULL},
+ {L"Error Status Register", 12, 144, NULL, DumpGas, NULL,
+ ValidatePccGas, NULL},
+ {L"Error Status Mask", 8, 156, L"0x%lx", NULL, NULL, NULL, NULL}, };
+
+/**
+ This function parses the PCC Subspace type 0.
+
+ @param [in] Ptr Pointer to the start of Subspace Structure.
+ @param [in] Length Length of the Subspace Structure.
+**/
+STATIC
+VOID
+DumpPccSubspaceType0 (
+ IN UINT8* Ptr,
+ IN UINT8 Length
+ )
+{
+ ParseAcpi (
+ TRUE,
+ 2,
+ "Subspace Type 0",
+ Ptr,
+ Length,
+ PARSER_PARAMS (PccSubspaceType0Parser)
+ );
+}
+
+/**
+ This function parses the PCC Subspace type 1.
+
+ @param [in] Ptr Pointer to the start of the Subspace Structure.
+ @param [in] Length Length of the Subspace Structure.
+**/
+STATIC
+VOID
+DumpPccSubspaceType1 (
+ IN UINT8* Ptr,
+ IN UINT8 Length
+ )
+{
+ ParseAcpi (
+ TRUE,
+ 2,
+ "Subspace Type 1",
+ Ptr,
+ Length,
+ PARSER_PARAMS (PccSubspaceType1Parser)
+ );
+}
+
+/**
+ This function parses the PCC Subspace type 2.
+
+ @param [in] Ptr Pointer to the start of the Subspace Structure.
+ @param [in] Length Length of the Subspace Structure.
+**/
+STATIC
+VOID
+DumpPccSubspaceType2 (
+ IN UINT8* Ptr,
+ IN UINT8 Length
+ )
+{
+ ParseAcpi (
+ TRUE,
+ 2,
+ "Subspace Type 2",
+ Ptr,
+ Length,
+ PARSER_PARAMS (PccSubspaceType2Parser)
+ );
+}
+
+/**
+ This function parses the PCC Subspace type 3.
+
+ @param [in] Ptr Pointer to the start of the Subspace Structure.
+ @param [in] Length Length of the Subspace Structure.
+**/
+STATIC
+VOID
+DumpPccSubspaceType3 (
+ IN UINT8* Ptr,
+ IN UINT8 Length
+ )
+{
+ ParseAcpi (
+ TRUE,
+ 2,
+ "Subspace Type 3",
+ Ptr,
+ Length,
+ PARSER_PARAMS (PccSubspaceType3Parser)
+ );
+}
+
+/**
+ This function parses the PCC Subspace type 4.
+
+ @param [in] Ptr Pointer to the start of the Subspace Structure.
+ @param [in] Length Length of the Subspace Structure.
+**/
+STATIC
+VOID
+DumpPccSubspaceType4 (
+ IN UINT8* Ptr,
+ IN UINT8 Length
+ )
+{
+ ParseAcpi (
+ TRUE,
+ 2,
+ "Subspace Type 4",
+ Ptr,
+ Length,
+ PARSER_PARAMS (PccSubspaceType3Parser)
+ );
+}
+
+/**
+ This function parses the ACPI PCCT table including its sub-structures
+ of type 0 through 4.
+ When trace is enabled this function parses the PCCT table and
+ traces the ACPI table fields.
+
+ This function also performs validation of the ACPI table fields.
+
+ @param [in] Trace If TRUE, trace the ACPI fields.
+ @param [in] Ptr Pointer to the start of the buffer.
+ @param [in] AcpiTableLength Length of the ACPI table.
+ @param [in] AcpiTableRevision Revision of the ACPI table.
+**/
+VOID
+EFIAPI
+ParseAcpiPcct (
+ IN BOOLEAN Trace,
+ IN UINT8* Ptr,
+ IN UINT32 AcpiTableLength,
+ IN UINT8 AcpiTableRevision
+ )
+{
+ UINT32 Offset;
+ UINT8* PccSubspacePtr;
+ UINTN SubspaceCount;
+
+ if (!Trace) {
+ return;
+ }
+
+ Offset = ParseAcpi (
+ TRUE,
+ 0,
+ "PCCT",
+ Ptr,
+ AcpiTableLength,
+ PARSER_PARAMS (PcctParser)
+ );
+
+ PccSubspacePtr = Ptr + Offset;
+
+ SubspaceCount = 0;
+ while (Offset < AcpiTableLength) {
+ // Parse common structure header to obtain Type and Length.
+ ParseAcpi (
+ FALSE,
+ 0,
+ NULL,
+ PccSubspacePtr,
+ AcpiTableLength - Offset,
+ PARSER_PARAMS (PccSubspaceHeaderParser)
+ );
+
+ // Check if the values used to control the parsing logic have been
+ // successfully read.
+ if ((PccSubspaceType == NULL) ||
+ (PccSubspaceLength == NULL)) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Insufficient remaining table buffer length to read the " \
+ L"structure header. Length = %u.\n",
+ AcpiTableLength - Offset
+ );
+ return;
+ }
+
+ // Validate Structure length
+ if ((*PccSubspaceLength == 0) ||
+ ((Offset + (*PccSubspaceLength)) > AcpiTableLength)) {
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Invalid Structure length. " \
+ L"Length = %u. Offset = %u. AcpiTableLength = %u.\n",
+ *PccSubspaceLength,
+ Offset,
+ AcpiTableLength
+ );
+ return;
+ }
+
+ switch (*PccSubspaceType) {
+ case EFI_ACPI_6_3_PCCT_SUBSPACE_TYPE_GENERIC:
+ DumpPccSubspaceType0 (
+ PccSubspacePtr,
+ *PccSubspaceLength
+ );
+ break;
+ case
EFI_ACPI_6_3_PCCT_SUBSPACE_TYPE_1_HW_REDUCED_COMMUNICATIONS:
+ DumpPccSubspaceType1 (
+ PccSubspacePtr,
+ *PccSubspaceLength
+ );
+ break;
+ case
EFI_ACPI_6_3_PCCT_SUBSPACE_TYPE_2_HW_REDUCED_COMMUNICATIONS:
+ DumpPccSubspaceType2 (
+ PccSubspacePtr,
+ *PccSubspaceLength
+ );
+ break;
+ case EFI_ACPI_6_3_PCCT_SUBSPACE_TYPE_3_EXTENDED_PCC:
+ DumpPccSubspaceType3 (
+ PccSubspacePtr,
+ *PccSubspaceLength
+ );
+ break;
+ case EFI_ACPI_6_3_PCCT_SUBSPACE_TYPE_4_EXTENDED_PCC:
+ DumpPccSubspaceType4 (
+ PccSubspacePtr,
+ *PccSubspaceLength
+ );
+ break;
+ default:
+ IncrementErrorCount ();
+ Print (
+ L"ERROR: Unknown PCC subspace structure:"
+ L" Type = %u, Length = %u\n",
+ PccSubspaceType,
+ *PccSubspaceLength
+ );
+ }
+
+ PccSubspacePtr += *PccSubspaceLength;
+ Offset += *PccSubspaceLength;
+ SubspaceCount++;
+ } // while
+
+ if (SubspaceCount > MAX_PCC_SUBSPACES) {
+ IncrementErrorCount ();
+ Print (L"ERROR: Too many PCC subspaces.");
+ }
+}
diff --git
a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pcct/PcctParser.h
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pcct/PcctParser.h
new file mode 100644
index
0000000000000000000000000000000000000000..278dc83c5de8860cbb2b1e2b2
e277aa7c6c58698
--- /dev/null
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pcct/PcctPars
+++ er.h
@@ -0,0 +1,33 @@
+/** @file
+ Header file for PCCT parser
+
+ Copyright (c) 2020, Arm Limited.
+ SPDX-License-Identifier: BSD-2-Clause-Patent **/
+
+#ifndef PCCT_PARSER_H_
+#define PCCT_PARSER_H_
+
+/**
+ Minimum value for the 'length' field in subspaces of types 0, 1 and 2.
+*/
+#define MIN_MEMORY_RANGE_LENGTH 8
+
+/**
+ Minimum value for the 'length' field in subspaces of types 3 and 4.
+*/
+#define MIN_EXT_PCC_SUBSPACE_MEM_RANGE_LEN 16
+
+/**
+ Maximum number of PCC subspaces.
+*/
+#define MAX_PCC_SUBSPACES 256
+
+/**
+ Parser for the header of any type of PCC subspace.
+*/
+#define PCC_SUBSPACE_HEADER() \
+ {L"Type", 1, 0, L"0x%x", NULL, (VOID**)&PccSubspaceType, NULL, NULL}, \
+ {L"Length", 1, 1, L"%u", NULL, (VOID**)&PccSubspaceLength, NULL,
+NULL}
+
+#endif // PCCT_PARSER_H_
diff --git
a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLi
b.c
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLi
b.c
index
d2f26ff89f12e596702281c38ab0de3729aa68e4..feb80661cddc420670edb2d8c7a
570b0a89272d8 100644
---
a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLi
b.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewComm
+++ andLib.c
@@ -1,7 +1,7 @@
/** @file
Main file for 'acpiview' Shell command function.

- Copyright (c) 2016 - 2020, ARM Limited. All rights reserved.<BR>
+ Copyright (c) 2016 - 2020, Arm Limited. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent **/

@@ -57,6 +57,8 @@ ACPI_TABLE_PARSER ParserList[] = {
{EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE,
ParseAcpiMadt},

{EFI_ACPI_6_2_PCI_EXPRESS_MEMORY_MAPPED_CONFIGURATION_SPACE_BA
SE_ADDRESS_DESCRIPTION_TABLE_SIGNATURE,
ParseAcpiMcfg},
+
{EFI_ACPI_6_2_PLATFORM_COMMUNICATIONS_CHANNEL_TABLE_SIGNATURE,
+ ParseAcpiPcct},

{EFI_ACPI_6_2_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE_STRUCTURE_SIGN
ATURE,
ParseAcpiPptt},
{RSDP_TABLE_INFO, ParseAcpiRsdp},
diff --git
a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLi
b.inf
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLi
b.inf
index
91459f9ec632635ee453c5ef46f67445cd9eee0c..efa9c8784a6670e5a4f500e0ae5
59a4938852f95 100644
---
a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLi
b.inf
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewComm
+++ andLib.inf
@@ -1,7 +1,7 @@
## @file
# Provides Shell 'acpiview' command functions # -# Copyright (c) 2016 - 2020,
ARM Limited. All rights reserved.<BR>
+# Copyright (c) 2016 - 2020, Arm Limited. All rights reserved.<BR>
#
# SPDX-License-Identifier: BSD-2-Clause-Patent # @@ -37,6 +37,8 @@
[Sources.common]
Parsers/Madt/MadtParser.c
Parsers/Madt/MadtParser.h
Parsers/Mcfg/McfgParser.c
+ Parsers/Pcct/PcctParser.c
+ Parsers/Pcct/PcctParser.h
Parsers/Pptt/PpttParser.c
Parsers/Pptt/PpttParser.h
Parsers/Rsdp/RsdpParser.c
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'



Re: [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface

Laszlo Ersek
 

On 09/16/20 13:44, Yao, Jiewen wrote:
Hi Laszlo
I disagree to put OPTIONAL for DigestValue, just because NULL is checked.
If we need follow this, then we need add OPTIONAL to almost all pointer, which is unnecessary.
I'm not suggesting OPTIONAL *only* because NULL is checked. I'm
suggesting OPTIONAL because there is a specific use case related to
that. Please see the comments on the function:

"If DigestValue is NULL, free the Context then return FALSE."

But, anyway, I don't insist. It's not a huge deal. Feel free to ignore
my comment regarding OPTIONAL.

Thanks
Laszlo



+BOOLEAN
+EFIAPI
+EvpMdFinal (
+ IN OUT VOID *EvpMdContext,
+ OUT UINT8 *DigestValue
(3) DigestValue should be marked OPTIONAL in my opinion, as NULL is
deliberately permitted (for just freeing the context).




-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com>
Sent: Wednesday, September 16, 2020 7:07 PM
To: devel@edk2.groups.io; Zurcher, Christopher J
<christopher.j.zurcher@intel.com>
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
Lu, XiaoyuX <xiaoyux.lu@intel.com>
Subject: Re: [edk2-devel] [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP
(Envelope) Digest interface

Hello Christopher,

On 09/16/20 02:59, Zurcher, Christopher J wrote:
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2545

The EVP interface should be used in place of discrete digest function
calls.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
Signed-off-by: Christopher J Zurcher <christopher.j.zurcher@intel.com>
---
CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf | 1 +
CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf | 1 +
CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf | 1 +
CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf | 1 +
CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf | 1 +
CryptoPkg/Include/Library/BaseCryptLib.h | 129 ++++++++++
CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c | 257
++++++++++++++++++++
CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c | 128 ++++++++++
CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c | 128 ++++++++++
9 files changed, 647 insertions(+)
I agree that "CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c"
is necessary. (If I understand correctly, that file was missing from
your v2 posting.)

But "CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c" seems
superfluous. This file is never referenced in the INF files.

The point of this file would be to allow *some* of the Base / Pei /
Runtime / Smm instances to "stub out" the EVP MD functions (i.e. provide
only stub implementations). But this isn't what's happening -- all of
the Base / Pei / Runtime / Smm instances are getting the real deal
("CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c").

(1) So I think "CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c"
should be dropped. Only the Null instance of the library needs null
versions of the new functions. The Base / Pei / Runtime / Smm instances
don't.


diff --git a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
index 4aae2aba95..3968f29412 100644
--- a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
@@ -50,6 +50,7 @@
Pk/CryptAuthenticode.c
Pk/CryptTs.c
Pem/CryptPem.c
+ Evp/CryptEvpMd.c

SysCall/CrtWrapper.c
SysCall/TimerWrapper.c
diff --git a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
index dc28e3a11d..d0b91716d0 100644
--- a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
@@ -57,6 +57,7 @@
Pk/CryptTsNull.c
Pem/CryptPemNull.c
Rand/CryptRandNull.c
+ Evp/CryptEvpMd.c

SysCall/CrtWrapper.c
SysCall/ConstantTimeClock.c
diff --git a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
index 5005beed02..9f3accd35b 100644
--- a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
@@ -56,6 +56,7 @@
Pk/CryptAuthenticodeNull.c
Pk/CryptTsNull.c
Pem/CryptPem.c
+ Evp/CryptEvpMd.c

SysCall/CrtWrapper.c
SysCall/TimerWrapper.c
diff --git a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
index 91ec3e03bf..420623cdc6 100644
--- a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
@@ -54,6 +54,7 @@
Pk/CryptAuthenticodeNull.c
Pk/CryptTsNull.c
Pem/CryptPem.c
+ Evp/CryptEvpMd.c

SysCall/CrtWrapper.c
SysCall/ConstantTimeClock.c
diff --git a/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
b/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
index 689af4fedd..542ac2e2e1 100644
--- a/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
+++ b/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
@@ -50,6 +50,7 @@
Pk/CryptTsNull.c
Pem/CryptPemNull.c
Rand/CryptRandNull.c
+ Evp/CryptEvpMdNull.c

[Packages]
MdePkg/MdePkg.dec
diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h
b/CryptoPkg/Include/Library/BaseCryptLib.h
index ae9bde9e37..5e1b408b54 100644
--- a/CryptoPkg/Include/Library/BaseCryptLib.h
+++ b/CryptoPkg/Include/Library/BaseCryptLib.h
@@ -1012,6 +1012,135 @@ HmacSha256Final (
OUT UINT8 *HmacValue
);

+//==============================================================
=======================
+// EVP (Envelope) Primitive
+//==============================================================
=======================
+
+/**
+ Allocates and initializes one EVP_MD_CTX context for subsequent EVP_MD
use.
+
+ If DigestName is NULL, then return FALSE.
+
+ @param[in] DigestName Pointer to the digest name as a NULL-
terminated ASCII string.
+ Valid digest names are:
+ MD5, SHA1, SHA224, SHA256, SHA384, SHA512
+ SHA3-224, SHA3-256, SHA3-384, SHA3-512
+ SM3
+
+ @return Pointer to the EVP_MD_CTX context that has been allocated and
initialized.
+ If DigestName is invalid, returns NULL.
+ If the allocations fails, returns NULL.
+ If initialization fails, returns NULL.
+
+**/
+VOID *
+EFIAPI
+EvpMdInit (
+ IN CONST CHAR8 *DigestName
+ );
+
+/**
+ Makes a copy of an existing EVP_MD context.
+
+ If EvpMdContext is NULL, then return FALSE.
+ If NewEvpMdContext is NULL, then return FALSE.
+
+ @param[in] EvpMdContext Pointer to EVP_MD context being copied.
+ @param[out] NewEvpMdContext Pointer to new EVP_MD context.
+
+ @retval TRUE EVP_MD context copy succeeded.
+ @retval FALSE EVP_MD context copy failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdDuplicate (
+ IN CONST VOID *EvpMdContext,
+ OUT VOID *NewEvpMdContext
+ );
+
+/**
+ Digests the input data and updates EVP_MD context.
+
+ This function performs EVP digest on a data buffer of the specified size.
+ It can be called multiple times to compute the digest of long or
discontinuous data streams.
+ EVP_MD context should be already correctly initialized by EvpMdInit(), and
should not
+ be finalized by EvpMdFinal(). Behavior with invalid context is undefined.
+
+ If EvpMdContext is NULL, then return FALSE.
+ If Data is NULL and DataSize is not zero, return FALSE.
+
+ @param[in, out] EvpMdContext Pointer to the EVP_MD context.
+ @param[in] Data Pointer to the buffer containing the data to be
digested.
+ @param[in] DataSize Size of Data buffer in bytes.
+
+ @retval TRUE EVP data digest succeeded.
+ @retval FALSE EVP data digest failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdUpdate (
+ IN OUT VOID *EvpMdContext,
+ IN CONST VOID *Data,
+ IN UINTN DataSize
+ );
+
+/**
+ Completes computation of the EVP digest value.
+ Releases the specified EVP_MD_CTX context.
+
+ This function completes EVP hash computation and retrieves the digest
value into
+ the specified memory. After this function has been called, the EVP context
cannot
+ be used again.
+ EVP context should be already correctly initialized by EvpMdInit(), and
should
+ not be finalized by EvpMdFinal(). Behavior with invalid EVP context is
undefined.
+
+ If EvpMdContext is NULL, then return FALSE.
+ If DigestValue is NULL, free the Context then return FALSE.
+
+ @param[in, out] EvpMdContext Pointer to the EVP context.
+ @param[out] Digest Pointer to a buffer that receives the EVP digest
value.

(2) Please extend the comment on Digest with the following:

The caller is responsible for providing enough storage for the digest
algorithm selected with EvpMdInit(). Providing EVP_MAX_MD_SIZE bytes
will suffice for storing the digest regardless of the algorithm chosen
in EvpMdInit().

(EVP_MAX_MD_SIZE is a public OpenSSL macro and I think we should openly
advertise it to consumers in edk2.)

+
+ @retval TRUE EVP digest computation succeeded.
+ @retval FALSE EVP digest computation failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdFinal (
+ IN OUT VOID *EvpMdContext,
+ OUT UINT8 *DigestValue
(3) DigestValue should be marked OPTIONAL in my opinion, as NULL is
deliberately permitted (for just freeing the context).

+ );
+
+/**
+ Computes the message digest of an input data buffer.
+
+ This function performs the message digest of a given data buffer, and places
+ the digest value into the specified memory.
+
+ If DigestName is NULL, return FALSE.
+ If Data is NULL and DataSize is not zero, return FALSE.
+ If HashValue is NULL, return FALSE.
+
+ @param[in] DigestName Pointer to the digest name.
+ @param[in] Data Pointer to the buffer containing the data to be
hashed.
+ @param[in] DataSize Size of Data buffer in bytes.
+ @param[out] HashValue Pointer to a buffer that receives the digest
value.
+
+ @retval TRUE Digest computation succeeded.
+ @retval FALSE Digest computation failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdHashAll (
+ IN CONST CHAR8 *DigestName,
+ IN CONST VOID *Data,
+ IN UINTN DataSize,
+ OUT UINT8 *HashValue
+ );
+
//===============================================================
======================
// Symmetric Cryptography Primitive
//===============================================================
======================
diff --git a/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
new file mode 100644
index 0000000000..b2770a9186
--- /dev/null
+++ b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
@@ -0,0 +1,257 @@
+/** @file
+ EVP MD Wrapper Implementation for OpenSSL.
+
+Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include "InternalCryptLib.h"
+#include <openssl/evp.h>
+
+/**
+ Allocates and initializes one EVP_MD_CTX context for subsequent EVP_MD
use.
+
+ If DigestName is NULL, then return FALSE.
+
+ @param[in] DigestName Pointer to the digest name as a NULL-
terminated ASCII string.
+ Valid digest names are:
+ MD5, SHA1, SHA224, SHA256, SHA384, SHA512
+ SHA3-224, SHA3-256, SHA3-384, SHA3-512
+ SM3
+
+ @return Pointer to the EVP_MD_CTX context that has been allocated and
initialized.
+ If DigestName is invalid, returns NULL.
+ If the allocations fails, returns NULL.
+ If initialization fails, returns NULL.
+
+**/
+VOID *
+EFIAPI
+EvpMdInit (
+ IN CONST CHAR8 *DigestName
+ )
+{
+ EVP_MD *Digest;
+ VOID *EvpMdContext;
+
+ //
+ // Check input parameters.
+ //
+ if (DigestName == NULL) {
+ return NULL;
+ }
+
+ //
+ // Allocate EVP_MD_CTX Context
+ //
+ EvpMdContext = EVP_MD_CTX_new ();
+ if (EvpMdContext == NULL) {
+ return NULL;
+ }
+
+ Digest = EVP_get_digestbyname (DigestName);
I think this may not compile with gcc (and correctly so). The pointer
returned by EVP_get_digestbyname() is const-qualified, but with the
assignment, we're throwing away the const-ness.

(4) Please const-qualify the "Digest" local pointer.

+ if (Digest == NULL) {
+ return NULL;
+ }
(5) This is a memory leak I believe; "EvpMdContext" is leaked.

For keeping the control flow simple, consider moving
EVP_get_digestbyname() above EVP_MD_CTX_new().

+
+ //
+ // Initialize Context
+ //
+ if (EVP_DigestInit_ex (EvpMdContext, Digest, NULL) != 1) {
+ EVP_MD_CTX_free (EvpMdContext);
+ return NULL;
+ }
+
+ return EvpMdContext;
+}
+
+/**
+ Makes a copy of an existing EVP_MD context.
+
+ If EvpMdContext is NULL, then return FALSE.
+ If NewEvpMdContext is NULL, then return FALSE.
+
+ @param[in] EvpMdContext Pointer to EVP_MD context being copied.
+ @param[out] NewEvpMdContext Pointer to new EVP_MD context.
+
+ @retval TRUE EVP_MD context copy succeeded.
+ @retval FALSE EVP_MD context copy failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdDuplicate (
+ IN CONST VOID *EvpMdContext,
+ OUT VOID *NewEvpMdContext
+ )
+{
+ //
+ // Check input parameters.
+ //
+ if (EvpMdContext == NULL || NewEvpMdContext == NULL) {
+ return FALSE;
+ }
+
+ if (EVP_MD_CTX_copy (NewEvpMdContext, EvpMdContext) != 1) {
+ return FALSE;
+ }
+
+ return TRUE;
+}
(6) Can you please confirm that the caller is supposed to initialize
"NewEvpMdContext" with EvpMdInit() first, before calling EvpMdDuplicate()?

+
+/**
+ Digests the input data and updates EVP_MD context.
+
+ This function performs EVP digest on a data buffer of the specified size.
+ It can be called multiple times to compute the digest of long or
discontinuous data streams.
+ EVP_MD context should be already correctly initialized by EvpMdInit(), and
should not
+ be finalized by EvpMdFinal(). Behavior with invalid context is undefined.
+
+ If EvpMdContext is NULL, then return FALSE.
+ If Data is NULL and DataSize is not zero, return FALSE.
+
+ @param[in, out] EvpMdContext Pointer to the EVP_MD context.
+ @param[in] Data Pointer to the buffer containing the data to be
digested.
+ @param[in] DataSize Size of Data buffer in bytes.
+
+ @retval TRUE EVP data digest succeeded.
+ @retval FALSE EVP data digest failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdUpdate (
+ IN OUT VOID *EvpMdContext,
+ IN CONST VOID *Data,
+ IN UINTN DataSize
+ )
+{
+ //
+ // Check input parameters.
+ //
+ if (EvpMdContext == NULL) {
+ return FALSE;
+ }
+
+ //
+ // Check invalid parameters, in case only DataLength was checked in
OpenSSL
+ //
+ if (Data == NULL && DataSize != 0) {
+ return FALSE;
+ }
+
+ //
+ // OpenSSL EVP digest update
+ //
+ if (EVP_DigestUpdate (EvpMdContext, Data, DataSize) != 1) {
+ return FALSE;
+ }
+
+ return TRUE;
+}
+
+/**
+ Completes computation of the EVP digest value.
+ Releases the specified EVP_MD_CTX context.
+
+ This function completes EVP hash computation and retrieves the digest
value into
+ the specified memory. After this function has been called, the EVP context
cannot
+ be used again.
+ EVP context should be already correctly initialized by EvpMdInit(), and
should
+ not be finalized by EvpMdFinal(). Behavior with invalid EVP context is
undefined.
+
+ If EvpMdContext is NULL, then return FALSE.
+ If DigestValue is NULL, free the Context then return FALSE.
+
+ @param[in, out] EvpMdContext Pointer to the EVP context.
+ @param[out] Digest Pointer to a buffer that receives the EVP digest
value.
+
+ @retval TRUE EVP digest computation succeeded.
+ @retval FALSE EVP digest computation failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdFinal (
+ IN OUT VOID *EvpMdContext,
+ OUT UINT8 *DigestValue
+ )
+{
+ UINT32 Length;
+ BOOLEAN ReturnValue;
+
+ ReturnValue = TRUE;
+
+ //
+ // Check input parameters.
+ //
+ if (EvpMdContext == NULL) {
+ return FALSE;
+ }
+ if (DigestValue == NULL) {
+ EVP_MD_CTX_free (EvpMdContext);
+ return FALSE;
+ }
+
+ //
+ // OpenSSL EVP digest finalization
+ //
+ if (EVP_DigestFinal_ex (EvpMdContext, DigestValue, &Length) != 1) {
+ ReturnValue = FALSE;
+ }

(7) I suggest dropping the "Length" local variable. EVP_DigestFinal_ex()
deals fine with the third parameter being NULL, according to the docs
(and the code).


+
+ //
+ // Free OpenSSL EVP_MD_CTX Context
+ //
+ EVP_MD_CTX_free (EvpMdContext);
+
+ return ReturnValue;
+}
+
+/**
+ Computes the message digest of an input data buffer.
+
+ This function performs the message digest of a given data buffer, and places
+ the digest value into the specified memory.
+
+ If DigestName is NULL, return FALSE.
+ If Data is NULL and DataSize is not zero, return FALSE.
+ If HashValue is NULL, return FALSE.
+
+ @param[in] DigestName Pointer to the digest name.
+ @param[in] Data Pointer to the buffer containing the data to be
hashed.
+ @param[in] DataSize Size of Data buffer in bytes.
+ @param[out] HashValue Pointer to a buffer that receives the digest
value.
+
+ @retval TRUE Digest computation succeeded.
+ @retval FALSE Digest computation failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdHashAll (
+ IN CONST CHAR8 *DigestName,
+ IN CONST VOID *Data,
+ IN UINTN DataSize,
+ OUT UINT8 *HashValue
+ )
+{
+ BOOLEAN Result;
+ VOID *EvpMdContext;
+
+ EvpMdContext = EvpMdInit (DigestName);
+ if (EvpMdContext == NULL) {
+ return FALSE;
+ }
+
+ Result = EvpMdUpdate (EvpMdContext, Data, DataSize);
+ if (Result == FALSE) {
(8) Style: please write (!Result).


+ EvpMdFinal (EvpMdContext, NULL);
+ return FALSE;
+ }
+
+ Result = EvpMdFinal (EvpMdContext, HashValue);
+
+ return Result;
+}
diff --git a/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c
b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c
new file mode 100644
index 0000000000..038f63801f
--- /dev/null
+++ b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c
@@ -0,0 +1,128 @@
+/** @file
+ EVP MD Wrapper Null Library.
+
+Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include "InternalCryptLib.h"
+
+/**
+ Allocates and initializes one EVP_MD_CTX context for subsequent EVP_MD
use.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in] DigestName Pointer to the digest name as a NULL-
terminated ASCII string.
+ Valid digest names are:
+ MD5, SHA1, SHA224, SHA256, SHA384, SHA512
+ SHA3-224, SHA3-256, SHA3-384, SHA3-512
+ SM3
+
+ @return NULL This interface is not supported.
+
+**/
+VOID *
+EFIAPI
+EvpMdInit (
+ IN CONST CHAR8 *DigestName
+ )
+{
+ ASSERT (FALSE);
+ return NULL;
+}
+
+/**
+ Makes a copy of an existing EVP_MD context.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in] EvpMdContext Pointer to EVP_MD context being copied.
+ @param[out] NewEvpMdContext Pointer to new EVP_MD context.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdDuplicate (
+ IN CONST VOID *EvpMdContext,
+ OUT VOID *NewEvpMdContext
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
+
+/**
+ Digests the input data and updates EVP_MD context.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in, out] EvpMdContext Pointer to the EVP_MD context.
+ @param[in] Data Pointer to the buffer containing the data to be
digested.
+ @param[in] DataSize Size of Data buffer in bytes.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdUpdate (
+ IN OUT VOID *EvpMdContext,
+ IN CONST VOID *Data,
+ IN UINTN DataSize
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
+
+/**
+ Completes computation of the EVP digest value.
+ Releases the specified EVP_MD_CTX context.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in, out] EvpMdContext Pointer to the EVP context.
+ @param[out] Digest Pointer to a buffer that receives the EVP digest
value.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdFinal (
+ IN OUT VOID *EvpMdContext,
+ OUT UINT8 *DigestValue
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
+
+/**
+ Computes the message digest of an input data buffer.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in] DigestName Pointer to the digest name.
+ @param[in] Data Pointer to the buffer containing the data to be
hashed.
+ @param[in] DataSize Size of Data buffer in bytes.
+ @param[out] HashValue Pointer to a buffer that receives the digest
value.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdHashAll (
+ IN CONST CHAR8 *DigestName,
+ IN CONST VOID *Data,
+ IN UINTN DataSize,
+ OUT UINT8 *HashValue
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
diff --git a/CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c
b/CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c
new file mode 100644
index 0000000000..038f63801f
--- /dev/null
+++ b/CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c
@@ -0,0 +1,128 @@
+/** @file
+ EVP MD Wrapper Null Library.
+
+Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include "InternalCryptLib.h"
+
+/**
+ Allocates and initializes one EVP_MD_CTX context for subsequent EVP_MD
use.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in] DigestName Pointer to the digest name as a NULL-
terminated ASCII string.
+ Valid digest names are:
+ MD5, SHA1, SHA224, SHA256, SHA384, SHA512
+ SHA3-224, SHA3-256, SHA3-384, SHA3-512
+ SM3
+
+ @return NULL This interface is not supported.
+
+**/
+VOID *
+EFIAPI
+EvpMdInit (
+ IN CONST CHAR8 *DigestName
+ )
+{
+ ASSERT (FALSE);
+ return NULL;
+}
+
+/**
+ Makes a copy of an existing EVP_MD context.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in] EvpMdContext Pointer to EVP_MD context being copied.
+ @param[out] NewEvpMdContext Pointer to new EVP_MD context.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdDuplicate (
+ IN CONST VOID *EvpMdContext,
+ OUT VOID *NewEvpMdContext
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
+
+/**
+ Digests the input data and updates EVP_MD context.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in, out] EvpMdContext Pointer to the EVP_MD context.
+ @param[in] Data Pointer to the buffer containing the data to be
digested.
+ @param[in] DataSize Size of Data buffer in bytes.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdUpdate (
+ IN OUT VOID *EvpMdContext,
+ IN CONST VOID *Data,
+ IN UINTN DataSize
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
+
+/**
+ Completes computation of the EVP digest value.
+ Releases the specified EVP_MD_CTX context.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in, out] EvpMdContext Pointer to the EVP context.
+ @param[out] Digest Pointer to a buffer that receives the EVP digest
value.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdFinal (
+ IN OUT VOID *EvpMdContext,
+ OUT UINT8 *DigestValue
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
+
+/**
+ Computes the message digest of an input data buffer.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in] DigestName Pointer to the digest name.
+ @param[in] Data Pointer to the buffer containing the data to be
hashed.
+ @param[in] DataSize Size of Data buffer in bytes.
+ @param[out] HashValue Pointer to a buffer that receives the digest
value.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdHashAll (
+ IN CONST CHAR8 *DigestName,
+ IN CONST VOID *Data,
+ IN UINTN DataSize,
+ OUT UINT8 *HashValue
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
Thanks,
Laszlo


Re: [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface

Laszlo Ersek
 

On 09/16/20 14:39, Laszlo Ersek wrote:
On 09/16/20 14:07, Yao, Jiewen wrote:
I overlooked the behavior that NULL DigestValue will take side effect to free Context.

Hi Christopher
May I understand why we need free the context?
This does not align with other existing HASH or HMAC functions.
I requested that.

It makes no sense to keep a finalized context.

In other words, it makes no sense to separate finalization from freeing.
A context that has been finalized is unusable for anything except freeing.
To clarify my point about (DigestValue==NULL): the freeing of the
context is not tied to (DigestValue==NULL). When finalizing the context,
the context should *always* be freed. (DigestValue==NULL) only decides
whether we *only* free the context, or we store the computed digest *in
addition to* freeing the context.

So the unconditional part of the finalization is the freeing; the
conditional part is the storing of the digest.

Thanks
Laszlo


Re: [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface

Yao, Jiewen
 

I try to understand what is the use case to EvpMdFinal without DigestValue and only Free the EvpMdContext.

A normal usage will be:
1) EvpMdHashAll (Digest) - get digest.
2) EvpMdInit, EvpMdUpdate, EvpMdUpdate, ... , EvpMdFinal (Digest) - get digest

Why we need support the case to use EvpMdFinal (NULL) in the end to free context?


Thank you
Yao Jiewen

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
Sent: Wednesday, September 16, 2020 8:40 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io; Zurcher,
Christopher J <christopher.j.zurcher@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>
Subject: Re: [edk2-devel] [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP
(Envelope) Digest interface

On 09/16/20 14:07, Yao, Jiewen wrote:
I overlooked the behavior that NULL DigestValue will take side effect to free
Context.

Hi Christopher
May I understand why we need free the context?
This does not align with other existing HASH or HMAC functions.
I requested that.

It makes no sense to keep a finalized context.

In other words, it makes no sense to separate finalization from freeing.
A context that has been finalized is unusable for anything except freeing.

Thanks
Laszlo


Thank you
Yao Jiewen

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao,
Jiewen
Sent: Wednesday, September 16, 2020 7:45 PM
To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Zurcher,
Christopher J <christopher.j.zurcher@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
<xiaoyux.lu@intel.com>
Subject: Re: [edk2-devel] [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP
(Envelope) Digest interface

Hi Laszlo
I disagree to put OPTIONAL for DigestValue, just because NULL is checked.
If we need follow this, then we need add OPTIONAL to almost all pointer,
which
is unnecessary.

+BOOLEAN
+EFIAPI
+EvpMdFinal (
+ IN OUT VOID *EvpMdContext,
+ OUT UINT8 *DigestValue
(3) DigestValue should be marked OPTIONAL in my opinion, as NULL is
deliberately permitted (for just freeing the context).




-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com>
Sent: Wednesday, September 16, 2020 7:07 PM
To: devel@edk2.groups.io; Zurcher, Christopher J
<christopher.j.zurcher@intel.com>
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
<jian.j.wang@intel.com>;
Lu, XiaoyuX <xiaoyux.lu@intel.com>
Subject: Re: [edk2-devel] [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP
(Envelope) Digest interface

Hello Christopher,

On 09/16/20 02:59, Zurcher, Christopher J wrote:
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2545

The EVP interface should be used in place of discrete digest function
calls.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
Signed-off-by: Christopher J Zurcher <christopher.j.zurcher@intel.com>
---
CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf | 1 +
CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf | 1 +
CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf | 1 +
CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf | 1 +
CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf | 1 +
CryptoPkg/Include/Library/BaseCryptLib.h | 129 ++++++++++
CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c | 257
++++++++++++++++++++
CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c | 128
++++++++++
CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c | 128
++++++++++
9 files changed, 647 insertions(+)
I agree that "CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c"
is necessary. (If I understand correctly, that file was missing from
your v2 posting.)

But "CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c" seems
superfluous. This file is never referenced in the INF files.

The point of this file would be to allow *some* of the Base / Pei /
Runtime / Smm instances to "stub out" the EVP MD functions (i.e. provide
only stub implementations). But this isn't what's happening -- all of
the Base / Pei / Runtime / Smm instances are getting the real deal
("CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c").

(1) So I think "CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c"
should be dropped. Only the Null instance of the library needs null
versions of the new functions. The Base / Pei / Runtime / Smm instances
don't.


diff --git a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
index 4aae2aba95..3968f29412 100644
--- a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
@@ -50,6 +50,7 @@
Pk/CryptAuthenticode.c
Pk/CryptTs.c
Pem/CryptPem.c
+ Evp/CryptEvpMd.c

SysCall/CrtWrapper.c
SysCall/TimerWrapper.c
diff --git a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
index dc28e3a11d..d0b91716d0 100644
--- a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
@@ -57,6 +57,7 @@
Pk/CryptTsNull.c
Pem/CryptPemNull.c
Rand/CryptRandNull.c
+ Evp/CryptEvpMd.c

SysCall/CrtWrapper.c
SysCall/ConstantTimeClock.c
diff --git a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
index 5005beed02..9f3accd35b 100644
--- a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
@@ -56,6 +56,7 @@
Pk/CryptAuthenticodeNull.c
Pk/CryptTsNull.c
Pem/CryptPem.c
+ Evp/CryptEvpMd.c

SysCall/CrtWrapper.c
SysCall/TimerWrapper.c
diff --git a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
index 91ec3e03bf..420623cdc6 100644
--- a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
@@ -54,6 +54,7 @@
Pk/CryptAuthenticodeNull.c
Pk/CryptTsNull.c
Pem/CryptPem.c
+ Evp/CryptEvpMd.c

SysCall/CrtWrapper.c
SysCall/ConstantTimeClock.c
diff --git a/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
b/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
index 689af4fedd..542ac2e2e1 100644
--- a/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
+++ b/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
@@ -50,6 +50,7 @@
Pk/CryptTsNull.c
Pem/CryptPemNull.c
Rand/CryptRandNull.c
+ Evp/CryptEvpMdNull.c

[Packages]
MdePkg/MdePkg.dec
diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h
b/CryptoPkg/Include/Library/BaseCryptLib.h
index ae9bde9e37..5e1b408b54 100644
--- a/CryptoPkg/Include/Library/BaseCryptLib.h
+++ b/CryptoPkg/Include/Library/BaseCryptLib.h
@@ -1012,6 +1012,135 @@ HmacSha256Final (
OUT UINT8 *HmacValue
);

+//==============================================================
=======================
+// EVP (Envelope) Primitive
+//==============================================================
=======================
+
+/**
+ Allocates and initializes one EVP_MD_CTX context for subsequent
EVP_MD
use.
+
+ If DigestName is NULL, then return FALSE.
+
+ @param[in] DigestName Pointer to the digest name as a NULL-
terminated ASCII string.
+ Valid digest names are:
+ MD5, SHA1, SHA224, SHA256, SHA384, SHA512
+ SHA3-224, SHA3-256, SHA3-384, SHA3-512
+ SM3
+
+ @return Pointer to the EVP_MD_CTX context that has been allocated
and
initialized.
+ If DigestName is invalid, returns NULL.
+ If the allocations fails, returns NULL.
+ If initialization fails, returns NULL.
+
+**/
+VOID *
+EFIAPI
+EvpMdInit (
+ IN CONST CHAR8 *DigestName
+ );
+
+/**
+ Makes a copy of an existing EVP_MD context.
+
+ If EvpMdContext is NULL, then return FALSE.
+ If NewEvpMdContext is NULL, then return FALSE.
+
+ @param[in] EvpMdContext Pointer to EVP_MD context being copied.
+ @param[out] NewEvpMdContext Pointer to new EVP_MD context.
+
+ @retval TRUE EVP_MD context copy succeeded.
+ @retval FALSE EVP_MD context copy failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdDuplicate (
+ IN CONST VOID *EvpMdContext,
+ OUT VOID *NewEvpMdContext
+ );
+
+/**
+ Digests the input data and updates EVP_MD context.
+
+ This function performs EVP digest on a data buffer of the specified size.
+ It can be called multiple times to compute the digest of long or
discontinuous data streams.
+ EVP_MD context should be already correctly initialized by EvpMdInit(),
and
should not
+ be finalized by EvpMdFinal(). Behavior with invalid context is undefined.
+
+ If EvpMdContext is NULL, then return FALSE.
+ If Data is NULL and DataSize is not zero, return FALSE.
+
+ @param[in, out] EvpMdContext Pointer to the EVP_MD context.
+ @param[in] Data Pointer to the buffer containing the data to
be
digested.
+ @param[in] DataSize Size of Data buffer in bytes.
+
+ @retval TRUE EVP data digest succeeded.
+ @retval FALSE EVP data digest failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdUpdate (
+ IN OUT VOID *EvpMdContext,
+ IN CONST VOID *Data,
+ IN UINTN DataSize
+ );
+
+/**
+ Completes computation of the EVP digest value.
+ Releases the specified EVP_MD_CTX context.
+
+ This function completes EVP hash computation and retrieves the digest
value into
+ the specified memory. After this function has been called, the EVP
context
cannot
+ be used again.
+ EVP context should be already correctly initialized by EvpMdInit(), and
should
+ not be finalized by EvpMdFinal(). Behavior with invalid EVP context is
undefined.
+
+ If EvpMdContext is NULL, then return FALSE.
+ If DigestValue is NULL, free the Context then return FALSE.
+
+ @param[in, out] EvpMdContext Pointer to the EVP context.
+ @param[out] Digest Pointer to a buffer that receives the EVP
digest
value.

(2) Please extend the comment on Digest with the following:

The caller is responsible for providing enough storage for the digest
algorithm selected with EvpMdInit(). Providing EVP_MAX_MD_SIZE bytes
will suffice for storing the digest regardless of the algorithm chosen
in EvpMdInit().

(EVP_MAX_MD_SIZE is a public OpenSSL macro and I think we should openly
advertise it to consumers in edk2.)

+
+ @retval TRUE EVP digest computation succeeded.
+ @retval FALSE EVP digest computation failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdFinal (
+ IN OUT VOID *EvpMdContext,
+ OUT UINT8 *DigestValue
(3) DigestValue should be marked OPTIONAL in my opinion, as NULL is
deliberately permitted (for just freeing the context).

+ );
+
+/**
+ Computes the message digest of an input data buffer.
+
+ This function performs the message digest of a given data buffer, and
places
+ the digest value into the specified memory.
+
+ If DigestName is NULL, return FALSE.
+ If Data is NULL and DataSize is not zero, return FALSE.
+ If HashValue is NULL, return FALSE.
+
+ @param[in] DigestName Pointer to the digest name.
+ @param[in] Data Pointer to the buffer containing the data to be
hashed.
+ @param[in] DataSize Size of Data buffer in bytes.
+ @param[out] HashValue Pointer to a buffer that receives the digest
value.
+
+ @retval TRUE Digest computation succeeded.
+ @retval FALSE Digest computation failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdHashAll (
+ IN CONST CHAR8 *DigestName,
+ IN CONST VOID *Data,
+ IN UINTN DataSize,
+ OUT UINT8 *HashValue
+ );
+
//===============================================================
======================
// Symmetric Cryptography Primitive
//===============================================================
======================
diff --git a/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
new file mode 100644
index 0000000000..b2770a9186
--- /dev/null
+++ b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
@@ -0,0 +1,257 @@
+/** @file
+ EVP MD Wrapper Implementation for OpenSSL.
+
+Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include "InternalCryptLib.h"
+#include <openssl/evp.h>
+
+/**
+ Allocates and initializes one EVP_MD_CTX context for subsequent
EVP_MD
use.
+
+ If DigestName is NULL, then return FALSE.
+
+ @param[in] DigestName Pointer to the digest name as a NULL-
terminated ASCII string.
+ Valid digest names are:
+ MD5, SHA1, SHA224, SHA256, SHA384, SHA512
+ SHA3-224, SHA3-256, SHA3-384, SHA3-512
+ SM3
+
+ @return Pointer to the EVP_MD_CTX context that has been allocated
and
initialized.
+ If DigestName is invalid, returns NULL.
+ If the allocations fails, returns NULL.
+ If initialization fails, returns NULL.
+
+**/
+VOID *
+EFIAPI
+EvpMdInit (
+ IN CONST CHAR8 *DigestName
+ )
+{
+ EVP_MD *Digest;
+ VOID *EvpMdContext;
+
+ //
+ // Check input parameters.
+ //
+ if (DigestName == NULL) {
+ return NULL;
+ }
+
+ //
+ // Allocate EVP_MD_CTX Context
+ //
+ EvpMdContext = EVP_MD_CTX_new ();
+ if (EvpMdContext == NULL) {
+ return NULL;
+ }
+
+ Digest = EVP_get_digestbyname (DigestName);
I think this may not compile with gcc (and correctly so). The pointer
returned by EVP_get_digestbyname() is const-qualified, but with the
assignment, we're throwing away the const-ness.

(4) Please const-qualify the "Digest" local pointer.

+ if (Digest == NULL) {
+ return NULL;
+ }
(5) This is a memory leak I believe; "EvpMdContext" is leaked.

For keeping the control flow simple, consider moving
EVP_get_digestbyname() above EVP_MD_CTX_new().

+
+ //
+ // Initialize Context
+ //
+ if (EVP_DigestInit_ex (EvpMdContext, Digest, NULL) != 1) {
+ EVP_MD_CTX_free (EvpMdContext);
+ return NULL;
+ }
+
+ return EvpMdContext;
+}
+
+/**
+ Makes a copy of an existing EVP_MD context.
+
+ If EvpMdContext is NULL, then return FALSE.
+ If NewEvpMdContext is NULL, then return FALSE.
+
+ @param[in] EvpMdContext Pointer to EVP_MD context being copied.
+ @param[out] NewEvpMdContext Pointer to new EVP_MD context.
+
+ @retval TRUE EVP_MD context copy succeeded.
+ @retval FALSE EVP_MD context copy failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdDuplicate (
+ IN CONST VOID *EvpMdContext,
+ OUT VOID *NewEvpMdContext
+ )
+{
+ //
+ // Check input parameters.
+ //
+ if (EvpMdContext == NULL || NewEvpMdContext == NULL) {
+ return FALSE;
+ }
+
+ if (EVP_MD_CTX_copy (NewEvpMdContext, EvpMdContext) != 1) {
+ return FALSE;
+ }
+
+ return TRUE;
+}
(6) Can you please confirm that the caller is supposed to initialize
"NewEvpMdContext" with EvpMdInit() first, before calling EvpMdDuplicate()?

+
+/**
+ Digests the input data and updates EVP_MD context.
+
+ This function performs EVP digest on a data buffer of the specified size.
+ It can be called multiple times to compute the digest of long or
discontinuous data streams.
+ EVP_MD context should be already correctly initialized by EvpMdInit(),
and
should not
+ be finalized by EvpMdFinal(). Behavior with invalid context is undefined.
+
+ If EvpMdContext is NULL, then return FALSE.
+ If Data is NULL and DataSize is not zero, return FALSE.
+
+ @param[in, out] EvpMdContext Pointer to the EVP_MD context.
+ @param[in] Data Pointer to the buffer containing the data to
be
digested.
+ @param[in] DataSize Size of Data buffer in bytes.
+
+ @retval TRUE EVP data digest succeeded.
+ @retval FALSE EVP data digest failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdUpdate (
+ IN OUT VOID *EvpMdContext,
+ IN CONST VOID *Data,
+ IN UINTN DataSize
+ )
+{
+ //
+ // Check input parameters.
+ //
+ if (EvpMdContext == NULL) {
+ return FALSE;
+ }
+
+ //
+ // Check invalid parameters, in case only DataLength was checked in
OpenSSL
+ //
+ if (Data == NULL && DataSize != 0) {
+ return FALSE;
+ }
+
+ //
+ // OpenSSL EVP digest update
+ //
+ if (EVP_DigestUpdate (EvpMdContext, Data, DataSize) != 1) {
+ return FALSE;
+ }
+
+ return TRUE;
+}
+
+/**
+ Completes computation of the EVP digest value.
+ Releases the specified EVP_MD_CTX context.
+
+ This function completes EVP hash computation and retrieves the digest
value into
+ the specified memory. After this function has been called, the EVP
context
cannot
+ be used again.
+ EVP context should be already correctly initialized by EvpMdInit(), and
should
+ not be finalized by EvpMdFinal(). Behavior with invalid EVP context is
undefined.
+
+ If EvpMdContext is NULL, then return FALSE.
+ If DigestValue is NULL, free the Context then return FALSE.
+
+ @param[in, out] EvpMdContext Pointer to the EVP context.
+ @param[out] Digest Pointer to a buffer that receives the EVP
digest
value.
+
+ @retval TRUE EVP digest computation succeeded.
+ @retval FALSE EVP digest computation failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdFinal (
+ IN OUT VOID *EvpMdContext,
+ OUT UINT8 *DigestValue
+ )
+{
+ UINT32 Length;
+ BOOLEAN ReturnValue;
+
+ ReturnValue = TRUE;
+
+ //
+ // Check input parameters.
+ //
+ if (EvpMdContext == NULL) {
+ return FALSE;
+ }
+ if (DigestValue == NULL) {
+ EVP_MD_CTX_free (EvpMdContext);
+ return FALSE;
+ }
+
+ //
+ // OpenSSL EVP digest finalization
+ //
+ if (EVP_DigestFinal_ex (EvpMdContext, DigestValue, &Length) != 1) {
+ ReturnValue = FALSE;
+ }

(7) I suggest dropping the "Length" local variable. EVP_DigestFinal_ex()
deals fine with the third parameter being NULL, according to the docs
(and the code).


+
+ //
+ // Free OpenSSL EVP_MD_CTX Context
+ //
+ EVP_MD_CTX_free (EvpMdContext);
+
+ return ReturnValue;
+}
+
+/**
+ Computes the message digest of an input data buffer.
+
+ This function performs the message digest of a given data buffer, and
places
+ the digest value into the specified memory.
+
+ If DigestName is NULL, return FALSE.
+ If Data is NULL and DataSize is not zero, return FALSE.
+ If HashValue is NULL, return FALSE.
+
+ @param[in] DigestName Pointer to the digest name.
+ @param[in] Data Pointer to the buffer containing the data to be
hashed.
+ @param[in] DataSize Size of Data buffer in bytes.
+ @param[out] HashValue Pointer to a buffer that receives the digest
value.
+
+ @retval TRUE Digest computation succeeded.
+ @retval FALSE Digest computation failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdHashAll (
+ IN CONST CHAR8 *DigestName,
+ IN CONST VOID *Data,
+ IN UINTN DataSize,
+ OUT UINT8 *HashValue
+ )
+{
+ BOOLEAN Result;
+ VOID *EvpMdContext;
+
+ EvpMdContext = EvpMdInit (DigestName);
+ if (EvpMdContext == NULL) {
+ return FALSE;
+ }
+
+ Result = EvpMdUpdate (EvpMdContext, Data, DataSize);
+ if (Result == FALSE) {
(8) Style: please write (!Result).


+ EvpMdFinal (EvpMdContext, NULL);
+ return FALSE;
+ }
+
+ Result = EvpMdFinal (EvpMdContext, HashValue);
+
+ return Result;
+}
diff --git a/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c
b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c
new file mode 100644
index 0000000000..038f63801f
--- /dev/null
+++ b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c
@@ -0,0 +1,128 @@
+/** @file
+ EVP MD Wrapper Null Library.
+
+Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include "InternalCryptLib.h"
+
+/**
+ Allocates and initializes one EVP_MD_CTX context for subsequent
EVP_MD
use.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in] DigestName Pointer to the digest name as a NULL-
terminated ASCII string.
+ Valid digest names are:
+ MD5, SHA1, SHA224, SHA256, SHA384, SHA512
+ SHA3-224, SHA3-256, SHA3-384, SHA3-512
+ SM3
+
+ @return NULL This interface is not supported.
+
+**/
+VOID *
+EFIAPI
+EvpMdInit (
+ IN CONST CHAR8 *DigestName
+ )
+{
+ ASSERT (FALSE);
+ return NULL;
+}
+
+/**
+ Makes a copy of an existing EVP_MD context.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in] EvpMdContext Pointer to EVP_MD context being copied.
+ @param[out] NewEvpMdContext Pointer to new EVP_MD context.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdDuplicate (
+ IN CONST VOID *EvpMdContext,
+ OUT VOID *NewEvpMdContext
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
+
+/**
+ Digests the input data and updates EVP_MD context.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in, out] EvpMdContext Pointer to the EVP_MD context.
+ @param[in] Data Pointer to the buffer containing the data to
be
digested.
+ @param[in] DataSize Size of Data buffer in bytes.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdUpdate (
+ IN OUT VOID *EvpMdContext,
+ IN CONST VOID *Data,
+ IN UINTN DataSize
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
+
+/**
+ Completes computation of the EVP digest value.
+ Releases the specified EVP_MD_CTX context.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in, out] EvpMdContext Pointer to the EVP context.
+ @param[out] Digest Pointer to a buffer that receives the EVP
digest
value.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdFinal (
+ IN OUT VOID *EvpMdContext,
+ OUT UINT8 *DigestValue
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
+
+/**
+ Computes the message digest of an input data buffer.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in] DigestName Pointer to the digest name.
+ @param[in] Data Pointer to the buffer containing the data to be
hashed.
+ @param[in] DataSize Size of Data buffer in bytes.
+ @param[out] HashValue Pointer to a buffer that receives the digest
value.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdHashAll (
+ IN CONST CHAR8 *DigestName,
+ IN CONST VOID *Data,
+ IN UINTN DataSize,
+ OUT UINT8 *HashValue
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
diff --git a/CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c
b/CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c
new file mode 100644
index 0000000000..038f63801f
--- /dev/null
+++ b/CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c
@@ -0,0 +1,128 @@
+/** @file
+ EVP MD Wrapper Null Library.
+
+Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include "InternalCryptLib.h"
+
+/**
+ Allocates and initializes one EVP_MD_CTX context for subsequent
EVP_MD
use.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in] DigestName Pointer to the digest name as a NULL-
terminated ASCII string.
+ Valid digest names are:
+ MD5, SHA1, SHA224, SHA256, SHA384, SHA512
+ SHA3-224, SHA3-256, SHA3-384, SHA3-512
+ SM3
+
+ @return NULL This interface is not supported.
+
+**/
+VOID *
+EFIAPI
+EvpMdInit (
+ IN CONST CHAR8 *DigestName
+ )
+{
+ ASSERT (FALSE);
+ return NULL;
+}
+
+/**
+ Makes a copy of an existing EVP_MD context.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in] EvpMdContext Pointer to EVP_MD context being copied.
+ @param[out] NewEvpMdContext Pointer to new EVP_MD context.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdDuplicate (
+ IN CONST VOID *EvpMdContext,
+ OUT VOID *NewEvpMdContext
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
+
+/**
+ Digests the input data and updates EVP_MD context.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in, out] EvpMdContext Pointer to the EVP_MD context.
+ @param[in] Data Pointer to the buffer containing the data to
be
digested.
+ @param[in] DataSize Size of Data buffer in bytes.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdUpdate (
+ IN OUT VOID *EvpMdContext,
+ IN CONST VOID *Data,
+ IN UINTN DataSize
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
+
+/**
+ Completes computation of the EVP digest value.
+ Releases the specified EVP_MD_CTX context.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in, out] EvpMdContext Pointer to the EVP context.
+ @param[out] Digest Pointer to a buffer that receives the EVP
digest
value.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdFinal (
+ IN OUT VOID *EvpMdContext,
+ OUT UINT8 *DigestValue
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
+
+/**
+ Computes the message digest of an input data buffer.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in] DigestName Pointer to the digest name.
+ @param[in] Data Pointer to the buffer containing the data to be
hashed.
+ @param[in] DataSize Size of Data buffer in bytes.
+ @param[out] HashValue Pointer to a buffer that receives the digest
value.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdHashAll (
+ IN CONST CHAR8 *DigestName,
+ IN CONST VOID *Data,
+ IN UINTN DataSize,
+ OUT UINT8 *HashValue
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
Thanks,
Laszlo



Re: [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP (Envelope) Digest interface

Laszlo Ersek
 

On 09/16/20 14:07, Yao, Jiewen wrote:
I overlooked the behavior that NULL DigestValue will take side effect to free Context.

Hi Christopher
May I understand why we need free the context?
This does not align with other existing HASH or HMAC functions.
I requested that.

It makes no sense to keep a finalized context.

In other words, it makes no sense to separate finalization from freeing.
A context that has been finalized is unusable for anything except freeing.

Thanks
Laszlo


Thank you
Yao Jiewen

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao, Jiewen
Sent: Wednesday, September 16, 2020 7:45 PM
To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Zurcher,
Christopher J <christopher.j.zurcher@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>
Subject: Re: [edk2-devel] [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP
(Envelope) Digest interface

Hi Laszlo
I disagree to put OPTIONAL for DigestValue, just because NULL is checked.
If we need follow this, then we need add OPTIONAL to almost all pointer, which
is unnecessary.

+BOOLEAN
+EFIAPI
+EvpMdFinal (
+ IN OUT VOID *EvpMdContext,
+ OUT UINT8 *DigestValue
(3) DigestValue should be marked OPTIONAL in my opinion, as NULL is
deliberately permitted (for just freeing the context).




-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com>
Sent: Wednesday, September 16, 2020 7:07 PM
To: devel@edk2.groups.io; Zurcher, Christopher J
<christopher.j.zurcher@intel.com>
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
<jian.j.wang@intel.com>;
Lu, XiaoyuX <xiaoyux.lu@intel.com>
Subject: Re: [edk2-devel] [PATCH v3 1/3] CryptoPkg/BaseCryptLib: Add EVP
(Envelope) Digest interface

Hello Christopher,

On 09/16/20 02:59, Zurcher, Christopher J wrote:
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2545

The EVP interface should be used in place of discrete digest function
calls.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
Signed-off-by: Christopher J Zurcher <christopher.j.zurcher@intel.com>
---
CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf | 1 +
CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf | 1 +
CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf | 1 +
CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf | 1 +
CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf | 1 +
CryptoPkg/Include/Library/BaseCryptLib.h | 129 ++++++++++
CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c | 257
++++++++++++++++++++
CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c | 128 ++++++++++
CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c | 128
++++++++++
9 files changed, 647 insertions(+)
I agree that "CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c"
is necessary. (If I understand correctly, that file was missing from
your v2 posting.)

But "CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c" seems
superfluous. This file is never referenced in the INF files.

The point of this file would be to allow *some* of the Base / Pei /
Runtime / Smm instances to "stub out" the EVP MD functions (i.e. provide
only stub implementations). But this isn't what's happening -- all of
the Base / Pei / Runtime / Smm instances are getting the real deal
("CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c").

(1) So I think "CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c"
should be dropped. Only the Null instance of the library needs null
versions of the new functions. The Base / Pei / Runtime / Smm instances
don't.


diff --git a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
index 4aae2aba95..3968f29412 100644
--- a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
@@ -50,6 +50,7 @@
Pk/CryptAuthenticode.c
Pk/CryptTs.c
Pem/CryptPem.c
+ Evp/CryptEvpMd.c

SysCall/CrtWrapper.c
SysCall/TimerWrapper.c
diff --git a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
index dc28e3a11d..d0b91716d0 100644
--- a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
@@ -57,6 +57,7 @@
Pk/CryptTsNull.c
Pem/CryptPemNull.c
Rand/CryptRandNull.c
+ Evp/CryptEvpMd.c

SysCall/CrtWrapper.c
SysCall/ConstantTimeClock.c
diff --git a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
index 5005beed02..9f3accd35b 100644
--- a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
@@ -56,6 +56,7 @@
Pk/CryptAuthenticodeNull.c
Pk/CryptTsNull.c
Pem/CryptPem.c
+ Evp/CryptEvpMd.c

SysCall/CrtWrapper.c
SysCall/TimerWrapper.c
diff --git a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
index 91ec3e03bf..420623cdc6 100644
--- a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
@@ -54,6 +54,7 @@
Pk/CryptAuthenticodeNull.c
Pk/CryptTsNull.c
Pem/CryptPem.c
+ Evp/CryptEvpMd.c

SysCall/CrtWrapper.c
SysCall/ConstantTimeClock.c
diff --git a/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
b/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
index 689af4fedd..542ac2e2e1 100644
--- a/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
+++ b/CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
@@ -50,6 +50,7 @@
Pk/CryptTsNull.c
Pem/CryptPemNull.c
Rand/CryptRandNull.c
+ Evp/CryptEvpMdNull.c

[Packages]
MdePkg/MdePkg.dec
diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h
b/CryptoPkg/Include/Library/BaseCryptLib.h
index ae9bde9e37..5e1b408b54 100644
--- a/CryptoPkg/Include/Library/BaseCryptLib.h
+++ b/CryptoPkg/Include/Library/BaseCryptLib.h
@@ -1012,6 +1012,135 @@ HmacSha256Final (
OUT UINT8 *HmacValue
);

+//==============================================================
=======================
+// EVP (Envelope) Primitive
+//==============================================================
=======================
+
+/**
+ Allocates and initializes one EVP_MD_CTX context for subsequent EVP_MD
use.
+
+ If DigestName is NULL, then return FALSE.
+
+ @param[in] DigestName Pointer to the digest name as a NULL-
terminated ASCII string.
+ Valid digest names are:
+ MD5, SHA1, SHA224, SHA256, SHA384, SHA512
+ SHA3-224, SHA3-256, SHA3-384, SHA3-512
+ SM3
+
+ @return Pointer to the EVP_MD_CTX context that has been allocated and
initialized.
+ If DigestName is invalid, returns NULL.
+ If the allocations fails, returns NULL.
+ If initialization fails, returns NULL.
+
+**/
+VOID *
+EFIAPI
+EvpMdInit (
+ IN CONST CHAR8 *DigestName
+ );
+
+/**
+ Makes a copy of an existing EVP_MD context.
+
+ If EvpMdContext is NULL, then return FALSE.
+ If NewEvpMdContext is NULL, then return FALSE.
+
+ @param[in] EvpMdContext Pointer to EVP_MD context being copied.
+ @param[out] NewEvpMdContext Pointer to new EVP_MD context.
+
+ @retval TRUE EVP_MD context copy succeeded.
+ @retval FALSE EVP_MD context copy failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdDuplicate (
+ IN CONST VOID *EvpMdContext,
+ OUT VOID *NewEvpMdContext
+ );
+
+/**
+ Digests the input data and updates EVP_MD context.
+
+ This function performs EVP digest on a data buffer of the specified size.
+ It can be called multiple times to compute the digest of long or
discontinuous data streams.
+ EVP_MD context should be already correctly initialized by EvpMdInit(), and
should not
+ be finalized by EvpMdFinal(). Behavior with invalid context is undefined.
+
+ If EvpMdContext is NULL, then return FALSE.
+ If Data is NULL and DataSize is not zero, return FALSE.
+
+ @param[in, out] EvpMdContext Pointer to the EVP_MD context.
+ @param[in] Data Pointer to the buffer containing the data to
be
digested.
+ @param[in] DataSize Size of Data buffer in bytes.
+
+ @retval TRUE EVP data digest succeeded.
+ @retval FALSE EVP data digest failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdUpdate (
+ IN OUT VOID *EvpMdContext,
+ IN CONST VOID *Data,
+ IN UINTN DataSize
+ );
+
+/**
+ Completes computation of the EVP digest value.
+ Releases the specified EVP_MD_CTX context.
+
+ This function completes EVP hash computation and retrieves the digest
value into
+ the specified memory. After this function has been called, the EVP context
cannot
+ be used again.
+ EVP context should be already correctly initialized by EvpMdInit(), and
should
+ not be finalized by EvpMdFinal(). Behavior with invalid EVP context is
undefined.
+
+ If EvpMdContext is NULL, then return FALSE.
+ If DigestValue is NULL, free the Context then return FALSE.
+
+ @param[in, out] EvpMdContext Pointer to the EVP context.
+ @param[out] Digest Pointer to a buffer that receives the EVP
digest
value.

(2) Please extend the comment on Digest with the following:

The caller is responsible for providing enough storage for the digest
algorithm selected with EvpMdInit(). Providing EVP_MAX_MD_SIZE bytes
will suffice for storing the digest regardless of the algorithm chosen
in EvpMdInit().

(EVP_MAX_MD_SIZE is a public OpenSSL macro and I think we should openly
advertise it to consumers in edk2.)

+
+ @retval TRUE EVP digest computation succeeded.
+ @retval FALSE EVP digest computation failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdFinal (
+ IN OUT VOID *EvpMdContext,
+ OUT UINT8 *DigestValue
(3) DigestValue should be marked OPTIONAL in my opinion, as NULL is
deliberately permitted (for just freeing the context).

+ );
+
+/**
+ Computes the message digest of an input data buffer.
+
+ This function performs the message digest of a given data buffer, and
places
+ the digest value into the specified memory.
+
+ If DigestName is NULL, return FALSE.
+ If Data is NULL and DataSize is not zero, return FALSE.
+ If HashValue is NULL, return FALSE.
+
+ @param[in] DigestName Pointer to the digest name.
+ @param[in] Data Pointer to the buffer containing the data to be
hashed.
+ @param[in] DataSize Size of Data buffer in bytes.
+ @param[out] HashValue Pointer to a buffer that receives the digest
value.
+
+ @retval TRUE Digest computation succeeded.
+ @retval FALSE Digest computation failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdHashAll (
+ IN CONST CHAR8 *DigestName,
+ IN CONST VOID *Data,
+ IN UINTN DataSize,
+ OUT UINT8 *HashValue
+ );
+
//===============================================================
======================
// Symmetric Cryptography Primitive
//===============================================================
======================
diff --git a/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
new file mode 100644
index 0000000000..b2770a9186
--- /dev/null
+++ b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMd.c
@@ -0,0 +1,257 @@
+/** @file
+ EVP MD Wrapper Implementation for OpenSSL.
+
+Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include "InternalCryptLib.h"
+#include <openssl/evp.h>
+
+/**
+ Allocates and initializes one EVP_MD_CTX context for subsequent EVP_MD
use.
+
+ If DigestName is NULL, then return FALSE.
+
+ @param[in] DigestName Pointer to the digest name as a NULL-
terminated ASCII string.
+ Valid digest names are:
+ MD5, SHA1, SHA224, SHA256, SHA384, SHA512
+ SHA3-224, SHA3-256, SHA3-384, SHA3-512
+ SM3
+
+ @return Pointer to the EVP_MD_CTX context that has been allocated and
initialized.
+ If DigestName is invalid, returns NULL.
+ If the allocations fails, returns NULL.
+ If initialization fails, returns NULL.
+
+**/
+VOID *
+EFIAPI
+EvpMdInit (
+ IN CONST CHAR8 *DigestName
+ )
+{
+ EVP_MD *Digest;
+ VOID *EvpMdContext;
+
+ //
+ // Check input parameters.
+ //
+ if (DigestName == NULL) {
+ return NULL;
+ }
+
+ //
+ // Allocate EVP_MD_CTX Context
+ //
+ EvpMdContext = EVP_MD_CTX_new ();
+ if (EvpMdContext == NULL) {
+ return NULL;
+ }
+
+ Digest = EVP_get_digestbyname (DigestName);
I think this may not compile with gcc (and correctly so). The pointer
returned by EVP_get_digestbyname() is const-qualified, but with the
assignment, we're throwing away the const-ness.

(4) Please const-qualify the "Digest" local pointer.

+ if (Digest == NULL) {
+ return NULL;
+ }
(5) This is a memory leak I believe; "EvpMdContext" is leaked.

For keeping the control flow simple, consider moving
EVP_get_digestbyname() above EVP_MD_CTX_new().

+
+ //
+ // Initialize Context
+ //
+ if (EVP_DigestInit_ex (EvpMdContext, Digest, NULL) != 1) {
+ EVP_MD_CTX_free (EvpMdContext);
+ return NULL;
+ }
+
+ return EvpMdContext;
+}
+
+/**
+ Makes a copy of an existing EVP_MD context.
+
+ If EvpMdContext is NULL, then return FALSE.
+ If NewEvpMdContext is NULL, then return FALSE.
+
+ @param[in] EvpMdContext Pointer to EVP_MD context being copied.
+ @param[out] NewEvpMdContext Pointer to new EVP_MD context.
+
+ @retval TRUE EVP_MD context copy succeeded.
+ @retval FALSE EVP_MD context copy failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdDuplicate (
+ IN CONST VOID *EvpMdContext,
+ OUT VOID *NewEvpMdContext
+ )
+{
+ //
+ // Check input parameters.
+ //
+ if (EvpMdContext == NULL || NewEvpMdContext == NULL) {
+ return FALSE;
+ }
+
+ if (EVP_MD_CTX_copy (NewEvpMdContext, EvpMdContext) != 1) {
+ return FALSE;
+ }
+
+ return TRUE;
+}
(6) Can you please confirm that the caller is supposed to initialize
"NewEvpMdContext" with EvpMdInit() first, before calling EvpMdDuplicate()?

+
+/**
+ Digests the input data and updates EVP_MD context.
+
+ This function performs EVP digest on a data buffer of the specified size.
+ It can be called multiple times to compute the digest of long or
discontinuous data streams.
+ EVP_MD context should be already correctly initialized by EvpMdInit(), and
should not
+ be finalized by EvpMdFinal(). Behavior with invalid context is undefined.
+
+ If EvpMdContext is NULL, then return FALSE.
+ If Data is NULL and DataSize is not zero, return FALSE.
+
+ @param[in, out] EvpMdContext Pointer to the EVP_MD context.
+ @param[in] Data Pointer to the buffer containing the data to
be
digested.
+ @param[in] DataSize Size of Data buffer in bytes.
+
+ @retval TRUE EVP data digest succeeded.
+ @retval FALSE EVP data digest failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdUpdate (
+ IN OUT VOID *EvpMdContext,
+ IN CONST VOID *Data,
+ IN UINTN DataSize
+ )
+{
+ //
+ // Check input parameters.
+ //
+ if (EvpMdContext == NULL) {
+ return FALSE;
+ }
+
+ //
+ // Check invalid parameters, in case only DataLength was checked in
OpenSSL
+ //
+ if (Data == NULL && DataSize != 0) {
+ return FALSE;
+ }
+
+ //
+ // OpenSSL EVP digest update
+ //
+ if (EVP_DigestUpdate (EvpMdContext, Data, DataSize) != 1) {
+ return FALSE;
+ }
+
+ return TRUE;
+}
+
+/**
+ Completes computation of the EVP digest value.
+ Releases the specified EVP_MD_CTX context.
+
+ This function completes EVP hash computation and retrieves the digest
value into
+ the specified memory. After this function has been called, the EVP context
cannot
+ be used again.
+ EVP context should be already correctly initialized by EvpMdInit(), and
should
+ not be finalized by EvpMdFinal(). Behavior with invalid EVP context is
undefined.
+
+ If EvpMdContext is NULL, then return FALSE.
+ If DigestValue is NULL, free the Context then return FALSE.
+
+ @param[in, out] EvpMdContext Pointer to the EVP context.
+ @param[out] Digest Pointer to a buffer that receives the EVP
digest
value.
+
+ @retval TRUE EVP digest computation succeeded.
+ @retval FALSE EVP digest computation failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdFinal (
+ IN OUT VOID *EvpMdContext,
+ OUT UINT8 *DigestValue
+ )
+{
+ UINT32 Length;
+ BOOLEAN ReturnValue;
+
+ ReturnValue = TRUE;
+
+ //
+ // Check input parameters.
+ //
+ if (EvpMdContext == NULL) {
+ return FALSE;
+ }
+ if (DigestValue == NULL) {
+ EVP_MD_CTX_free (EvpMdContext);
+ return FALSE;
+ }
+
+ //
+ // OpenSSL EVP digest finalization
+ //
+ if (EVP_DigestFinal_ex (EvpMdContext, DigestValue, &Length) != 1) {
+ ReturnValue = FALSE;
+ }

(7) I suggest dropping the "Length" local variable. EVP_DigestFinal_ex()
deals fine with the third parameter being NULL, according to the docs
(and the code).


+
+ //
+ // Free OpenSSL EVP_MD_CTX Context
+ //
+ EVP_MD_CTX_free (EvpMdContext);
+
+ return ReturnValue;
+}
+
+/**
+ Computes the message digest of an input data buffer.
+
+ This function performs the message digest of a given data buffer, and
places
+ the digest value into the specified memory.
+
+ If DigestName is NULL, return FALSE.
+ If Data is NULL and DataSize is not zero, return FALSE.
+ If HashValue is NULL, return FALSE.
+
+ @param[in] DigestName Pointer to the digest name.
+ @param[in] Data Pointer to the buffer containing the data to be
hashed.
+ @param[in] DataSize Size of Data buffer in bytes.
+ @param[out] HashValue Pointer to a buffer that receives the digest
value.
+
+ @retval TRUE Digest computation succeeded.
+ @retval FALSE Digest computation failed.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdHashAll (
+ IN CONST CHAR8 *DigestName,
+ IN CONST VOID *Data,
+ IN UINTN DataSize,
+ OUT UINT8 *HashValue
+ )
+{
+ BOOLEAN Result;
+ VOID *EvpMdContext;
+
+ EvpMdContext = EvpMdInit (DigestName);
+ if (EvpMdContext == NULL) {
+ return FALSE;
+ }
+
+ Result = EvpMdUpdate (EvpMdContext, Data, DataSize);
+ if (Result == FALSE) {
(8) Style: please write (!Result).


+ EvpMdFinal (EvpMdContext, NULL);
+ return FALSE;
+ }
+
+ Result = EvpMdFinal (EvpMdContext, HashValue);
+
+ return Result;
+}
diff --git a/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c
b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c
new file mode 100644
index 0000000000..038f63801f
--- /dev/null
+++ b/CryptoPkg/Library/BaseCryptLib/Evp/CryptEvpMdNull.c
@@ -0,0 +1,128 @@
+/** @file
+ EVP MD Wrapper Null Library.
+
+Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include "InternalCryptLib.h"
+
+/**
+ Allocates and initializes one EVP_MD_CTX context for subsequent EVP_MD
use.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in] DigestName Pointer to the digest name as a NULL-
terminated ASCII string.
+ Valid digest names are:
+ MD5, SHA1, SHA224, SHA256, SHA384, SHA512
+ SHA3-224, SHA3-256, SHA3-384, SHA3-512
+ SM3
+
+ @return NULL This interface is not supported.
+
+**/
+VOID *
+EFIAPI
+EvpMdInit (
+ IN CONST CHAR8 *DigestName
+ )
+{
+ ASSERT (FALSE);
+ return NULL;
+}
+
+/**
+ Makes a copy of an existing EVP_MD context.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in] EvpMdContext Pointer to EVP_MD context being copied.
+ @param[out] NewEvpMdContext Pointer to new EVP_MD context.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdDuplicate (
+ IN CONST VOID *EvpMdContext,
+ OUT VOID *NewEvpMdContext
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
+
+/**
+ Digests the input data and updates EVP_MD context.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in, out] EvpMdContext Pointer to the EVP_MD context.
+ @param[in] Data Pointer to the buffer containing the data to
be
digested.
+ @param[in] DataSize Size of Data buffer in bytes.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdUpdate (
+ IN OUT VOID *EvpMdContext,
+ IN CONST VOID *Data,
+ IN UINTN DataSize
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
+
+/**
+ Completes computation of the EVP digest value.
+ Releases the specified EVP_MD_CTX context.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in, out] EvpMdContext Pointer to the EVP context.
+ @param[out] Digest Pointer to a buffer that receives the EVP
digest
value.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdFinal (
+ IN OUT VOID *EvpMdContext,
+ OUT UINT8 *DigestValue
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
+
+/**
+ Computes the message digest of an input data buffer.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in] DigestName Pointer to the digest name.
+ @param[in] Data Pointer to the buffer containing the data to be
hashed.
+ @param[in] DataSize Size of Data buffer in bytes.
+ @param[out] HashValue Pointer to a buffer that receives the digest
value.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdHashAll (
+ IN CONST CHAR8 *DigestName,
+ IN CONST VOID *Data,
+ IN UINTN DataSize,
+ OUT UINT8 *HashValue
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
diff --git a/CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c
b/CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c
new file mode 100644
index 0000000000..038f63801f
--- /dev/null
+++ b/CryptoPkg/Library/BaseCryptLibNull/Evp/CryptEvpMdNull.c
@@ -0,0 +1,128 @@
+/** @file
+ EVP MD Wrapper Null Library.
+
+Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include "InternalCryptLib.h"
+
+/**
+ Allocates and initializes one EVP_MD_CTX context for subsequent EVP_MD
use.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in] DigestName Pointer to the digest name as a NULL-
terminated ASCII string.
+ Valid digest names are:
+ MD5, SHA1, SHA224, SHA256, SHA384, SHA512
+ SHA3-224, SHA3-256, SHA3-384, SHA3-512
+ SM3
+
+ @return NULL This interface is not supported.
+
+**/
+VOID *
+EFIAPI
+EvpMdInit (
+ IN CONST CHAR8 *DigestName
+ )
+{
+ ASSERT (FALSE);
+ return NULL;
+}
+
+/**
+ Makes a copy of an existing EVP_MD context.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in] EvpMdContext Pointer to EVP_MD context being copied.
+ @param[out] NewEvpMdContext Pointer to new EVP_MD context.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdDuplicate (
+ IN CONST VOID *EvpMdContext,
+ OUT VOID *NewEvpMdContext
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
+
+/**
+ Digests the input data and updates EVP_MD context.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in, out] EvpMdContext Pointer to the EVP_MD context.
+ @param[in] Data Pointer to the buffer containing the data to
be
digested.
+ @param[in] DataSize Size of Data buffer in bytes.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdUpdate (
+ IN OUT VOID *EvpMdContext,
+ IN CONST VOID *Data,
+ IN UINTN DataSize
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
+
+/**
+ Completes computation of the EVP digest value.
+ Releases the specified EVP_MD_CTX context.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in, out] EvpMdContext Pointer to the EVP context.
+ @param[out] Digest Pointer to a buffer that receives the EVP
digest
value.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdFinal (
+ IN OUT VOID *EvpMdContext,
+ OUT UINT8 *DigestValue
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
+
+/**
+ Computes the message digest of an input data buffer.
+
+ Return FALSE to indicate this interface is not supported.
+
+ @param[in] DigestName Pointer to the digest name.
+ @param[in] Data Pointer to the buffer containing the data to be
hashed.
+ @param[in] DataSize Size of Data buffer in bytes.
+ @param[out] HashValue Pointer to a buffer that receives the digest
value.
+
+ @retval FALSE This interface is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+EvpMdHashAll (
+ IN CONST CHAR8 *DigestName,
+ IN CONST VOID *Data,
+ IN UINTN DataSize,
+ OUT UINT8 *HashValue
+ )
+{
+ ASSERT (FALSE);
+ return FALSE;
+}
Thanks,
Laszlo


Re: development process failure [was: remove TPM related ppi from Depex for Fsp wrapper PEIM driver]

Yao, Jiewen
 

Right. Now I understand why this is not enforced before - We do support Unicode in some content.

Unfortunately, I don’t think you can enforce the contributor's keyboard device. We sometime write different document with different context, and switch between English and other language. It is not feasible.

We do out best, but we still make mistakes.

I still prefer a tool to do some simple check. For example, dash "-", colon ":", quote "\"", bracket "()" - per my experience.

If you really only care about "REF: ", then we can only add check for "REF :", together with "Cc:", "Reviewed-by:", "Signed-off-by:", etc, to ensure they are using correct ":".

Thank you
Yao Jiewen

-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com>
Sent: Wednesday, September 16, 2020 8:14 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>
Cc: devel@edk2.groups.io; Zhang, Qi1 <qi1.zhang@intel.com>; Desimone,
Nathaniel L <nathaniel.l.desimone@intel.com>; Zeng, Star
<star.zeng@intel.com>; Wang, Jian J <jian.j.wang@intel.com>
Subject: Re: development process failure [was: remove TPM related ppi from
Depex for Fsp wrapper PEIM driver]

On 09/16/20 11:31, Yao, Jiewen wrote:
Hi Laszlo
Thanks. I agree 1, 2, 3. I take the blame. It is my fault.

For 4, it is out of my scope. I cannot find this by my eyes. Everything works
well on my side.
Can we improve patch checker to catch this in CI ?
I don’t think I can find any Unicode in code or commit message easily.
I prefer to let a tool to do that work.
Yes, we could perhaps enhance "BaseTools/Scripts/PatchCheck.py" to
require subjects to be 7-bit ASCII only. (And then some people would
disagree...)

I guess the idea is, unless it's a proper name being inserted in the
subject line, we should stick with 7-bit ASCII. For example, we should
reject U+FF1A (because U+003A is the right code point), but we should
still accept proper names in full UTF-8 (maybe not even restricted to
Latin script only)!

I don't know how this could be implemented in "PatchCheck.py"...

I guess what I would prefer is if contributors' input devices were
configured accordingly. When they press a key that promises to insert a
colon -- that is, when it *looks like* a colon --, then the symbol
should *become* a colon -- that is, U+003A, and not U+FF1A.

Thanks,
Laszlo


-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com>
Sent: Wednesday, September 16, 2020 4:43 PM
To: Chiu, Chasel <chasel.chiu@intel.com>; Yao, Jiewen
<jiewen.yao@intel.com>
Cc: devel@edk2.groups.io; Zhang, Qi1 <qi1.zhang@intel.com>; Desimone,
Nathaniel L <nathaniel.l.desimone@intel.com>; Zeng, Star
<star.zeng@intel.com>; Wang, Jian J <jian.j.wang@intel.com>
Subject: development process failure [was: remove TPM related ppi from
Depex
for Fsp wrapper PEIM driver]

Jiewen, Chasel,

On 09/15/20 08:21, Qi Zhang wrote:
Some open board are TPM disabled. So the boot may hang because
these PPIs can't arrive. And gEdkiiTcgPpiGuid will be notified where
it is used. So we need to remove these PPIs from Depex for Fsp wrapper
PEI and PeiTpmMeasurementLib.

Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>

Qi Zhang (2):
IntelFsp2WrapperPkg: remove gPeiTpmInitializationDonePpiGuid from
Depex
SecurityPkg/PeiTpmMeasurementLib: remove gEfiTpmDeviceSelectedGuid

IntelFsp2WrapperPkg/FspmWrapperPeim/FspmWrapperPeim.inf | 3 +--
IntelFsp2WrapperPkg/FspsWrapperPeim/FspsWrapperPeim.inf | 3 +--
.../Library/PeiTpmMeasurementLib/PeiTpmMeasurementLib.inf | 3 +--
3 files changed, 3 insertions(+), 6 deletions(-)
Please adopt a *much more* disciplined approach when merging patch series.


(1) When you merge a patch set, please report back on the list. Identify
both the pull request URL, and the commit reange.

In this case, the pull request was

https://github.com/tianocore/edk2/pull/930

and the commit range is a62fb4229d14..7bcb021a6d54.


(2) The associated Bugzilla:

https://bugzilla.tianocore.org/show_bug.cgi?id=2963

has been completely neglected, by both submitter and maintainers.

- The original BZ report is *absolute trash*.

- No URL into the mailing list archive has been captured in the BZ,
about the posted series.

- The BZ status is still CONFIRMED.

- No mention of the pull request, or the resultant commit, range in the
BZ ticket.


(3) The github pull request at
<https://github.com/tianocore/edk2/pull/930> does contain *any*
indication of the bugzilla ticket, or the cover letter on the list.

Basically we have random artifacts in three different places (Bugzilla,
github.com, mailing list), and nobody of the involved parties
(reviewers, maintainers, constributors) on this patch set have made
*any* effort to cross-reference them. We now have to hunt down
everything separately.


(4) Worst of all, the subject line of commit 414d7d11e6ea contains a
Unicode code point called FULLWIDTH COLON (U+FF1A) rather than a normal
colon (U+003A).

Compare:

- bad (current): IntelFsp2WrapperPkg: remove [...]
- good (should have been): IntelFsp2WrapperPkg: remove [...]

It makes absolutely no sense to use non-ASCII code points in subject
lines, for something as trivial as a colon.


I've been here for 8-9 years now and it's incredibly frustrating that I
*still* have to whine about basic stuff like this on a regular basis.

I don't even know whom I should CC at Intel (management or otherwise) to
see an improvement in attitude here.

I guess this community cannot be saved.

Laszlo

13101 - 13120 of 78392