From: "Michael D Kinney" <michael.d.kinney@intel.com>
To: "Sukerkar, Amol N" <amol.n.sukerkar@intel.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>,
"Kinney, Michael D" <michael.d.kinney@intel.com>
Cc: "Yao, Jiewen" <jiewen.yao@intel.com>,
"Wang, Jian J" <jian.j.wang@intel.com>,
"Agrawal, Sachin" <sachin.agrawal@intel.com>,
"Gao, Liming" <liming.gao@intel.com>
Subject: Re: [PATCH v2 1/2] CryptoPkg/BaseHashApiLib: Align BaseHashApiLib with TPM 2.0 Implementation
Date: Fri, 14 Feb 2020 22:29:37 +0000 [thread overview]
Message-ID: <E92EE9817A31E24EB0585FDF735412F5B9EBA55E@ORSMSX113.amr.corp.intel.com> (raw)
In-Reply-To: <20200214180545.5872-2-amol.n.sukerkar@intel.com>
Amol,
Comments included below.
Mike
> -----Original Message-----
> From: Sukerkar, Amol N <amol.n.sukerkar@intel.com>
> Sent: Friday, February 14, 2020 10:06 AM
> To: devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>;
> Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Agrawal, Sachin
> <sachin.agrawal@intel.com>; Gao, Liming
> <liming.gao@intel.com>
> Subject: [PATCH v2 1/2] CryptoPkg/BaseHashApiLib: Align
> BaseHashApiLib with TPM 2.0 Implementation
>
> Ref:
> https://bugzilla.tianocore.org/show_bug.cgi?id=2511
>
> This commit aligns the baseHashApiLib with TPM 2.0
> Implementation
> as follows:
> - Remove reference to MD4 and MD5 algorithms as they
> are deprecated
> - Align the enumerations for hashing algoerithms with
> the one used
> in TPM 2.0 implementation defined in
> IndustryStandard/Tpm20.h.
>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Signed-off-by: Amol N Sukerkar
> <amol.n.sukerkar@intel.com>
> ---
>
> Notes:
> v2
> - Fixed closed parentheses in commit message
>
> CryptoPkg/Library/BaseHashApiLib/BaseHashApiLib.c |
> 120 ++++++--------------
> CryptoPkg/CryptoPkg.dec |
> 16 ++-
> CryptoPkg/CryptoPkg.uni |
> 12 +-
> CryptoPkg/Include/Library/HashApiLib.h |
> 14 +--
> 4 files changed, 50 insertions(+), 112 deletions(-)
>
> diff --git
> a/CryptoPkg/Library/BaseHashApiLib/BaseHashApiLib.c
> b/CryptoPkg/Library/BaseHashApiLib/BaseHashApiLib.c
> index 277ef9f0b421..b87a82b06ce1 100644
> --- a/CryptoPkg/Library/BaseHashApiLib/BaseHashApiLib.c
> +++ b/CryptoPkg/Library/BaseHashApiLib/BaseHashApiLib.c
> @@ -31,32 +31,24 @@ HashApiGetContextSize (
> VOID
> )
> {
> - switch (PcdGet8 (PcdHashApiLibPolicy)) {
> - case HASH_API_ALGO_MD4:
> - return Md4GetContextSize ();
> - break;
> -
> - case HASH_API_ALGO_MD5:
> - return Md5GetContextSize ();
> - break;
> -
> - case HASH_API_ALGO_SHA1:
> + switch (PcdGet16 (PcdHashApiLibPolicy)) {
> + case HASH_ALG_SHA1:
> return Sha1GetContextSize ();
> break;
>
> - case HASH_API_ALGO_SHA256:
> + case HASH_ALG_SHA256:
> return Sha256GetContextSize ();
> break;
>
> - case HASH_API_ALGO_SHA384:
> + case HASH_ALG_SHA384:
> return Sha384GetContextSize ();
> break;
>
> - case HASH_API_ALGO_SHA512:
> + case HASH_ALG_SHA512:
> return Sha512GetContextSize ();
> break;
>
> - case HASH_API_ALGO_SM3_256:
> + case HASH_ALG_SM3_256:
> return Sm3GetContextSize ();
> break;
>
> @@ -81,32 +73,24 @@ HashApiInit (
> OUT HASH_API_CONTEXT HashContext
> )
> {
> - switch (PcdGet8 (PcdHashApiLibPolicy)) {
> - case HASH_API_ALGO_MD4:
> - return Md4Init (HashContext);
> - break;
> -
> - case HASH_API_ALGO_MD5:
> - return Md5Init (HashContext);
> - break;
> -
> - case HASH_API_ALGO_SHA1:
> + switch (PcdGet16 (PcdHashApiLibPolicy)) {
> + case HASH_ALG_SHA1:
> return Sha1Init (HashContext);
> break;
>
> - case HASH_API_ALGO_SHA256:
> + case HASH_ALG_SHA256:
> return Sha256Init (HashContext);
> break;
>
> - case HASH_API_ALGO_SHA384:
> + case HASH_ALG_SHA384:
> return Sha384Init (HashContext);
> break;
>
> - case HASH_API_ALGO_SHA512:
> + case HASH_ALG_SHA512:
> return Sha512Init (HashContext);
> break;
>
> - case HASH_API_ALGO_SM3_256:
> + case HASH_ALG_SM3_256:
> return Sm3Init (HashContext);
> break;
>
> @@ -133,32 +117,24 @@ HashApiDuplicate (
> OUT HASH_API_CONTEXT NewHashContext
> )
> {
> - switch (PcdGet8 (PcdHashApiLibPolicy)) {
> - case HASH_API_ALGO_MD4:
> - return Md4Duplicate (HashContext,
> NewHashContext);
> - break;
> -
> - case HASH_API_ALGO_MD5:
> - return Md5Duplicate (HashContext,
> NewHashContext);
> - break;
> -
> - case HASH_API_ALGO_SHA1:
> + switch (PcdGet16 (PcdHashApiLibPolicy)) {
> + case HASH_ALG_SHA1:
> return Sha1Duplicate (HashContext,
> NewHashContext);
> break;
>
> - case HASH_API_ALGO_SHA256:
> + case HASH_ALG_SHA256:
> return Sha256Duplicate (HashContext,
> NewHashContext);
> break;
>
> - case HASH_API_ALGO_SHA384:
> + case HASH_ALG_SHA384:
> return Sha384Duplicate (HashContext,
> NewHashContext);
> break;
>
> - case HASH_API_ALGO_SHA512:
> + case HASH_ALG_SHA512:
> return Sha512Duplicate (HashContext,
> NewHashContext);
> break;
>
> - case HASH_API_ALGO_SM3_256:
> + case HASH_ALG_SM3_256:
> return Sm3Duplicate (HashContext,
> NewHashContext);
> break;
>
> @@ -187,32 +163,24 @@ HashApiUpdate (
> IN UINTN DataToHashLen
> )
> {
> - switch (PcdGet8 (PcdHashApiLibPolicy)) {
> - case HASH_API_ALGO_MD4:
> - return Md4Update (HashContext, DataToHash,
> DataToHashLen);
> - break;
> -
> - case HASH_API_ALGO_MD5:
> - return Md5Update (HashContext, DataToHash,
> DataToHashLen);
> - break;
> -
> - case HASH_API_ALGO_SHA1:
> + switch (PcdGet16 (PcdHashApiLibPolicy)) {
> + case HASH_ALG_SHA1:
> return Sha1Update (HashContext, DataToHash,
> DataToHashLen);
> break;
>
> - case HASH_API_ALGO_SHA256:
> + case HASH_ALG_SHA256:
> return Sha256Update (HashContext, DataToHash,
> DataToHashLen);
> break;
>
> - case HASH_API_ALGO_SHA384:
> + case HASH_ALG_SHA384:
> return Sha384Update (HashContext, DataToHash,
> DataToHashLen);
> break;
>
> - case HASH_API_ALGO_SHA512:
> + case HASH_ALG_SHA512:
> return Sha512Update (HashContext, DataToHash,
> DataToHashLen);
> break;
>
> - case HASH_API_ALGO_SM3_256:
> + case HASH_ALG_SM3_256:
> return Sm3Update (HashContext, DataToHash,
> DataToHashLen);
> break;
>
> @@ -239,32 +207,24 @@ HashApiFinal (
> OUT UINT8 *Digest
> )
> {
> - switch (PcdGet8 (PcdHashApiLibPolicy)) {
> - case HASH_API_ALGO_MD4:
> - return Md4Final (HashContext, Digest);
> - break;
> -
> - case HASH_API_ALGO_MD5:
> - return Md5Final (HashContext, Digest);
> - break;
> -
> - case HASH_API_ALGO_SHA1:
> + switch (PcdGet16 (PcdHashApiLibPolicy)) {
> + case HASH_ALG_SHA1:
> return Sha1Final (HashContext, Digest);
> break;
>
> - case HASH_API_ALGO_SHA256:
> + case HASH_ALG_SHA256:
> return Sha256Final (HashContext, Digest);
> break;
>
> - case HASH_API_ALGO_SHA384:
> + case HASH_ALG_SHA384:
> return Sha384Final (HashContext, Digest);
> break;
>
> - case HASH_API_ALGO_SHA512:
> + case HASH_ALG_SHA512:
> return Sha512Final (HashContext, Digest);
> break;
>
> - case HASH_API_ALGO_SM3_256:
> + case HASH_ALG_SM3_256:
> return Sm3Final (HashContext, Digest);
> break;
>
> @@ -293,32 +253,24 @@ HashApiHashAll (
> OUT UINT8 *Digest
> )
> {
> - switch (PcdGet8 (PcdHashApiLibPolicy)) {
> - case HASH_API_ALGO_MD4:
> - return Md4HashAll (DataToHash, DataToHashLen,
> Digest);
> - break;
> -
> - case HASH_API_ALGO_MD5:
> - return Md5HashAll (DataToHash, DataToHashLen,
> Digest);
> - break;
> -
> - case HASH_API_ALGO_SHA1:
> + switch (PcdGet16 (PcdHashApiLibPolicy)) {
> + case HASH_ALG_SHA1:
> return Sha1HashAll (DataToHash, DataToHashLen,
> Digest);
> break;
>
> - case HASH_API_ALGO_SHA256:
> + case HASH_ALG_SHA256:
> return Sha256HashAll (DataToHash, DataToHashLen,
> Digest);
> break;
>
> - case HASH_API_ALGO_SHA384:
> + case HASH_ALG_SHA384:
> return Sha384HashAll (DataToHash, DataToHashLen,
> Digest);
> break;
>
> - case HASH_API_ALGO_SHA512:
> + case HASH_ALG_SHA512:
> return Sha512HashAll (DataToHash, DataToHashLen,
> Digest);
> break;
>
> - case HASH_API_ALGO_SM3_256:
> + case HASH_ALG_SM3_256:
> return Sm3HashAll (DataToHash, DataToHashLen,
> Digest);
> break;
>
> diff --git a/CryptoPkg/CryptoPkg.dec
> b/CryptoPkg/CryptoPkg.dec
> index 8bd63a76dd22..f185bcc82515 100644
> --- a/CryptoPkg/CryptoPkg.dec
> +++ b/CryptoPkg/CryptoPkg.dec
> @@ -74,16 +74,14 @@ [PcdsFixedAtBuild,
> PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
> # Based on the value set, the required algorithm is
> chosen to calculate
> # the hash of data.<BR>
> # The default hashing algorithm for BaseHashApiLib
> is set to SHA256.<BR>
> - # 0x00000001 - MD4.<BR>
> - # 0x00000002 - MD5.<BR>
> - # 0x00000003 - SHA1.<BR>
> - # 0x00000004 - SHA256.<BR>
> - # 0x00000005 - SHA384.<BR>
> - # 0x00000006 - SHA512.<BR>
> - # 0x00000007 - SM3_256.<BR>
> + # 0x00000001 - SHA1.<BR>
> + # 0x00000002 - SHA256.<BR>
> + # 0x00000004 - SHA384.<BR>
> + # 0x00000008 - SHA512.<BR>
> + # 0x00000010 - SM3_256.<BR>
Update the names to match the define names in Tpm20.h
such as HASH_ALG_SHA256.
> # @Prompt Set policy for hashing unsigned image for
> Secure Boot.
> - # @ValidRange 0x80000001 | 0x00000001 - 0x00000007
> -
> gEfiCryptoPkgTokenSpaceGuid.PcdHashApiLibPolicy|0x04|UI
> NT8|0x00000001
> + # @ValidRange 0x80000001 | 0x00000001 - 0x00000010
Using ValidRange is not correct because not all the
values from 0x0000001 - 0x00000010 are valid. Should
change to @ValidList.
# @ValidList 0x80000001 | 0x00000001, 0x00000002, 0x00000004, 0x00000008, 0x00000010
> +
> gEfiCryptoPkgTokenSpaceGuid.PcdHashApiLibPolicy|0x02|UI
> NT16|0x00000001
>
> [UserExtensions.TianoCore."ExtraFiles"]
> CryptoPkgExtra.uni
> diff --git a/CryptoPkg/CryptoPkg.uni
> b/CryptoPkg/CryptoPkg.uni
> index 2222762f42ee..7e97ac7af8b7 100644
> --- a/CryptoPkg/CryptoPkg.uni
> +++ b/CryptoPkg/CryptoPkg.uni
> @@ -21,13 +21,11 @@
>
> "Based on the value set, the required algorithm is
> chosen to calculate\n"
>
> "the hash of data.<BR>\n"
>
> "The default hashing algorithm for BaseHashApiLib is
> set to SHA256.<BR>\n"
> -
> "0x00000001 - MD4.<BR>\n"
> -
> "0x00000002 - MD5.<BR>\n"
> -
> "0x00000003 - SHA1.<BR>\n"
> -
> "0x00000004 - SHA256.<BR>\n"
> -
> "0x00000005 - SHA384.<BR>\n"
> -
> "0x00000006 - SHA512.<BR>\n"
> -
> "0x00000007 - SM3.<BR>"
> +
> "0x00000001 - SHA1.<BR>\n"
> +
> "0x00000002 - SHA256.<BR>\n"
> +
> "0x00000004 - SHA384.<BR>\n"
> +
> "0x00000008 - SHA512.<BR>\n"
> +
> "0x00000010 - SM3.<BR>"
Update the names to match the define names in Tpm20.h
such as HASH_ALG_SHA256.
>
> #string
> STR_gEfiCryptoPkgTokenSpaceGuid_PcdCryptoServiceFamilyE
> nable_PROMPT #language en-US "Enable/Disable EDK II
> Crypto Protocol/PPI services"
>
> diff --git a/CryptoPkg/Include/Library/HashApiLib.h
> b/CryptoPkg/Include/Library/HashApiLib.h
> index 22068e5a1756..b8b52ae15bd9 100644
> --- a/CryptoPkg/Include/Library/HashApiLib.h
> +++ b/CryptoPkg/Include/Library/HashApiLib.h
> @@ -12,20 +12,10 @@
> #ifndef __BASEHASHAPILIB_H_
> #define __BASEHASHAPILIB_H_
This define name does not match the pattern for
other includes and BASE should not be used here.
Please change to:
#ifndef __HASH_API_LIB_H_
#define __HASH_API_LIB_H_
>
> +#include <IndustryStandard/Tpm20.h>
> +
> typedef VOID *HASH_API_CONTEXT;
>
> -//
> -// Hash Algorithms
> -//
> -#define HASH_API_ALGO_INVALID 0x00000000
> -#define HASH_API_ALGO_MD4 0x00000001
> -#define HASH_API_ALGO_MD5 0x00000002
> -#define HASH_API_ALGO_SHA1 0x00000003
> -#define HASH_API_ALGO_SHA256 0x00000004
> -#define HASH_API_ALGO_SHA384 0x00000005
> -#define HASH_API_ALGO_SHA512 0x00000006
> -#define HASH_API_ALGO_SM3_256 0x00000007
> -
> /**
> Retrieves the size, in bytes, of the context buffer
> required for hash operations.
>
> --
> 2.16.2.windows.1
next prev parent reply other threads:[~2020-02-14 22:29 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-14 18:05 [PATCH v2 0/2] Enhancement and Fixes to BaseHashApiLib Sukerkar, Amol N
2020-02-14 18:05 ` [PATCH v2 1/2] CryptoPkg/BaseHashApiLib: Align BaseHashApiLib with TPM 2.0 Implementation Sukerkar, Amol N
2020-02-14 22:29 ` Michael D Kinney [this message]
2020-02-14 22:38 ` Michael D Kinney
2020-02-14 23:21 ` Sukerkar, Amol N
2020-02-14 18:05 ` [PATCH v2 2/2] CryptoPkg/BaseHashApiLib: Change PcdHashApiLibPolicy type to FixedAtBuild Sukerkar, Amol N
2020-02-14 22:29 ` Michael D Kinney
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=E92EE9817A31E24EB0585FDF735412F5B9EBA55E@ORSMSX113.amr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox