public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [staging/crypto-new-api PATCH] CryptoPkg: Fix issues from crypto code review.
@ 2022-07-15  2:07 yi1 li
  2022-07-15  2:13 ` Heng Luo
  0 siblings, 1 reply; 3+ messages in thread
From: yi1 li @ 2022-07-15  2:07 UTC (permalink / raw)
  To: devel; +Cc: Yi Li, Ming Tan, Heng Luo

Details:
1. Some APIs need more detail comment.
2. Correct BnRShift() param order.
3. Remove unsecure ECC curve from GroupToNid().
4. Add full public key validating procedures to EcDhDeriveSecret().

Cc: Ming Tan <ming.tan@intel.com>
Cc: Heng Luo <heng.luo@intel.com>
Signed-off-by: Yi Li <yi1.li@intel.com>

---
 CryptoPkg/Driver/Crypto.c                              | 31 ++++++++++++++++++++-----------
 CryptoPkg/Include/Library/BaseCryptLib.h               | 31 ++++++++++++++++++++-----------
 CryptoPkg/Library/BaseCryptLib/Bn/CryptBn.c            |  7 ++++---
 CryptoPkg/Library/BaseCryptLib/Bn/CryptBnNull.c        |  4 +++-
 CryptoPkg/Library/BaseCryptLib/Ec/CryptEc.c            | 61 ++++++++++++++++++++++++++++++++++---------------------------
 CryptoPkg/Library/BaseCryptLib/Ec/CryptEcNull.c        | 27 +++++++++++++++++----------
 CryptoPkg/Library/BaseCryptLibNull/Bn/CryptBnNull.c    |  4 +++-
 CryptoPkg/Library/BaseCryptLibNull/Ec/CryptEcNull.c    | 27 +++++++++++++++++----------
 CryptoPkg/Library/BaseCryptLibOnProtocolPpi/CryptLib.c | 31 ++++++++++++++++++++-----------
 CryptoPkg/Private/Protocol/Crypto.h                    | 31 ++++++++++++++++++++-----------
 10 files changed, 158 insertions(+), 96 deletions(-)

diff --git a/CryptoPkg/Driver/Crypto.c b/CryptoPkg/Driver/Crypto.c
index de422b7f53..10a0ce8800 100644
--- a/CryptoPkg/Driver/Crypto.c
+++ b/CryptoPkg/Driver/Crypto.c
@@ -4962,7 +4962,6 @@ CryptoServiceBigNumValueOne (
   @param[out]  BnRes   The result.
 
   @retval EFI_SUCCESS          On success.
-  @retval EFI_OUT_OF_RESOURCES In case of internal allocation failures.
   @retval EFI_PROTOCOL_ERROR   Otherwise.
 **/
 EFI_STATUS
@@ -5051,6 +5050,9 @@ CryptoServiceBigNumContextFree (
 
   @param[in]   Bn     Big number to set.
   @param[in]   Val    Value to set.
+
+  @retval EFI_SUCCESS          On success.
+  @retval EFI_PROTOCOL_ERROR   Otherwise.
 **/
 EFI_STATUS
 EFIAPI
@@ -5092,7 +5094,7 @@ CryptoServiceBigNumAddMod (
   using EcGroupFree() function.
 
   @param[in]  Group  Identifying number for the ECC group (IANA "Group
-                     Description" attribute registrty for RFC 2409).
+                     Description" attribute registry for RFC 2409).
 
   @retval EcGroup object  On success.
   @retval NULL            On failure.
@@ -5114,8 +5116,8 @@ CryptoServiceEcGroupInit (
 
   @param[in]  EcGroup    EC group object.
   @param[out] BnPrime    Group prime number.
-  @param[out] BnA        A coofecient.
-  @param[out] BnB        B coofecient.
+  @param[out] BnA        A coefficient.
+  @param[out] BnB        B coefficient.
   @param[in]  BnCtx      BN context.
 
   @retval EFI_SUCCESS        On success.
@@ -5426,13 +5428,14 @@ CryptoServiceEcPointSetCompressedCoordinates (
 /**
   Generate a key using ECDH algorithm. Please note, this function uses
   pseudo random number generator. The caller must make sure RandomSeed()
-  funtion was properly called before.
+  function was properly called before.
 
   @param[in]  Group    Identifying number for the ECC group (IANA "Group
-                       Description" attribute registrty for RFC 2409).
+                       Description" attribute registry for RFC 2409).
   @param[out] PKey     Pointer to an object that will hold the ECDH key.
 
   @retval EFI_SUCCESS        On success.
+  @retval EFI_UNSUPPORTED    ECC group not supported.
   @retval EFI_PROTOCOL_ERROR On failure.
 **/
 EFI_STATUS
@@ -5466,8 +5469,9 @@ CryptoServiceEcDhKeyFree (
   @param[in]  PKey     ECDH Key object.
   @param[out] EcPoint  Properly initialized EC Point to hold the public key.
 
-  @retval EFI_SUCCESS        On success.
-  @retval EFI_PROTOCOL_ERROR On failure.
+  @retval EFI_SUCCESS           On success.
+  @retval EFI_INVALID_PARAMETER EcPoint should be initialized properly.
+  @retval EFI_PROTOCOL_ERROR    On failure.
 **/
 EFI_STATUS
 EFIAPI
@@ -5484,15 +5488,20 @@ CryptoServiceEcDhGetPubKey (
 
   @param[in]  PKey           ECDH Key object.
   @param[in]  Group          Identifying number for the ECC group (IANA "Group
-                             Description" attribute registrty for RFC 2409).
+                             Description" attribute registry for RFC 2409).
   @param[in]  EcPointPublic  Peer public key.
   @param[out] SecretSize     On success, holds secret size.
   @param[out] Secret         On success, holds the derived secret.
                              Should be freed by caller using FreePool()
                              function.
 
-  @retval EFI_SUCCESS        On success.
-  @retval EFI_PROTOCOL_ERROR On failure.
+  @retval EFI_SUCCESS           On success.
+  @retval EFI_UNSUPPORTED       ECC group not supported.
+  @retval EFI_INVALID_PARAMETER One or more of the following conditions is TRUE:
+                                Secret is NULL.
+                                SecretSize is NULL.
+                                Public key in EcPointPublic is invalid.
+  @retval EFI_PROTOCOL_ERROR    On failure.
 **/
 EFI_STATUS
 EFIAPI
diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h b/CryptoPkg/Include/Library/BaseCryptLib.h
index 8fcb496c40..0de9f0739e 100644
--- a/CryptoPkg/Include/Library/BaseCryptLib.h
+++ b/CryptoPkg/Include/Library/BaseCryptLib.h
@@ -2723,7 +2723,6 @@ BigNumValueOne (
   @param[out]  BnRes   The result.
 
   @retval EFI_SUCCESS          On success.
-  @retval EFI_OUT_OF_RESOURCES In case of internal allocation failures.
   @retval EFI_PROTOCOL_ERROR   Otherwise.
 **/
 EFI_STATUS
@@ -2797,6 +2796,9 @@ BigNumContextFree (
 
   @param[in]   Bn     Big number to set.
   @param[in]   Val    Value to set.
+
+  @retval EFI_SUCCESS          On success.
+  @retval EFI_PROTOCOL_ERROR   Otherwise.
 **/
 EFI_STATUS
 EFIAPI
@@ -2832,7 +2834,7 @@ BigNumAddMod (
   using EcGroupFree() function.
 
   @param[in]  Group  Identifying number for the ECC group (IANA "Group
-                     Description" attribute registrty for RFC 2409).
+                     Description" attribute registry for RFC 2409).
 
   @retval EcGroup object  On success.
   @retval NULL            On failure.
@@ -2851,8 +2853,8 @@ EcGroupInit (
 
   @param[in]  EcGroup    EC group object.
   @param[out] BnPrime    Group prime number.
-  @param[out] BnA        A coofecient.
-  @param[out] BnB        B coofecient.
+  @param[out] BnA        A coefficient.
+  @param[out] BnB        B coefficient.
   @param[in]  BnCtx      BN context.
 
   @retval EFI_SUCCESS        On success.
@@ -3121,13 +3123,14 @@ EcPointSetCompressedCoordinates (
 /**
   Generate a key using ECDH algorithm. Please note, this function uses
   pseudo random number generator. The caller must make sure RandomSeed()
-  funtion was properly called before.
+  function was properly called before.
 
   @param[in]  Group    Identifying number for the ECC group (IANA "Group
-                       Description" attribute registrty for RFC 2409).
+                       Description" attribute registry for RFC 2409).
   @param[out] PKey     Pointer to an object that will hold the ECDH key.
 
   @retval EFI_SUCCESS        On success.
+  @retval EFI_UNSUPPORTED    ECC group not supported.
   @retval EFI_PROTOCOL_ERROR On failure.
 **/
 EFI_STATUS
@@ -3155,8 +3158,9 @@ EcDhKeyFree (
   @param[in]  PKey     ECDH Key object.
   @param[out] EcPoint  Properly initialized EC Point to hold the public key.
 
-  @retval EFI_SUCCESS        On success.
-  @retval EFI_PROTOCOL_ERROR On failure.
+  @retval EFI_SUCCESS           On success.
+  @retval EFI_INVALID_PARAMETER EcPoint should be initialized properly.
+  @retval EFI_PROTOCOL_ERROR    On failure.
 **/
 EFI_STATUS
 EFIAPI
@@ -3170,15 +3174,20 @@ EcDhGetPubKey (
 
   @param[in]  PKey           ECDH Key object.
   @param[in]  Group          Identifying number for the ECC group (IANA "Group
-                             Description" attribute registrty for RFC 2409).
+                             Description" attribute registry for RFC 2409).
   @param[in]  EcPointPublic  Peer public key.
   @param[out] SecretSize     On success, holds secret size.
   @param[out] Secret         On success, holds the derived secret.
                              Should be freed by caller using FreePool()
                              function.
 
-  @retval EFI_SUCCESS        On success.
-  @retval EFI_PROTOCOL_ERROR On failure.
+  @retval EFI_SUCCESS           On success.
+  @retval EFI_UNSUPPORTED       ECC group not supported.
+  @retval EFI_INVALID_PARAMETER One or more of the following conditions is TRUE:
+                                Secret is NULL.
+                                SecretSize is NULL.
+                                Public key in EcPointPublic is invalid.
+  @retval EFI_PROTOCOL_ERROR    On failure.
 **/
 EFI_STATUS
 EFIAPI
diff --git a/CryptoPkg/Library/BaseCryptLib/Bn/CryptBn.c b/CryptoPkg/Library/BaseCryptLib/Bn/CryptBn.c
index 3e43492a56..b6411cd541 100644
--- a/CryptoPkg/Library/BaseCryptLib/Bn/CryptBn.c
+++ b/CryptoPkg/Library/BaseCryptLib/Bn/CryptBn.c
@@ -442,7 +442,6 @@ BigNumValueOne (
   @param[out]  BnRes   The result.
 
   @retval EFI_SUCCESS          On success.
-  @retval EFI_OUT_OF_RESOURCES In case of internal allocation failures.
   @retval EFI_PROTOCOL_ERROR   Otherwise.
 **/
 EFI_STATUS
@@ -453,8 +452,7 @@ BigNumRShift (
   OUT VOID       *BnRes
   )
 {
-  // BN_rshift() does not modify the first argument, so we remove const.
-  if (BN_rshift ((BIGNUM *)Bn, BnRes, (int)n) == 1) {
+  if (BN_rshift (BnRes, Bn, (int)n) == 1) {
     return EFI_SUCCESS;
   } else {
     return EFI_PROTOCOL_ERROR;
@@ -547,6 +545,9 @@ BigNumContextFree (
 
   @param[in]   Bn     Big number to set.
   @param[in]   Val    Value to set.
+
+  @retval EFI_SUCCESS          On success.
+  @retval EFI_PROTOCOL_ERROR   Otherwise.
 **/
 EFI_STATUS
 EFIAPI
diff --git a/CryptoPkg/Library/BaseCryptLib/Bn/CryptBnNull.c b/CryptoPkg/Library/BaseCryptLib/Bn/CryptBnNull.c
index 4a27433a0e..4d2fa039df 100644
--- a/CryptoPkg/Library/BaseCryptLib/Bn/CryptBnNull.c
+++ b/CryptoPkg/Library/BaseCryptLib/Bn/CryptBnNull.c
@@ -395,7 +395,6 @@ BigNumValueOne (
   @param[out]  BnRes   The result.
 
   @retval EFI_SUCCESS          On success.
-  @retval EFI_OUT_OF_RESOURCES In case of internal allocation failures.
   @retval EFI_PROTOCOL_ERROR   Otherwise.
 **/
 EFI_STATUS
@@ -487,6 +486,9 @@ BigNumContextFree (
 
   @param[in]   Bn     Big number to set.
   @param[in]   Val    Value to set.
+
+  @retval EFI_SUCCESS          On success.
+  @retval EFI_PROTOCOL_ERROR   Otherwise.
 **/
 EFI_STATUS
 EFIAPI
diff --git a/CryptoPkg/Library/BaseCryptLib/Ec/CryptEc.c b/CryptoPkg/Library/BaseCryptLib/Ec/CryptEc.c
index 4d1aab8d32..90d1b8bce7 100644
--- a/CryptoPkg/Library/BaseCryptLib/Ec/CryptEc.c
+++ b/CryptoPkg/Library/BaseCryptLib/Ec/CryptEc.c
@@ -21,13 +21,13 @@
 #include <openssl/ec.h>
 
 /**
-  Temp comment.
+  Return the Nid of certain ECC group.
 
   @param[in]  Group  Identifying number for the ECC group (IANA "Group
-                     Description" attribute registrty for RFC 2409).
+                     Description" attribute registry for RFC 2409).
 
-  @retval EcGroup object  On success.
-  @retval NULL            On failure.
+  @retval !=-1    On success.
+  @retval -1      ECC group not supported.
 **/
 STATIC
 INT32
@@ -47,12 +47,6 @@ GroupToNid (
     case 21:
       Nid = NID_secp521r1;
       break;
-    case 25:
-      Nid = NID_X9_62_prime192v1;
-      break;
-    case 26:
-      Nid = NID_secp224r1;
-      break;
     default:
       return -1;
   }
@@ -66,7 +60,7 @@ GroupToNid (
   using EcGroupFree() function.
 
   @param[in]  Group  Identifying number for the ECC group (IANA "Group
-                     Description" attribute registrty for RFC 2409).
+                     Description" attribute registry for RFC 2409).
 
   @retval EcGroup object  On success.
   @retval NULL            On failure.
@@ -96,8 +90,8 @@ EcGroupInit (
 
   @param[in]  EcGroup    EC group object.
   @param[out] BnPrime    Group prime number.
-  @param[out] BnA        A coofecient.
-  @param[out] BnB        B coofecient.
+  @param[out] BnA        A coefficient.
+  @param[out] BnB        B coefficient.
   @param[in]  BnCtx      BN context.
 
   @retval EFI_SUCCESS        On success.
@@ -218,7 +212,7 @@ EcPointGetAffineCoordinates (
   )
 {
   return EC_POINT_get_affine_coordinates (EcGroup, EcPoint, BnX, BnY, BnCtx) ?
-         EFI_SUCCESS : EFI_INVALID_PARAMETER;
+         EFI_SUCCESS : EFI_PROTOCOL_ERROR;
 }
 
 /**
@@ -244,7 +238,7 @@ EcPointSetAffineCoordinates (
   )
 {
   return EC_POINT_set_affine_coordinates (EcGroup, EcPoint, BnX, BnY, BnCtx) ?
-         EFI_SUCCESS : EFI_INVALID_PARAMETER;
+         EFI_SUCCESS : EFI_PROTOCOL_ERROR;
 }
 
 /**
@@ -271,7 +265,7 @@ EcPointAdd (
   )
 {
   return EC_POINT_add (EcGroup, EcPointResult, EcPointA, EcPointB, BnCtx) ?
-         EFI_SUCCESS : EFI_INVALID_PARAMETER;
+         EFI_SUCCESS : EFI_PROTOCOL_ERROR;
 }
 
 /**
@@ -298,7 +292,7 @@ EcPointMul (
   )
 {
   return EC_POINT_mul (EcGroup, EcPointResult, NULL, EcPoint, BnPScalar, BnCtx) ?
-         EFI_SUCCESS : EFI_INVALID_PARAMETER;
+         EFI_SUCCESS : EFI_PROTOCOL_ERROR;
 }
 
 /**
@@ -320,7 +314,7 @@ EcPointInvert (
   )
 {
   return EC_POINT_invert (EcGroup, EcPoint, BnCtx) ?
-         EFI_SUCCESS : EFI_INVALID_PARAMETER;
+         EFI_SUCCESS : EFI_PROTOCOL_ERROR;
 }
 
 /**
@@ -414,19 +408,20 @@ EcPointSetCompressedCoordinates (
   )
 {
   return EC_POINT_set_compressed_coordinates (EcGroup, EcPoint, BnX, YBit, BnCtx) ?
-         EFI_SUCCESS : EFI_INVALID_PARAMETER;
+         EFI_SUCCESS : EFI_PROTOCOL_ERROR;
 }
 
 /**
   Generate a key using ECDH algorithm. Please note, this function uses
   pseudo random number generator. The caller must make sure RandomSeed()
-  funtion was properly called before.
+  function was properly called before.
 
   @param[in]  Group    Identifying number for the ECC group (IANA "Group
-                       Description" attribute registrty for RFC 2409).
+                       Description" attribute registry for RFC 2409).
   @param[out] PKey     Pointer to an object that will hold the ECDH key.
 
   @retval EFI_SUCCESS        On success.
+  @retval EFI_UNSUPPORTED    ECC group not supported.
   @retval EFI_PROTOCOL_ERROR On failure.
 **/
 EFI_STATUS
@@ -508,8 +503,9 @@ EcDhKeyFree (
   @param[in]  PKey     ECDH Key object.
   @param[out] EcPoint  Properly initialized EC Point to hold the public key.
 
-  @retval EFI_SUCCESS        On success.
-  @retval EFI_PROTOCOL_ERROR On failure.
+  @retval EFI_SUCCESS           On success.
+  @retval EFI_INVALID_PARAMETER EcPoint should be initialized properly.
+  @retval EFI_PROTOCOL_ERROR    On failure.
 **/
 EFI_STATUS
 EFIAPI
@@ -553,15 +549,21 @@ out:
 
   @param[in]  PKey           ECDH Key object.
   @param[in]  Group          Identifying number for the ECC group (IANA "Group
-                             Description" attribute registrty for RFC 2409).
-  @param[in]  EcPointPublic  Peer public key.
+                             Description" attribute registry for RFC 2409).
+  @param[in]  EcPointPublic  Peer public key. Certain sanity checks on the key
+                             will be performed to confirm that it is valid.
   @param[out] SecretSize     On success, holds secret size.
   @param[out] Secret         On success, holds the derived secret.
                              Should be freed by caller using FreePool()
                              function.
 
-  @retval EFI_SUCCESS        On success.
-  @retval EFI_PROTOCOL_ERROR On failure.
+  @retval EFI_SUCCESS           On success.
+  @retval EFI_UNSUPPORTED       ECC group not supported.
+  @retval EFI_INVALID_PARAMETER One or more of the following conditions is TRUE:
+                                Secret is NULL.
+                                SecretSize is NULL.
+                                Public key in EcPointPublic is invalid.
+  @retval EFI_PROTOCOL_ERROR    On failure.
 **/
 EFI_STATUS
 EFIAPI
@@ -605,6 +607,11 @@ EcDhDeriveSecret (
     goto fail;
   }
 
+  if (!EC_KEY_check_key (EcKey)) {
+    Status = EFI_INVALID_PARAMETER;
+    goto fail;
+  }
+
   Ctx = EVP_PKEY_CTX_new (PKey, NULL);
   if ((Ctx == NULL) || (EVP_PKEY_derive_init (Ctx) != 1) ||
       (EVP_PKEY_derive_set_peer (Ctx, PeerKey) != 1) ||
diff --git a/CryptoPkg/Library/BaseCryptLib/Ec/CryptEcNull.c b/CryptoPkg/Library/BaseCryptLib/Ec/CryptEcNull.c
index 2d7e5db464..e7fe378095 100644
--- a/CryptoPkg/Library/BaseCryptLib/Ec/CryptEcNull.c
+++ b/CryptoPkg/Library/BaseCryptLib/Ec/CryptEcNull.c
@@ -15,7 +15,7 @@
   using EcGroupFree() function.
 
   @param[in]  Group  Identifying number for the ECC group (IANA "Group
-                     Description" attribute registrty for RFC 2409).
+                     Description" attribute registry for RFC 2409).
 
   @retval EcGroup object  On success.
   @retval NULL            On failure.
@@ -38,8 +38,8 @@ EcGroupInit (
 
   @param[in]  EcGroup    EC group object.
   @param[out] BnPrime    Group prime number.
-  @param[out] BnA        A coofecient.
-  @param[out] BnB        B coofecient.
+  @param[out] BnA        A coefficient.
+  @param[out] BnB        B coefficient.
   @param[in]  BnCtx      BN context.
 
   @retval EFI_SUCCESS        On success.
@@ -362,13 +362,14 @@ EcPointSetCompressedCoordinates (
 /**
   Generate a key using ECDH algorithm. Please note, this function uses
   pseudo random number generator. The caller must make sure RandomSeed()
-  funtion was properly called before.
+  function was properly called before.
 
   @param[in]  Group    Identifying number for the ECC group (IANA "Group
-                       Description" attribute registrty for RFC 2409).
+                       Description" attribute registry for RFC 2409).
   @param[out] PKey     Pointer to an object that will hold the ECDH key.
 
   @retval EFI_SUCCESS        On success.
+  @retval EFI_UNSUPPORTED    ECC group not supported.
   @retval EFI_PROTOCOL_ERROR On failure.
 **/
 EFI_STATUS
@@ -403,8 +404,9 @@ EcDhKeyFree (
   @param[in]  PKey     ECDH Key object.
   @param[out] EcPoint  Properly initialized EC Point to hold the public key.
 
-  @retval EFI_SUCCESS        On success.
-  @retval EFI_PROTOCOL_ERROR On failure.
+  @retval EFI_SUCCESS           On success.
+  @retval EFI_INVALID_PARAMETER EcPoint should be initialized properly.
+  @retval EFI_PROTOCOL_ERROR    On failure.
 **/
 EFI_STATUS
 EFIAPI
@@ -422,15 +424,20 @@ EcDhGetPubKey (
 
   @param[in]  PKey           ECDH Key object.
   @param[in]  Group          Identifying number for the ECC group (IANA "Group
-                             Description" attribute registrty for RFC 2409).
+                             Description" attribute registry for RFC 2409).
   @param[in]  EcPointPublic  Peer public key.
   @param[out] SecretSize     On success, holds secret size.
   @param[out] Secret         On success, holds the derived secret.
                              Should be freed by caller using FreePool()
                              function.
 
-  @retval EFI_SUCCESS        On success.
-  @retval EFI_PROTOCOL_ERROR On failure.
+  @retval EFI_SUCCESS           On success.
+  @retval EFI_UNSUPPORTED       ECC group not supported.
+  @retval EFI_INVALID_PARAMETER One or more of the following conditions is TRUE:
+                                Secret is NULL.
+                                SecretSize is NULL.
+                                Public key in EcPointPublic is invalid.
+  @retval EFI_PROTOCOL_ERROR    On failure.
 **/
 EFI_STATUS
 EFIAPI
diff --git a/CryptoPkg/Library/BaseCryptLibNull/Bn/CryptBnNull.c b/CryptoPkg/Library/BaseCryptLibNull/Bn/CryptBnNull.c
index 4a27433a0e..4d2fa039df 100644
--- a/CryptoPkg/Library/BaseCryptLibNull/Bn/CryptBnNull.c
+++ b/CryptoPkg/Library/BaseCryptLibNull/Bn/CryptBnNull.c
@@ -395,7 +395,6 @@ BigNumValueOne (
   @param[out]  BnRes   The result.
 
   @retval EFI_SUCCESS          On success.
-  @retval EFI_OUT_OF_RESOURCES In case of internal allocation failures.
   @retval EFI_PROTOCOL_ERROR   Otherwise.
 **/
 EFI_STATUS
@@ -487,6 +486,9 @@ BigNumContextFree (
 
   @param[in]   Bn     Big number to set.
   @param[in]   Val    Value to set.
+
+  @retval EFI_SUCCESS          On success.
+  @retval EFI_PROTOCOL_ERROR   Otherwise.
 **/
 EFI_STATUS
 EFIAPI
diff --git a/CryptoPkg/Library/BaseCryptLibNull/Ec/CryptEcNull.c b/CryptoPkg/Library/BaseCryptLibNull/Ec/CryptEcNull.c
index 2d7e5db464..e7fe378095 100644
--- a/CryptoPkg/Library/BaseCryptLibNull/Ec/CryptEcNull.c
+++ b/CryptoPkg/Library/BaseCryptLibNull/Ec/CryptEcNull.c
@@ -15,7 +15,7 @@
   using EcGroupFree() function.
 
   @param[in]  Group  Identifying number for the ECC group (IANA "Group
-                     Description" attribute registrty for RFC 2409).
+                     Description" attribute registry for RFC 2409).
 
   @retval EcGroup object  On success.
   @retval NULL            On failure.
@@ -38,8 +38,8 @@ EcGroupInit (
 
   @param[in]  EcGroup    EC group object.
   @param[out] BnPrime    Group prime number.
-  @param[out] BnA        A coofecient.
-  @param[out] BnB        B coofecient.
+  @param[out] BnA        A coefficient.
+  @param[out] BnB        B coefficient.
   @param[in]  BnCtx      BN context.
 
   @retval EFI_SUCCESS        On success.
@@ -362,13 +362,14 @@ EcPointSetCompressedCoordinates (
 /**
   Generate a key using ECDH algorithm. Please note, this function uses
   pseudo random number generator. The caller must make sure RandomSeed()
-  funtion was properly called before.
+  function was properly called before.
 
   @param[in]  Group    Identifying number for the ECC group (IANA "Group
-                       Description" attribute registrty for RFC 2409).
+                       Description" attribute registry for RFC 2409).
   @param[out] PKey     Pointer to an object that will hold the ECDH key.
 
   @retval EFI_SUCCESS        On success.
+  @retval EFI_UNSUPPORTED    ECC group not supported.
   @retval EFI_PROTOCOL_ERROR On failure.
 **/
 EFI_STATUS
@@ -403,8 +404,9 @@ EcDhKeyFree (
   @param[in]  PKey     ECDH Key object.
   @param[out] EcPoint  Properly initialized EC Point to hold the public key.
 
-  @retval EFI_SUCCESS        On success.
-  @retval EFI_PROTOCOL_ERROR On failure.
+  @retval EFI_SUCCESS           On success.
+  @retval EFI_INVALID_PARAMETER EcPoint should be initialized properly.
+  @retval EFI_PROTOCOL_ERROR    On failure.
 **/
 EFI_STATUS
 EFIAPI
@@ -422,15 +424,20 @@ EcDhGetPubKey (
 
   @param[in]  PKey           ECDH Key object.
   @param[in]  Group          Identifying number for the ECC group (IANA "Group
-                             Description" attribute registrty for RFC 2409).
+                             Description" attribute registry for RFC 2409).
   @param[in]  EcPointPublic  Peer public key.
   @param[out] SecretSize     On success, holds secret size.
   @param[out] Secret         On success, holds the derived secret.
                              Should be freed by caller using FreePool()
                              function.
 
-  @retval EFI_SUCCESS        On success.
-  @retval EFI_PROTOCOL_ERROR On failure.
+  @retval EFI_SUCCESS           On success.
+  @retval EFI_UNSUPPORTED       ECC group not supported.
+  @retval EFI_INVALID_PARAMETER One or more of the following conditions is TRUE:
+                                Secret is NULL.
+                                SecretSize is NULL.
+                                Public key in EcPointPublic is invalid.
+  @retval EFI_PROTOCOL_ERROR    On failure.
 **/
 EFI_STATUS
 EFIAPI
diff --git a/CryptoPkg/Library/BaseCryptLibOnProtocolPpi/CryptLib.c b/CryptoPkg/Library/BaseCryptLibOnProtocolPpi/CryptLib.c
index 548116abb4..0410067c9d 100644
--- a/CryptoPkg/Library/BaseCryptLibOnProtocolPpi/CryptLib.c
+++ b/CryptoPkg/Library/BaseCryptLibOnProtocolPpi/CryptLib.c
@@ -4069,7 +4069,6 @@ BigNumValueOne (
   @param[out]  BnRes   The result.
 
   @retval EFI_SUCCESS          On success.
-  @retval EFI_OUT_OF_RESOURCES In case of internal allocation failures.
   @retval EFI_PROTOCOL_ERROR   Otherwise.
 **/
 EFI_STATUS
@@ -4158,6 +4157,9 @@ BigNumContextFree (
 
   @param[in]   Bn     Big number to set.
   @param[in]   Val    Value to set.
+
+  @retval EFI_SUCCESS          On success.
+  @retval EFI_PROTOCOL_ERROR   Otherwise.
 **/
 EFI_STATUS
 EFIAPI
@@ -4199,7 +4201,7 @@ BigNumAddMod (
   using EcGroupFree() function.
 
   @param[in]  Group  Identifying number for the ECC group (IANA "Group
-                     Description" attribute registrty for RFC 2409).
+                     Description" attribute registry for RFC 2409).
 
   @retval EcGroup object  On success.
   @retval NULL            On failure.
@@ -4221,8 +4223,8 @@ EcGroupInit (
 
   @param[in]  EcGroup    EC group object.
   @param[out] BnPrime    Group prime number.
-  @param[out] BnA        A coofecient.
-  @param[out] BnB        B coofecient.
+  @param[out] BnA        A coefficient.
+  @param[out] BnB        B coefficient.
   @param[in]  BnCtx      BN context.
 
   @retval EFI_SUCCESS        On success.
@@ -4533,13 +4535,14 @@ EcPointSetCompressedCoordinates (
 /**
   Generate a key using ECDH algorithm. Please note, this function uses
   pseudo random number generator. The caller must make sure RandomSeed()
-  funtion was properly called before.
+  function was properly called before.
 
   @param[in]  Group    Identifying number for the ECC group (IANA "Group
-                       Description" attribute registrty for RFC 2409).
+                       Description" attribute registry for RFC 2409).
   @param[out] PKey     Pointer to an object that will hold the ECDH key.
 
   @retval EFI_SUCCESS        On success.
+  @retval EFI_UNSUPPORTED    ECC group not supported.
   @retval EFI_PROTOCOL_ERROR On failure.
 **/
 EFI_STATUS
@@ -4573,8 +4576,9 @@ EcDhKeyFree (
   @param[in]  PKey     ECDH Key object.
   @param[out] EcPoint  Properly initialized EC Point to hold the public key.
 
-  @retval EFI_SUCCESS        On success.
-  @retval EFI_PROTOCOL_ERROR On failure.
+  @retval EFI_SUCCESS           On success.
+  @retval EFI_INVALID_PARAMETER EcPoint should be initialized properly.
+  @retval EFI_PROTOCOL_ERROR    On failure.
 **/
 EFI_STATUS
 EFIAPI
@@ -4591,15 +4595,20 @@ EcDhGetPubKey (
 
   @param[in]  PKey           ECDH Key object.
   @param[in]  Group          Identifying number for the ECC group (IANA "Group
-                             Description" attribute registrty for RFC 2409).
+                             Description" attribute registry for RFC 2409).
   @param[in]  EcPointPublic  Peer public key.
   @param[out] SecretSize     On success, holds secret size.
   @param[out] Secret         On success, holds the derived secret.
                              Should be freed by caller using FreePool()
                              function.
 
-  @retval EFI_SUCCESS        On success.
-  @retval EFI_PROTOCOL_ERROR On failure.
+  @retval EFI_SUCCESS           On success.
+  @retval EFI_UNSUPPORTED       ECC group not supported.
+  @retval EFI_INVALID_PARAMETER One or more of the following conditions is TRUE:
+                                Secret is NULL.
+                                SecretSize is NULL.
+                                Public key in EcPointPublic is invalid.
+  @retval EFI_PROTOCOL_ERROR    On failure.
 **/
 EFI_STATUS
 EFIAPI
diff --git a/CryptoPkg/Private/Protocol/Crypto.h b/CryptoPkg/Private/Protocol/Crypto.h
index 1b31714d77..1cf5d18cc3 100644
--- a/CryptoPkg/Private/Protocol/Crypto.h
+++ b/CryptoPkg/Private/Protocol/Crypto.h
@@ -3863,7 +3863,6 @@ CONST VOID *
   @param[out]  BnRes   The result, such that (BnA * BnB) % BnM.
 
   @retval EFI_SUCCESS          On success.
-  @retval EFI_OUT_OF_RESOURCES In case of internal allocation failures.
   @retval EFI_PROTOCOL_ERROR   Otherwise.
 **/
 typedef
@@ -3935,6 +3934,9 @@ VOID
 
   @param[in]   Bn     Big number to set.
   @param[in]   Val    Value to set.
+
+  @retval EFI_SUCCESS          On success.
+  @retval EFI_PROTOCOL_ERROR   Otherwise.
 **/
 typedef
 EFI_STATUS
@@ -3970,7 +3972,7 @@ EFI_STATUS
   using EcGroupFree() function.
 
   @param[in]  Group  Identifying number for the ECC group (IANA "Group
-                     Description" attribute registrty for RFC 2409)
+                     Description" attribute registry for RFC 2409)
 
   @retval EcGroup object  On success
   @retval NULL            On failure
@@ -3989,8 +3991,8 @@ VOID *
 
   @param[in]  EcGroup    EC group object
   @param[out] BnPrime    Group prime number
-  @param[out] BnA        A coofecient
-  @param[out] BnB        B coofecient
+  @param[out] BnA        A coefficient
+  @param[out] BnB        B coefficient
   @param[in]  BnCtx      BN context
 
   @retval EFI_SUCCESS        On success
@@ -4260,13 +4262,14 @@ EFI_STATUS
 /**
   Generate a key using ECDH algorithm. Please note, this function uses
   pseudo random number generator. The caller must make sure RandomSeed()
-  funtion was properly called before.
+  function was properly called before.
 
   @param[in]  Group    Identifying number for the ECC group (IANA "Group
-                       Description" attribute registrty for RFC 2409)
+                       Description" attribute registry for RFC 2409)
   @param[out] PKey     Pointer to an object that will hold the ECDH key
 
   @retval EFI_SUCCESS        On success
+  @retval EFI_UNSUPPORTED    ECC group not supported.
   @retval EFI_PROTOCOL_ERROR On failure
 **/
 typedef
@@ -4294,8 +4297,9 @@ VOID
   @param[in]  PKey     ECDH Key object
   @param[out] EcPoint  Properly initialized EC Point to hold the public key
 
-  @retval EFI_SUCCESS        On success
-  @retval EFI_PROTOCOL_ERROR On failure
+  @retval EFI_SUCCESS           On success
+  @retval EFI_INVALID_PARAMETER EcPoint should be initialized properly.
+  @retval EFI_PROTOCOL_ERROR    On failure
 **/
 typedef
 EFI_STATUS
@@ -4309,15 +4313,20 @@ EFI_STATUS
 
   @param[in]  PKey           ECDH Key object
   @param[in]  Group          Identifying number for the ECC group (IANA "Group
-                             Description" attribute registrty for RFC 2409)
+                             Description" attribute registry for RFC 2409)
   @param[in]  EcPointPublic  Peer public key
   @param[out] SecretSize     On success, holds secret size
   @param[out] Secret         On success, holds the derived secret
                              Should be freed by caller using FreePool()
                              function.
 
-  @retval EFI_SUCCESS        On success
-  @retval EFI_PROTOCOL_ERROR On failure
+  @retval EFI_SUCCESS           On success.
+  @retval EFI_UNSUPPORTED       ECC group not supported.
+  @retval EFI_INVALID_PARAMETER One or more of the following conditions is TRUE:
+                                Secret is NULL.
+                                SecretSize is NULL.
+                                Public key in EcPointPublic is invalid.
+  @retval EFI_PROTOCOL_ERROR    On failure.
 **/
 typedef
 EFI_STATUS
-- 
2.31.1.windows.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [staging/crypto-new-api PATCH] CryptoPkg: Fix issues from crypto code review.
  2022-07-15  2:07 [staging/crypto-new-api PATCH] CryptoPkg: Fix issues from crypto code review yi1 li
@ 2022-07-15  2:13 ` Heng Luo
  2022-07-15  3:31   ` Tan, Ming
  0 siblings, 1 reply; 3+ messages in thread
From: Heng Luo @ 2022-07-15  2:13 UTC (permalink / raw)
  To: Li, Yi1, devel@edk2.groups.io; +Cc: Tan, Ming

Reviewed-by: Heng Luo <heng.luo@intel.com>

> -----Original Message-----
> From: Li, Yi1 <yi1.li@intel.com>
> Sent: Friday, July 15, 2022 10:07 AM
> To: devel@edk2.groups.io
> Cc: Li, Yi1 <yi1.li@intel.com>; Tan, Ming <ming.tan@intel.com>; Luo, Heng
> <heng.luo@intel.com>
> Subject: [staging/crypto-new-api PATCH] CryptoPkg: Fix issues from crypto code
> review.
> 
> Details:
> 1. Some APIs need more detail comment.
> 2. Correct BnRShift() param order.
> 3. Remove unsecure ECC curve from GroupToNid().
> 4. Add full public key validating procedures to EcDhDeriveSecret().
> 
> Cc: Ming Tan <ming.tan@intel.com>
> Cc: Heng Luo <heng.luo@intel.com>
> Signed-off-by: Yi Li <yi1.li@intel.com>
> 
> ---
>  CryptoPkg/Driver/Crypto.c                              | 31 ++++++++++++++++++++---------
> --
>  CryptoPkg/Include/Library/BaseCryptLib.h               | 31
> ++++++++++++++++++++-----------
>  CryptoPkg/Library/BaseCryptLib/Bn/CryptBn.c            |  7 ++++---
>  CryptoPkg/Library/BaseCryptLib/Bn/CryptBnNull.c        |  4 +++-
>  CryptoPkg/Library/BaseCryptLib/Ec/CryptEc.c            | 61
> ++++++++++++++++++++++++++++++++++---------------------------
>  CryptoPkg/Library/BaseCryptLib/Ec/CryptEcNull.c        | 27
> +++++++++++++++++----------
>  CryptoPkg/Library/BaseCryptLibNull/Bn/CryptBnNull.c    |  4 +++-
>  CryptoPkg/Library/BaseCryptLibNull/Ec/CryptEcNull.c    | 27
> +++++++++++++++++----------
>  CryptoPkg/Library/BaseCryptLibOnProtocolPpi/CryptLib.c | 31
> ++++++++++++++++++++-----------
>  CryptoPkg/Private/Protocol/Crypto.h                    | 31 ++++++++++++++++++++--
> ---------
>  10 files changed, 158 insertions(+), 96 deletions(-)
> 
> diff --git a/CryptoPkg/Driver/Crypto.c b/CryptoPkg/Driver/Crypto.c index
> de422b7f53..10a0ce8800 100644
> --- a/CryptoPkg/Driver/Crypto.c
> +++ b/CryptoPkg/Driver/Crypto.c
> @@ -4962,7 +4962,6 @@ CryptoServiceBigNumValueOne (
>    @param[out]  BnRes   The result.
> 
>    @retval EFI_SUCCESS          On success.
> -  @retval EFI_OUT_OF_RESOURCES In case of internal allocation failures.
>    @retval EFI_PROTOCOL_ERROR   Otherwise.
>  **/
>  EFI_STATUS
> @@ -5051,6 +5050,9 @@ CryptoServiceBigNumContextFree (
> 
>    @param[in]   Bn     Big number to set.
>    @param[in]   Val    Value to set.
> +
> +  @retval EFI_SUCCESS          On success.
> +  @retval EFI_PROTOCOL_ERROR   Otherwise.
>  **/
>  EFI_STATUS
>  EFIAPI
> @@ -5092,7 +5094,7 @@ CryptoServiceBigNumAddMod (
>    using EcGroupFree() function.
> 
>    @param[in]  Group  Identifying number for the ECC group (IANA "Group
> -                     Description" attribute registrty for RFC 2409).
> +                     Description" attribute registry for RFC 2409).
> 
>    @retval EcGroup object  On success.
>    @retval NULL            On failure.
> @@ -5114,8 +5116,8 @@ CryptoServiceEcGroupInit (
> 
>    @param[in]  EcGroup    EC group object.
>    @param[out] BnPrime    Group prime number.
> -  @param[out] BnA        A coofecient.
> -  @param[out] BnB        B coofecient.
> +  @param[out] BnA        A coefficient.
> +  @param[out] BnB        B coefficient.
>    @param[in]  BnCtx      BN context.
> 
>    @retval EFI_SUCCESS        On success.
> @@ -5426,13 +5428,14 @@ CryptoServiceEcPointSetCompressedCoordinates (
>  /**
>    Generate a key using ECDH algorithm. Please note, this function uses
>    pseudo random number generator. The caller must make sure RandomSeed()
> -  funtion was properly called before.
> +  function was properly called before.
> 
>    @param[in]  Group    Identifying number for the ECC group (IANA "Group
> -                       Description" attribute registrty for RFC 2409).
> +                       Description" attribute registry for RFC 2409).
>    @param[out] PKey     Pointer to an object that will hold the ECDH key.
> 
>    @retval EFI_SUCCESS        On success.
> +  @retval EFI_UNSUPPORTED    ECC group not supported.
>    @retval EFI_PROTOCOL_ERROR On failure.
>  **/
>  EFI_STATUS
> @@ -5466,8 +5469,9 @@ CryptoServiceEcDhKeyFree (
>    @param[in]  PKey     ECDH Key object.
>    @param[out] EcPoint  Properly initialized EC Point to hold the public key.
> 
> -  @retval EFI_SUCCESS        On success.
> -  @retval EFI_PROTOCOL_ERROR On failure.
> +  @retval EFI_SUCCESS           On success.
> +  @retval EFI_INVALID_PARAMETER EcPoint should be initialized properly.
> +  @retval EFI_PROTOCOL_ERROR    On failure.
>  **/
>  EFI_STATUS
>  EFIAPI
> @@ -5484,15 +5488,20 @@ CryptoServiceEcDhGetPubKey (
> 
>    @param[in]  PKey           ECDH Key object.
>    @param[in]  Group          Identifying number for the ECC group (IANA "Group
> -                             Description" attribute registrty for RFC 2409).
> +                             Description" attribute registry for RFC 2409).
>    @param[in]  EcPointPublic  Peer public key.
>    @param[out] SecretSize     On success, holds secret size.
>    @param[out] Secret         On success, holds the derived secret.
>                               Should be freed by caller using FreePool()
>                               function.
> 
> -  @retval EFI_SUCCESS        On success.
> -  @retval EFI_PROTOCOL_ERROR On failure.
> +  @retval EFI_SUCCESS           On success.
> +  @retval EFI_UNSUPPORTED       ECC group not supported.
> +  @retval EFI_INVALID_PARAMETER One or more of the following conditions is
> TRUE:
> +                                Secret is NULL.
> +                                SecretSize is NULL.
> +                                Public key in EcPointPublic is invalid.
> +  @retval EFI_PROTOCOL_ERROR    On failure.
>  **/
>  EFI_STATUS
>  EFIAPI
> diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h
> b/CryptoPkg/Include/Library/BaseCryptLib.h
> index 8fcb496c40..0de9f0739e 100644
> --- a/CryptoPkg/Include/Library/BaseCryptLib.h
> +++ b/CryptoPkg/Include/Library/BaseCryptLib.h
> @@ -2723,7 +2723,6 @@ BigNumValueOne (
>    @param[out]  BnRes   The result.
> 
>    @retval EFI_SUCCESS          On success.
> -  @retval EFI_OUT_OF_RESOURCES In case of internal allocation failures.
>    @retval EFI_PROTOCOL_ERROR   Otherwise.
>  **/
>  EFI_STATUS
> @@ -2797,6 +2796,9 @@ BigNumContextFree (
> 
>    @param[in]   Bn     Big number to set.
>    @param[in]   Val    Value to set.
> +
> +  @retval EFI_SUCCESS          On success.
> +  @retval EFI_PROTOCOL_ERROR   Otherwise.
>  **/
>  EFI_STATUS
>  EFIAPI
> @@ -2832,7 +2834,7 @@ BigNumAddMod (
>    using EcGroupFree() function.
> 
>    @param[in]  Group  Identifying number for the ECC group (IANA "Group
> -                     Description" attribute registrty for RFC 2409).
> +                     Description" attribute registry for RFC 2409).
> 
>    @retval EcGroup object  On success.
>    @retval NULL            On failure.
> @@ -2851,8 +2853,8 @@ EcGroupInit (
> 
>    @param[in]  EcGroup    EC group object.
>    @param[out] BnPrime    Group prime number.
> -  @param[out] BnA        A coofecient.
> -  @param[out] BnB        B coofecient.
> +  @param[out] BnA        A coefficient.
> +  @param[out] BnB        B coefficient.
>    @param[in]  BnCtx      BN context.
> 
>    @retval EFI_SUCCESS        On success.
> @@ -3121,13 +3123,14 @@ EcPointSetCompressedCoordinates (
>  /**
>    Generate a key using ECDH algorithm. Please note, this function uses
>    pseudo random number generator. The caller must make sure RandomSeed()
> -  funtion was properly called before.
> +  function was properly called before.
> 
>    @param[in]  Group    Identifying number for the ECC group (IANA "Group
> -                       Description" attribute registrty for RFC 2409).
> +                       Description" attribute registry for RFC 2409).
>    @param[out] PKey     Pointer to an object that will hold the ECDH key.
> 
>    @retval EFI_SUCCESS        On success.
> +  @retval EFI_UNSUPPORTED    ECC group not supported.
>    @retval EFI_PROTOCOL_ERROR On failure.
>  **/
>  EFI_STATUS
> @@ -3155,8 +3158,9 @@ EcDhKeyFree (
>    @param[in]  PKey     ECDH Key object.
>    @param[out] EcPoint  Properly initialized EC Point to hold the public key.
> 
> -  @retval EFI_SUCCESS        On success.
> -  @retval EFI_PROTOCOL_ERROR On failure.
> +  @retval EFI_SUCCESS           On success.
> +  @retval EFI_INVALID_PARAMETER EcPoint should be initialized properly.
> +  @retval EFI_PROTOCOL_ERROR    On failure.
>  **/
>  EFI_STATUS
>  EFIAPI
> @@ -3170,15 +3174,20 @@ EcDhGetPubKey (
> 
>    @param[in]  PKey           ECDH Key object.
>    @param[in]  Group          Identifying number for the ECC group (IANA "Group
> -                             Description" attribute registrty for RFC 2409).
> +                             Description" attribute registry for RFC 2409).
>    @param[in]  EcPointPublic  Peer public key.
>    @param[out] SecretSize     On success, holds secret size.
>    @param[out] Secret         On success, holds the derived secret.
>                               Should be freed by caller using FreePool()
>                               function.
> 
> -  @retval EFI_SUCCESS        On success.
> -  @retval EFI_PROTOCOL_ERROR On failure.
> +  @retval EFI_SUCCESS           On success.
> +  @retval EFI_UNSUPPORTED       ECC group not supported.
> +  @retval EFI_INVALID_PARAMETER One or more of the following conditions is
> TRUE:
> +                                Secret is NULL.
> +                                SecretSize is NULL.
> +                                Public key in EcPointPublic is invalid.
> +  @retval EFI_PROTOCOL_ERROR    On failure.
>  **/
>  EFI_STATUS
>  EFIAPI
> diff --git a/CryptoPkg/Library/BaseCryptLib/Bn/CryptBn.c
> b/CryptoPkg/Library/BaseCryptLib/Bn/CryptBn.c
> index 3e43492a56..b6411cd541 100644
> --- a/CryptoPkg/Library/BaseCryptLib/Bn/CryptBn.c
> +++ b/CryptoPkg/Library/BaseCryptLib/Bn/CryptBn.c
> @@ -442,7 +442,6 @@ BigNumValueOne (
>    @param[out]  BnRes   The result.
> 
>    @retval EFI_SUCCESS          On success.
> -  @retval EFI_OUT_OF_RESOURCES In case of internal allocation failures.
>    @retval EFI_PROTOCOL_ERROR   Otherwise.
>  **/
>  EFI_STATUS
> @@ -453,8 +452,7 @@ BigNumRShift (
>    OUT VOID       *BnRes
>    )
>  {
> -  // BN_rshift() does not modify the first argument, so we remove const.
> -  if (BN_rshift ((BIGNUM *)Bn, BnRes, (int)n) == 1) {
> +  if (BN_rshift (BnRes, Bn, (int)n) == 1) {
>      return EFI_SUCCESS;
>    } else {
>      return EFI_PROTOCOL_ERROR;
> @@ -547,6 +545,9 @@ BigNumContextFree (
> 
>    @param[in]   Bn     Big number to set.
>    @param[in]   Val    Value to set.
> +
> +  @retval EFI_SUCCESS          On success.
> +  @retval EFI_PROTOCOL_ERROR   Otherwise.
>  **/
>  EFI_STATUS
>  EFIAPI
> diff --git a/CryptoPkg/Library/BaseCryptLib/Bn/CryptBnNull.c
> b/CryptoPkg/Library/BaseCryptLib/Bn/CryptBnNull.c
> index 4a27433a0e..4d2fa039df 100644
> --- a/CryptoPkg/Library/BaseCryptLib/Bn/CryptBnNull.c
> +++ b/CryptoPkg/Library/BaseCryptLib/Bn/CryptBnNull.c
> @@ -395,7 +395,6 @@ BigNumValueOne (
>    @param[out]  BnRes   The result.
> 
>    @retval EFI_SUCCESS          On success.
> -  @retval EFI_OUT_OF_RESOURCES In case of internal allocation failures.
>    @retval EFI_PROTOCOL_ERROR   Otherwise.
>  **/
>  EFI_STATUS
> @@ -487,6 +486,9 @@ BigNumContextFree (
> 
>    @param[in]   Bn     Big number to set.
>    @param[in]   Val    Value to set.
> +
> +  @retval EFI_SUCCESS          On success.
> +  @retval EFI_PROTOCOL_ERROR   Otherwise.
>  **/
>  EFI_STATUS
>  EFIAPI
> diff --git a/CryptoPkg/Library/BaseCryptLib/Ec/CryptEc.c
> b/CryptoPkg/Library/BaseCryptLib/Ec/CryptEc.c
> index 4d1aab8d32..90d1b8bce7 100644
> --- a/CryptoPkg/Library/BaseCryptLib/Ec/CryptEc.c
> +++ b/CryptoPkg/Library/BaseCryptLib/Ec/CryptEc.c
> @@ -21,13 +21,13 @@
>  #include <openssl/ec.h>
> 
>  /**
> -  Temp comment.
> +  Return the Nid of certain ECC group.
> 
>    @param[in]  Group  Identifying number for the ECC group (IANA "Group
> -                     Description" attribute registrty for RFC 2409).
> +                     Description" attribute registry for RFC 2409).
> 
> -  @retval EcGroup object  On success.
> -  @retval NULL            On failure.
> +  @retval !=-1    On success.
> +  @retval -1      ECC group not supported.
>  **/
>  STATIC
>  INT32
> @@ -47,12 +47,6 @@ GroupToNid (
>      case 21:
>        Nid = NID_secp521r1;
>        break;
> -    case 25:
> -      Nid = NID_X9_62_prime192v1;
> -      break;
> -    case 26:
> -      Nid = NID_secp224r1;
> -      break;
>      default:
>        return -1;
>    }
> @@ -66,7 +60,7 @@ GroupToNid (
>    using EcGroupFree() function.
> 
>    @param[in]  Group  Identifying number for the ECC group (IANA "Group
> -                     Description" attribute registrty for RFC 2409).
> +                     Description" attribute registry for RFC 2409).
> 
>    @retval EcGroup object  On success.
>    @retval NULL            On failure.
> @@ -96,8 +90,8 @@ EcGroupInit (
> 
>    @param[in]  EcGroup    EC group object.
>    @param[out] BnPrime    Group prime number.
> -  @param[out] BnA        A coofecient.
> -  @param[out] BnB        B coofecient.
> +  @param[out] BnA        A coefficient.
> +  @param[out] BnB        B coefficient.
>    @param[in]  BnCtx      BN context.
> 
>    @retval EFI_SUCCESS        On success.
> @@ -218,7 +212,7 @@ EcPointGetAffineCoordinates (
>    )
>  {
>    return EC_POINT_get_affine_coordinates (EcGroup, EcPoint, BnX, BnY, BnCtx) ?
> -         EFI_SUCCESS : EFI_INVALID_PARAMETER;
> +         EFI_SUCCESS : EFI_PROTOCOL_ERROR;
>  }
> 
>  /**
> @@ -244,7 +238,7 @@ EcPointSetAffineCoordinates (
>    )
>  {
>    return EC_POINT_set_affine_coordinates (EcGroup, EcPoint, BnX, BnY, BnCtx) ?
> -         EFI_SUCCESS : EFI_INVALID_PARAMETER;
> +         EFI_SUCCESS : EFI_PROTOCOL_ERROR;
>  }
> 
>  /**
> @@ -271,7 +265,7 @@ EcPointAdd (
>    )
>  {
>    return EC_POINT_add (EcGroup, EcPointResult, EcPointA, EcPointB, BnCtx) ?
> -         EFI_SUCCESS : EFI_INVALID_PARAMETER;
> +         EFI_SUCCESS : EFI_PROTOCOL_ERROR;
>  }
> 
>  /**
> @@ -298,7 +292,7 @@ EcPointMul (
>    )
>  {
>    return EC_POINT_mul (EcGroup, EcPointResult, NULL, EcPoint, BnPScalar,
> BnCtx) ?
> -         EFI_SUCCESS : EFI_INVALID_PARAMETER;
> +         EFI_SUCCESS : EFI_PROTOCOL_ERROR;
>  }
> 
>  /**
> @@ -320,7 +314,7 @@ EcPointInvert (
>    )
>  {
>    return EC_POINT_invert (EcGroup, EcPoint, BnCtx) ?
> -         EFI_SUCCESS : EFI_INVALID_PARAMETER;
> +         EFI_SUCCESS : EFI_PROTOCOL_ERROR;
>  }
> 
>  /**
> @@ -414,19 +408,20 @@ EcPointSetCompressedCoordinates (
>    )
>  {
>    return EC_POINT_set_compressed_coordinates (EcGroup, EcPoint, BnX, YBit,
> BnCtx) ?
> -         EFI_SUCCESS : EFI_INVALID_PARAMETER;
> +         EFI_SUCCESS : EFI_PROTOCOL_ERROR;
>  }
> 
>  /**
>    Generate a key using ECDH algorithm. Please note, this function uses
>    pseudo random number generator. The caller must make sure RandomSeed()
> -  funtion was properly called before.
> +  function was properly called before.
> 
>    @param[in]  Group    Identifying number for the ECC group (IANA "Group
> -                       Description" attribute registrty for RFC 2409).
> +                       Description" attribute registry for RFC 2409).
>    @param[out] PKey     Pointer to an object that will hold the ECDH key.
> 
>    @retval EFI_SUCCESS        On success.
> +  @retval EFI_UNSUPPORTED    ECC group not supported.
>    @retval EFI_PROTOCOL_ERROR On failure.
>  **/
>  EFI_STATUS
> @@ -508,8 +503,9 @@ EcDhKeyFree (
>    @param[in]  PKey     ECDH Key object.
>    @param[out] EcPoint  Properly initialized EC Point to hold the public key.
> 
> -  @retval EFI_SUCCESS        On success.
> -  @retval EFI_PROTOCOL_ERROR On failure.
> +  @retval EFI_SUCCESS           On success.
> +  @retval EFI_INVALID_PARAMETER EcPoint should be initialized properly.
> +  @retval EFI_PROTOCOL_ERROR    On failure.
>  **/
>  EFI_STATUS
>  EFIAPI
> @@ -553,15 +549,21 @@ out:
> 
>    @param[in]  PKey           ECDH Key object.
>    @param[in]  Group          Identifying number for the ECC group (IANA "Group
> -                             Description" attribute registrty for RFC 2409).
> -  @param[in]  EcPointPublic  Peer public key.
> +                             Description" attribute registry for RFC 2409).
> +  @param[in]  EcPointPublic  Peer public key. Certain sanity checks on the key
> +                             will be performed to confirm that it is valid.
>    @param[out] SecretSize     On success, holds secret size.
>    @param[out] Secret         On success, holds the derived secret.
>                               Should be freed by caller using FreePool()
>                               function.
> 
> -  @retval EFI_SUCCESS        On success.
> -  @retval EFI_PROTOCOL_ERROR On failure.
> +  @retval EFI_SUCCESS           On success.
> +  @retval EFI_UNSUPPORTED       ECC group not supported.
> +  @retval EFI_INVALID_PARAMETER One or more of the following conditions is
> TRUE:
> +                                Secret is NULL.
> +                                SecretSize is NULL.
> +                                Public key in EcPointPublic is invalid.
> +  @retval EFI_PROTOCOL_ERROR    On failure.
>  **/
>  EFI_STATUS
>  EFIAPI
> @@ -605,6 +607,11 @@ EcDhDeriveSecret (
>      goto fail;
>    }
> 
> +  if (!EC_KEY_check_key (EcKey)) {
> +    Status = EFI_INVALID_PARAMETER;
> +    goto fail;
> +  }
> +
>    Ctx = EVP_PKEY_CTX_new (PKey, NULL);
>    if ((Ctx == NULL) || (EVP_PKEY_derive_init (Ctx) != 1) ||
>        (EVP_PKEY_derive_set_peer (Ctx, PeerKey) != 1) || diff --git
> a/CryptoPkg/Library/BaseCryptLib/Ec/CryptEcNull.c
> b/CryptoPkg/Library/BaseCryptLib/Ec/CryptEcNull.c
> index 2d7e5db464..e7fe378095 100644
> --- a/CryptoPkg/Library/BaseCryptLib/Ec/CryptEcNull.c
> +++ b/CryptoPkg/Library/BaseCryptLib/Ec/CryptEcNull.c
> @@ -15,7 +15,7 @@
>    using EcGroupFree() function.
> 
>    @param[in]  Group  Identifying number for the ECC group (IANA "Group
> -                     Description" attribute registrty for RFC 2409).
> +                     Description" attribute registry for RFC 2409).
> 
>    @retval EcGroup object  On success.
>    @retval NULL            On failure.
> @@ -38,8 +38,8 @@ EcGroupInit (
> 
>    @param[in]  EcGroup    EC group object.
>    @param[out] BnPrime    Group prime number.
> -  @param[out] BnA        A coofecient.
> -  @param[out] BnB        B coofecient.
> +  @param[out] BnA        A coefficient.
> +  @param[out] BnB        B coefficient.
>    @param[in]  BnCtx      BN context.
> 
>    @retval EFI_SUCCESS        On success.
> @@ -362,13 +362,14 @@ EcPointSetCompressedCoordinates (
>  /**
>    Generate a key using ECDH algorithm. Please note, this function uses
>    pseudo random number generator. The caller must make sure RandomSeed()
> -  funtion was properly called before.
> +  function was properly called before.
> 
>    @param[in]  Group    Identifying number for the ECC group (IANA "Group
> -                       Description" attribute registrty for RFC 2409).
> +                       Description" attribute registry for RFC 2409).
>    @param[out] PKey     Pointer to an object that will hold the ECDH key.
> 
>    @retval EFI_SUCCESS        On success.
> +  @retval EFI_UNSUPPORTED    ECC group not supported.
>    @retval EFI_PROTOCOL_ERROR On failure.
>  **/
>  EFI_STATUS
> @@ -403,8 +404,9 @@ EcDhKeyFree (
>    @param[in]  PKey     ECDH Key object.
>    @param[out] EcPoint  Properly initialized EC Point to hold the public key.
> 
> -  @retval EFI_SUCCESS        On success.
> -  @retval EFI_PROTOCOL_ERROR On failure.
> +  @retval EFI_SUCCESS           On success.
> +  @retval EFI_INVALID_PARAMETER EcPoint should be initialized properly.
> +  @retval EFI_PROTOCOL_ERROR    On failure.
>  **/
>  EFI_STATUS
>  EFIAPI
> @@ -422,15 +424,20 @@ EcDhGetPubKey (
> 
>    @param[in]  PKey           ECDH Key object.
>    @param[in]  Group          Identifying number for the ECC group (IANA "Group
> -                             Description" attribute registrty for RFC 2409).
> +                             Description" attribute registry for RFC 2409).
>    @param[in]  EcPointPublic  Peer public key.
>    @param[out] SecretSize     On success, holds secret size.
>    @param[out] Secret         On success, holds the derived secret.
>                               Should be freed by caller using FreePool()
>                               function.
> 
> -  @retval EFI_SUCCESS        On success.
> -  @retval EFI_PROTOCOL_ERROR On failure.
> +  @retval EFI_SUCCESS           On success.
> +  @retval EFI_UNSUPPORTED       ECC group not supported.
> +  @retval EFI_INVALID_PARAMETER One or more of the following conditions is
> TRUE:
> +                                Secret is NULL.
> +                                SecretSize is NULL.
> +                                Public key in EcPointPublic is invalid.
> +  @retval EFI_PROTOCOL_ERROR    On failure.
>  **/
>  EFI_STATUS
>  EFIAPI
> diff --git a/CryptoPkg/Library/BaseCryptLibNull/Bn/CryptBnNull.c
> b/CryptoPkg/Library/BaseCryptLibNull/Bn/CryptBnNull.c
> index 4a27433a0e..4d2fa039df 100644
> --- a/CryptoPkg/Library/BaseCryptLibNull/Bn/CryptBnNull.c
> +++ b/CryptoPkg/Library/BaseCryptLibNull/Bn/CryptBnNull.c
> @@ -395,7 +395,6 @@ BigNumValueOne (
>    @param[out]  BnRes   The result.
> 
>    @retval EFI_SUCCESS          On success.
> -  @retval EFI_OUT_OF_RESOURCES In case of internal allocation failures.
>    @retval EFI_PROTOCOL_ERROR   Otherwise.
>  **/
>  EFI_STATUS
> @@ -487,6 +486,9 @@ BigNumContextFree (
> 
>    @param[in]   Bn     Big number to set.
>    @param[in]   Val    Value to set.
> +
> +  @retval EFI_SUCCESS          On success.
> +  @retval EFI_PROTOCOL_ERROR   Otherwise.
>  **/
>  EFI_STATUS
>  EFIAPI
> diff --git a/CryptoPkg/Library/BaseCryptLibNull/Ec/CryptEcNull.c
> b/CryptoPkg/Library/BaseCryptLibNull/Ec/CryptEcNull.c
> index 2d7e5db464..e7fe378095 100644
> --- a/CryptoPkg/Library/BaseCryptLibNull/Ec/CryptEcNull.c
> +++ b/CryptoPkg/Library/BaseCryptLibNull/Ec/CryptEcNull.c
> @@ -15,7 +15,7 @@
>    using EcGroupFree() function.
> 
>    @param[in]  Group  Identifying number for the ECC group (IANA "Group
> -                     Description" attribute registrty for RFC 2409).
> +                     Description" attribute registry for RFC 2409).
> 
>    @retval EcGroup object  On success.
>    @retval NULL            On failure.
> @@ -38,8 +38,8 @@ EcGroupInit (
> 
>    @param[in]  EcGroup    EC group object.
>    @param[out] BnPrime    Group prime number.
> -  @param[out] BnA        A coofecient.
> -  @param[out] BnB        B coofecient.
> +  @param[out] BnA        A coefficient.
> +  @param[out] BnB        B coefficient.
>    @param[in]  BnCtx      BN context.
> 
>    @retval EFI_SUCCESS        On success.
> @@ -362,13 +362,14 @@ EcPointSetCompressedCoordinates (
>  /**
>    Generate a key using ECDH algorithm. Please note, this function uses
>    pseudo random number generator. The caller must make sure RandomSeed()
> -  funtion was properly called before.
> +  function was properly called before.
> 
>    @param[in]  Group    Identifying number for the ECC group (IANA "Group
> -                       Description" attribute registrty for RFC 2409).
> +                       Description" attribute registry for RFC 2409).
>    @param[out] PKey     Pointer to an object that will hold the ECDH key.
> 
>    @retval EFI_SUCCESS        On success.
> +  @retval EFI_UNSUPPORTED    ECC group not supported.
>    @retval EFI_PROTOCOL_ERROR On failure.
>  **/
>  EFI_STATUS
> @@ -403,8 +404,9 @@ EcDhKeyFree (
>    @param[in]  PKey     ECDH Key object.
>    @param[out] EcPoint  Properly initialized EC Point to hold the public key.
> 
> -  @retval EFI_SUCCESS        On success.
> -  @retval EFI_PROTOCOL_ERROR On failure.
> +  @retval EFI_SUCCESS           On success.
> +  @retval EFI_INVALID_PARAMETER EcPoint should be initialized properly.
> +  @retval EFI_PROTOCOL_ERROR    On failure.
>  **/
>  EFI_STATUS
>  EFIAPI
> @@ -422,15 +424,20 @@ EcDhGetPubKey (
> 
>    @param[in]  PKey           ECDH Key object.
>    @param[in]  Group          Identifying number for the ECC group (IANA "Group
> -                             Description" attribute registrty for RFC 2409).
> +                             Description" attribute registry for RFC 2409).
>    @param[in]  EcPointPublic  Peer public key.
>    @param[out] SecretSize     On success, holds secret size.
>    @param[out] Secret         On success, holds the derived secret.
>                               Should be freed by caller using FreePool()
>                               function.
> 
> -  @retval EFI_SUCCESS        On success.
> -  @retval EFI_PROTOCOL_ERROR On failure.
> +  @retval EFI_SUCCESS           On success.
> +  @retval EFI_UNSUPPORTED       ECC group not supported.
> +  @retval EFI_INVALID_PARAMETER One or more of the following conditions is
> TRUE:
> +                                Secret is NULL.
> +                                SecretSize is NULL.
> +                                Public key in EcPointPublic is invalid.
> +  @retval EFI_PROTOCOL_ERROR    On failure.
>  **/
>  EFI_STATUS
>  EFIAPI
> diff --git a/CryptoPkg/Library/BaseCryptLibOnProtocolPpi/CryptLib.c
> b/CryptoPkg/Library/BaseCryptLibOnProtocolPpi/CryptLib.c
> index 548116abb4..0410067c9d 100644
> --- a/CryptoPkg/Library/BaseCryptLibOnProtocolPpi/CryptLib.c
> +++ b/CryptoPkg/Library/BaseCryptLibOnProtocolPpi/CryptLib.c
> @@ -4069,7 +4069,6 @@ BigNumValueOne (
>    @param[out]  BnRes   The result.
> 
>    @retval EFI_SUCCESS          On success.
> -  @retval EFI_OUT_OF_RESOURCES In case of internal allocation failures.
>    @retval EFI_PROTOCOL_ERROR   Otherwise.
>  **/
>  EFI_STATUS
> @@ -4158,6 +4157,9 @@ BigNumContextFree (
> 
>    @param[in]   Bn     Big number to set.
>    @param[in]   Val    Value to set.
> +
> +  @retval EFI_SUCCESS          On success.
> +  @retval EFI_PROTOCOL_ERROR   Otherwise.
>  **/
>  EFI_STATUS
>  EFIAPI
> @@ -4199,7 +4201,7 @@ BigNumAddMod (
>    using EcGroupFree() function.
> 
>    @param[in]  Group  Identifying number for the ECC group (IANA "Group
> -                     Description" attribute registrty for RFC 2409).
> +                     Description" attribute registry for RFC 2409).
> 
>    @retval EcGroup object  On success.
>    @retval NULL            On failure.
> @@ -4221,8 +4223,8 @@ EcGroupInit (
> 
>    @param[in]  EcGroup    EC group object.
>    @param[out] BnPrime    Group prime number.
> -  @param[out] BnA        A coofecient.
> -  @param[out] BnB        B coofecient.
> +  @param[out] BnA        A coefficient.
> +  @param[out] BnB        B coefficient.
>    @param[in]  BnCtx      BN context.
> 
>    @retval EFI_SUCCESS        On success.
> @@ -4533,13 +4535,14 @@ EcPointSetCompressedCoordinates (
>  /**
>    Generate a key using ECDH algorithm. Please note, this function uses
>    pseudo random number generator. The caller must make sure RandomSeed()
> -  funtion was properly called before.
> +  function was properly called before.
> 
>    @param[in]  Group    Identifying number for the ECC group (IANA "Group
> -                       Description" attribute registrty for RFC 2409).
> +                       Description" attribute registry for RFC 2409).
>    @param[out] PKey     Pointer to an object that will hold the ECDH key.
> 
>    @retval EFI_SUCCESS        On success.
> +  @retval EFI_UNSUPPORTED    ECC group not supported.
>    @retval EFI_PROTOCOL_ERROR On failure.
>  **/
>  EFI_STATUS
> @@ -4573,8 +4576,9 @@ EcDhKeyFree (
>    @param[in]  PKey     ECDH Key object.
>    @param[out] EcPoint  Properly initialized EC Point to hold the public key.
> 
> -  @retval EFI_SUCCESS        On success.
> -  @retval EFI_PROTOCOL_ERROR On failure.
> +  @retval EFI_SUCCESS           On success.
> +  @retval EFI_INVALID_PARAMETER EcPoint should be initialized properly.
> +  @retval EFI_PROTOCOL_ERROR    On failure.
>  **/
>  EFI_STATUS
>  EFIAPI
> @@ -4591,15 +4595,20 @@ EcDhGetPubKey (
> 
>    @param[in]  PKey           ECDH Key object.
>    @param[in]  Group          Identifying number for the ECC group (IANA "Group
> -                             Description" attribute registrty for RFC 2409).
> +                             Description" attribute registry for RFC 2409).
>    @param[in]  EcPointPublic  Peer public key.
>    @param[out] SecretSize     On success, holds secret size.
>    @param[out] Secret         On success, holds the derived secret.
>                               Should be freed by caller using FreePool()
>                               function.
> 
> -  @retval EFI_SUCCESS        On success.
> -  @retval EFI_PROTOCOL_ERROR On failure.
> +  @retval EFI_SUCCESS           On success.
> +  @retval EFI_UNSUPPORTED       ECC group not supported.
> +  @retval EFI_INVALID_PARAMETER One or more of the following conditions is
> TRUE:
> +                                Secret is NULL.
> +                                SecretSize is NULL.
> +                                Public key in EcPointPublic is invalid.
> +  @retval EFI_PROTOCOL_ERROR    On failure.
>  **/
>  EFI_STATUS
>  EFIAPI
> diff --git a/CryptoPkg/Private/Protocol/Crypto.h
> b/CryptoPkg/Private/Protocol/Crypto.h
> index 1b31714d77..1cf5d18cc3 100644
> --- a/CryptoPkg/Private/Protocol/Crypto.h
> +++ b/CryptoPkg/Private/Protocol/Crypto.h
> @@ -3863,7 +3863,6 @@ CONST VOID *
>    @param[out]  BnRes   The result, such that (BnA * BnB) % BnM.
> 
>    @retval EFI_SUCCESS          On success.
> -  @retval EFI_OUT_OF_RESOURCES In case of internal allocation failures.
>    @retval EFI_PROTOCOL_ERROR   Otherwise.
>  **/
>  typedef
> @@ -3935,6 +3934,9 @@ VOID
> 
>    @param[in]   Bn     Big number to set.
>    @param[in]   Val    Value to set.
> +
> +  @retval EFI_SUCCESS          On success.
> +  @retval EFI_PROTOCOL_ERROR   Otherwise.
>  **/
>  typedef
>  EFI_STATUS
> @@ -3970,7 +3972,7 @@ EFI_STATUS
>    using EcGroupFree() function.
> 
>    @param[in]  Group  Identifying number for the ECC group (IANA "Group
> -                     Description" attribute registrty for RFC 2409)
> +                     Description" attribute registry for RFC 2409)
> 
>    @retval EcGroup object  On success
>    @retval NULL            On failure
> @@ -3989,8 +3991,8 @@ VOID *
> 
>    @param[in]  EcGroup    EC group object
>    @param[out] BnPrime    Group prime number
> -  @param[out] BnA        A coofecient
> -  @param[out] BnB        B coofecient
> +  @param[out] BnA        A coefficient
> +  @param[out] BnB        B coefficient
>    @param[in]  BnCtx      BN context
> 
>    @retval EFI_SUCCESS        On success
> @@ -4260,13 +4262,14 @@ EFI_STATUS
>  /**
>    Generate a key using ECDH algorithm. Please note, this function uses
>    pseudo random number generator. The caller must make sure RandomSeed()
> -  funtion was properly called before.
> +  function was properly called before.
> 
>    @param[in]  Group    Identifying number for the ECC group (IANA "Group
> -                       Description" attribute registrty for RFC 2409)
> +                       Description" attribute registry for RFC 2409)
>    @param[out] PKey     Pointer to an object that will hold the ECDH key
> 
>    @retval EFI_SUCCESS        On success
> +  @retval EFI_UNSUPPORTED    ECC group not supported.
>    @retval EFI_PROTOCOL_ERROR On failure  **/  typedef @@ -4294,8 +4297,9
> @@ VOID
>    @param[in]  PKey     ECDH Key object
>    @param[out] EcPoint  Properly initialized EC Point to hold the public key
> 
> -  @retval EFI_SUCCESS        On success
> -  @retval EFI_PROTOCOL_ERROR On failure
> +  @retval EFI_SUCCESS           On success
> +  @retval EFI_INVALID_PARAMETER EcPoint should be initialized properly.
> +  @retval EFI_PROTOCOL_ERROR    On failure
>  **/
>  typedef
>  EFI_STATUS
> @@ -4309,15 +4313,20 @@ EFI_STATUS
> 
>    @param[in]  PKey           ECDH Key object
>    @param[in]  Group          Identifying number for the ECC group (IANA "Group
> -                             Description" attribute registrty for RFC 2409)
> +                             Description" attribute registry for RFC
> + 2409)
>    @param[in]  EcPointPublic  Peer public key
>    @param[out] SecretSize     On success, holds secret size
>    @param[out] Secret         On success, holds the derived secret
>                               Should be freed by caller using FreePool()
>                               function.
> 
> -  @retval EFI_SUCCESS        On success
> -  @retval EFI_PROTOCOL_ERROR On failure
> +  @retval EFI_SUCCESS           On success.
> +  @retval EFI_UNSUPPORTED       ECC group not supported.
> +  @retval EFI_INVALID_PARAMETER One or more of the following conditions is
> TRUE:
> +                                Secret is NULL.
> +                                SecretSize is NULL.
> +                                Public key in EcPointPublic is invalid.
> +  @retval EFI_PROTOCOL_ERROR    On failure.
>  **/
>  typedef
>  EFI_STATUS
> --
> 2.31.1.windows.1


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [staging/crypto-new-api PATCH] CryptoPkg: Fix issues from crypto code review.
  2022-07-15  2:13 ` Heng Luo
@ 2022-07-15  3:31   ` Tan, Ming
  0 siblings, 0 replies; 3+ messages in thread
From: Tan, Ming @ 2022-07-15  3:31 UTC (permalink / raw)
  To: Luo, Heng, Li, Yi1, devel@edk2.groups.io

Yi:
  I have the following suggestion.
  1. Suggest to split the patch to several patchs. Special the code which modify the logical.
  2. Change the commit title, "fix the issues the code review" sounds like show that the reviewers did not good job at before 😉

  BR/Tan Ming.

-----Original Message-----
From: Luo, Heng <heng.luo@intel.com> 
Sent: Friday, July 15, 2022 10:13 AM
To: Li, Yi1 <yi1.li@intel.com>; devel@edk2.groups.io
Cc: Tan, Ming <ming.tan@intel.com>
Subject: RE: [staging/crypto-new-api PATCH] CryptoPkg: Fix issues from crypto code review.

Reviewed-by: Heng Luo <heng.luo@intel.com>

> -----Original Message-----
> From: Li, Yi1 <yi1.li@intel.com>
> Sent: Friday, July 15, 2022 10:07 AM
> To: devel@edk2.groups.io
> Cc: Li, Yi1 <yi1.li@intel.com>; Tan, Ming <ming.tan@intel.com>; Luo, 
> Heng <heng.luo@intel.com>
> Subject: [staging/crypto-new-api PATCH] CryptoPkg: Fix issues from 
> crypto code review.
> 
> Details:
> 1. Some APIs need more detail comment.
> 2. Correct BnRShift() param order.
> 3. Remove unsecure ECC curve from GroupToNid().
> 4. Add full public key validating procedures to EcDhDeriveSecret().
> 
> Cc: Ming Tan <ming.tan@intel.com>
> Cc: Heng Luo <heng.luo@intel.com>
> Signed-off-by: Yi Li <yi1.li@intel.com>
> 
> ---
>  CryptoPkg/Driver/Crypto.c                              | 31 ++++++++++++++++++++---------
> --
>  CryptoPkg/Include/Library/BaseCryptLib.h               | 31
> ++++++++++++++++++++-----------
>  CryptoPkg/Library/BaseCryptLib/Bn/CryptBn.c            |  7 ++++---
>  CryptoPkg/Library/BaseCryptLib/Bn/CryptBnNull.c        |  4 +++-
>  CryptoPkg/Library/BaseCryptLib/Ec/CryptEc.c            | 61
> ++++++++++++++++++++++++++++++++++---------------------------
>  CryptoPkg/Library/BaseCryptLib/Ec/CryptEcNull.c        | 27
> +++++++++++++++++----------
>  CryptoPkg/Library/BaseCryptLibNull/Bn/CryptBnNull.c    |  4 +++-
>  CryptoPkg/Library/BaseCryptLibNull/Ec/CryptEcNull.c    | 27
> +++++++++++++++++----------
>  CryptoPkg/Library/BaseCryptLibOnProtocolPpi/CryptLib.c | 31
> ++++++++++++++++++++-----------
>  CryptoPkg/Private/Protocol/Crypto.h                    | 31 ++++++++++++++++++++--
> ---------
>  10 files changed, 158 insertions(+), 96 deletions(-)
> 
> diff --git a/CryptoPkg/Driver/Crypto.c b/CryptoPkg/Driver/Crypto.c 
> index
> de422b7f53..10a0ce8800 100644
> --- a/CryptoPkg/Driver/Crypto.c
> +++ b/CryptoPkg/Driver/Crypto.c
> @@ -4962,7 +4962,6 @@ CryptoServiceBigNumValueOne (
>    @param[out]  BnRes   The result.
> 
>    @retval EFI_SUCCESS          On success.
> -  @retval EFI_OUT_OF_RESOURCES In case of internal allocation failures.
>    @retval EFI_PROTOCOL_ERROR   Otherwise.
>  **/
>  EFI_STATUS
> @@ -5051,6 +5050,9 @@ CryptoServiceBigNumContextFree (
> 
>    @param[in]   Bn     Big number to set.
>    @param[in]   Val    Value to set.
> +
> +  @retval EFI_SUCCESS          On success.
> +  @retval EFI_PROTOCOL_ERROR   Otherwise.
>  **/
>  EFI_STATUS
>  EFIAPI
> @@ -5092,7 +5094,7 @@ CryptoServiceBigNumAddMod (
>    using EcGroupFree() function.
> 
>    @param[in]  Group  Identifying number for the ECC group (IANA "Group
> -                     Description" attribute registrty for RFC 2409).
> +                     Description" attribute registry for RFC 2409).
> 
>    @retval EcGroup object  On success.
>    @retval NULL            On failure.
> @@ -5114,8 +5116,8 @@ CryptoServiceEcGroupInit (
> 
>    @param[in]  EcGroup    EC group object.
>    @param[out] BnPrime    Group prime number.
> -  @param[out] BnA        A coofecient.
> -  @param[out] BnB        B coofecient.
> +  @param[out] BnA        A coefficient.
> +  @param[out] BnB        B coefficient.
>    @param[in]  BnCtx      BN context.
> 
>    @retval EFI_SUCCESS        On success.
> @@ -5426,13 +5428,14 @@ CryptoServiceEcPointSetCompressedCoordinates (
>  /**
>    Generate a key using ECDH algorithm. Please note, this function uses
>    pseudo random number generator. The caller must make sure 
> RandomSeed()
> -  funtion was properly called before.
> +  function was properly called before.
> 
>    @param[in]  Group    Identifying number for the ECC group (IANA "Group
> -                       Description" attribute registrty for RFC 2409).
> +                       Description" attribute registry for RFC 2409).
>    @param[out] PKey     Pointer to an object that will hold the ECDH key.
> 
>    @retval EFI_SUCCESS        On success.
> +  @retval EFI_UNSUPPORTED    ECC group not supported.
>    @retval EFI_PROTOCOL_ERROR On failure.
>  **/
>  EFI_STATUS
> @@ -5466,8 +5469,9 @@ CryptoServiceEcDhKeyFree (
>    @param[in]  PKey     ECDH Key object.
>    @param[out] EcPoint  Properly initialized EC Point to hold the public key.
> 
> -  @retval EFI_SUCCESS        On success.
> -  @retval EFI_PROTOCOL_ERROR On failure.
> +  @retval EFI_SUCCESS           On success.
> +  @retval EFI_INVALID_PARAMETER EcPoint should be initialized properly.
> +  @retval EFI_PROTOCOL_ERROR    On failure.
>  **/
>  EFI_STATUS
>  EFIAPI
> @@ -5484,15 +5488,20 @@ CryptoServiceEcDhGetPubKey (
> 
>    @param[in]  PKey           ECDH Key object.
>    @param[in]  Group          Identifying number for the ECC group (IANA "Group
> -                             Description" attribute registrty for RFC 2409).
> +                             Description" attribute registry for RFC 2409).
>    @param[in]  EcPointPublic  Peer public key.
>    @param[out] SecretSize     On success, holds secret size.
>    @param[out] Secret         On success, holds the derived secret.
>                               Should be freed by caller using FreePool()
>                               function.
> 
> -  @retval EFI_SUCCESS        On success.
> -  @retval EFI_PROTOCOL_ERROR On failure.
> +  @retval EFI_SUCCESS           On success.
> +  @retval EFI_UNSUPPORTED       ECC group not supported.
> +  @retval EFI_INVALID_PARAMETER One or more of the following 
> + conditions is
> TRUE:
> +                                Secret is NULL.
> +                                SecretSize is NULL.
> +                                Public key in EcPointPublic is invalid.
> +  @retval EFI_PROTOCOL_ERROR    On failure.
>  **/
>  EFI_STATUS
>  EFIAPI
> diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h
> b/CryptoPkg/Include/Library/BaseCryptLib.h
> index 8fcb496c40..0de9f0739e 100644
> --- a/CryptoPkg/Include/Library/BaseCryptLib.h
> +++ b/CryptoPkg/Include/Library/BaseCryptLib.h
> @@ -2723,7 +2723,6 @@ BigNumValueOne (
>    @param[out]  BnRes   The result.
> 
>    @retval EFI_SUCCESS          On success.
> -  @retval EFI_OUT_OF_RESOURCES In case of internal allocation failures.
>    @retval EFI_PROTOCOL_ERROR   Otherwise.
>  **/
>  EFI_STATUS
> @@ -2797,6 +2796,9 @@ BigNumContextFree (
> 
>    @param[in]   Bn     Big number to set.
>    @param[in]   Val    Value to set.
> +
> +  @retval EFI_SUCCESS          On success.
> +  @retval EFI_PROTOCOL_ERROR   Otherwise.
>  **/
>  EFI_STATUS
>  EFIAPI
> @@ -2832,7 +2834,7 @@ BigNumAddMod (
>    using EcGroupFree() function.
> 
>    @param[in]  Group  Identifying number for the ECC group (IANA "Group
> -                     Description" attribute registrty for RFC 2409).
> +                     Description" attribute registry for RFC 2409).
> 
>    @retval EcGroup object  On success.
>    @retval NULL            On failure.
> @@ -2851,8 +2853,8 @@ EcGroupInit (
> 
>    @param[in]  EcGroup    EC group object.
>    @param[out] BnPrime    Group prime number.
> -  @param[out] BnA        A coofecient.
> -  @param[out] BnB        B coofecient.
> +  @param[out] BnA        A coefficient.
> +  @param[out] BnB        B coefficient.
>    @param[in]  BnCtx      BN context.
> 
>    @retval EFI_SUCCESS        On success.
> @@ -3121,13 +3123,14 @@ EcPointSetCompressedCoordinates (
>  /**
>    Generate a key using ECDH algorithm. Please note, this function uses
>    pseudo random number generator. The caller must make sure 
> RandomSeed()
> -  funtion was properly called before.
> +  function was properly called before.
> 
>    @param[in]  Group    Identifying number for the ECC group (IANA "Group
> -                       Description" attribute registrty for RFC 2409).
> +                       Description" attribute registry for RFC 2409).
>    @param[out] PKey     Pointer to an object that will hold the ECDH key.
> 
>    @retval EFI_SUCCESS        On success.
> +  @retval EFI_UNSUPPORTED    ECC group not supported.
>    @retval EFI_PROTOCOL_ERROR On failure.
>  **/
>  EFI_STATUS
> @@ -3155,8 +3158,9 @@ EcDhKeyFree (
>    @param[in]  PKey     ECDH Key object.
>    @param[out] EcPoint  Properly initialized EC Point to hold the public key.
> 
> -  @retval EFI_SUCCESS        On success.
> -  @retval EFI_PROTOCOL_ERROR On failure.
> +  @retval EFI_SUCCESS           On success.
> +  @retval EFI_INVALID_PARAMETER EcPoint should be initialized properly.
> +  @retval EFI_PROTOCOL_ERROR    On failure.
>  **/
>  EFI_STATUS
>  EFIAPI
> @@ -3170,15 +3174,20 @@ EcDhGetPubKey (
> 
>    @param[in]  PKey           ECDH Key object.
>    @param[in]  Group          Identifying number for the ECC group (IANA "Group
> -                             Description" attribute registrty for RFC 2409).
> +                             Description" attribute registry for RFC 2409).
>    @param[in]  EcPointPublic  Peer public key.
>    @param[out] SecretSize     On success, holds secret size.
>    @param[out] Secret         On success, holds the derived secret.
>                               Should be freed by caller using FreePool()
>                               function.
> 
> -  @retval EFI_SUCCESS        On success.
> -  @retval EFI_PROTOCOL_ERROR On failure.
> +  @retval EFI_SUCCESS           On success.
> +  @retval EFI_UNSUPPORTED       ECC group not supported.
> +  @retval EFI_INVALID_PARAMETER One or more of the following 
> + conditions is
> TRUE:
> +                                Secret is NULL.
> +                                SecretSize is NULL.
> +                                Public key in EcPointPublic is invalid.
> +  @retval EFI_PROTOCOL_ERROR    On failure.
>  **/
>  EFI_STATUS
>  EFIAPI
> diff --git a/CryptoPkg/Library/BaseCryptLib/Bn/CryptBn.c
> b/CryptoPkg/Library/BaseCryptLib/Bn/CryptBn.c
> index 3e43492a56..b6411cd541 100644
> --- a/CryptoPkg/Library/BaseCryptLib/Bn/CryptBn.c
> +++ b/CryptoPkg/Library/BaseCryptLib/Bn/CryptBn.c
> @@ -442,7 +442,6 @@ BigNumValueOne (
>    @param[out]  BnRes   The result.
> 
>    @retval EFI_SUCCESS          On success.
> -  @retval EFI_OUT_OF_RESOURCES In case of internal allocation failures.
>    @retval EFI_PROTOCOL_ERROR   Otherwise.
>  **/
>  EFI_STATUS
> @@ -453,8 +452,7 @@ BigNumRShift (
>    OUT VOID       *BnRes
>    )
>  {
> -  // BN_rshift() does not modify the first argument, so we remove const.
> -  if (BN_rshift ((BIGNUM *)Bn, BnRes, (int)n) == 1) {
> +  if (BN_rshift (BnRes, Bn, (int)n) == 1) {
>      return EFI_SUCCESS;
>    } else {
>      return EFI_PROTOCOL_ERROR;
> @@ -547,6 +545,9 @@ BigNumContextFree (
> 
>    @param[in]   Bn     Big number to set.
>    @param[in]   Val    Value to set.
> +
> +  @retval EFI_SUCCESS          On success.
> +  @retval EFI_PROTOCOL_ERROR   Otherwise.
>  **/
>  EFI_STATUS
>  EFIAPI
> diff --git a/CryptoPkg/Library/BaseCryptLib/Bn/CryptBnNull.c
> b/CryptoPkg/Library/BaseCryptLib/Bn/CryptBnNull.c
> index 4a27433a0e..4d2fa039df 100644
> --- a/CryptoPkg/Library/BaseCryptLib/Bn/CryptBnNull.c
> +++ b/CryptoPkg/Library/BaseCryptLib/Bn/CryptBnNull.c
> @@ -395,7 +395,6 @@ BigNumValueOne (
>    @param[out]  BnRes   The result.
> 
>    @retval EFI_SUCCESS          On success.
> -  @retval EFI_OUT_OF_RESOURCES In case of internal allocation failures.
>    @retval EFI_PROTOCOL_ERROR   Otherwise.
>  **/
>  EFI_STATUS
> @@ -487,6 +486,9 @@ BigNumContextFree (
> 
>    @param[in]   Bn     Big number to set.
>    @param[in]   Val    Value to set.
> +
> +  @retval EFI_SUCCESS          On success.
> +  @retval EFI_PROTOCOL_ERROR   Otherwise.
>  **/
>  EFI_STATUS
>  EFIAPI
> diff --git a/CryptoPkg/Library/BaseCryptLib/Ec/CryptEc.c
> b/CryptoPkg/Library/BaseCryptLib/Ec/CryptEc.c
> index 4d1aab8d32..90d1b8bce7 100644
> --- a/CryptoPkg/Library/BaseCryptLib/Ec/CryptEc.c
> +++ b/CryptoPkg/Library/BaseCryptLib/Ec/CryptEc.c
> @@ -21,13 +21,13 @@
>  #include <openssl/ec.h>
> 
>  /**
> -  Temp comment.
> +  Return the Nid of certain ECC group.
> 
>    @param[in]  Group  Identifying number for the ECC group (IANA "Group
> -                     Description" attribute registrty for RFC 2409).
> +                     Description" attribute registry for RFC 2409).
> 
> -  @retval EcGroup object  On success.
> -  @retval NULL            On failure.
> +  @retval !=-1    On success.
> +  @retval -1      ECC group not supported.
>  **/
>  STATIC
>  INT32
> @@ -47,12 +47,6 @@ GroupToNid (
>      case 21:
>        Nid = NID_secp521r1;
>        break;
> -    case 25:
> -      Nid = NID_X9_62_prime192v1;
> -      break;
> -    case 26:
> -      Nid = NID_secp224r1;
> -      break;
>      default:
>        return -1;
>    }
> @@ -66,7 +60,7 @@ GroupToNid (
>    using EcGroupFree() function.
> 
>    @param[in]  Group  Identifying number for the ECC group (IANA "Group
> -                     Description" attribute registrty for RFC 2409).
> +                     Description" attribute registry for RFC 2409).
> 
>    @retval EcGroup object  On success.
>    @retval NULL            On failure.
> @@ -96,8 +90,8 @@ EcGroupInit (
> 
>    @param[in]  EcGroup    EC group object.
>    @param[out] BnPrime    Group prime number.
> -  @param[out] BnA        A coofecient.
> -  @param[out] BnB        B coofecient.
> +  @param[out] BnA        A coefficient.
> +  @param[out] BnB        B coefficient.
>    @param[in]  BnCtx      BN context.
> 
>    @retval EFI_SUCCESS        On success.
> @@ -218,7 +212,7 @@ EcPointGetAffineCoordinates (
>    )
>  {
>    return EC_POINT_get_affine_coordinates (EcGroup, EcPoint, BnX, BnY, BnCtx) ?
> -         EFI_SUCCESS : EFI_INVALID_PARAMETER;
> +         EFI_SUCCESS : EFI_PROTOCOL_ERROR;
>  }
> 
>  /**
> @@ -244,7 +238,7 @@ EcPointSetAffineCoordinates (
>    )
>  {
>    return EC_POINT_set_affine_coordinates (EcGroup, EcPoint, BnX, BnY, BnCtx) ?
> -         EFI_SUCCESS : EFI_INVALID_PARAMETER;
> +         EFI_SUCCESS : EFI_PROTOCOL_ERROR;
>  }
> 
>  /**
> @@ -271,7 +265,7 @@ EcPointAdd (
>    )
>  {
>    return EC_POINT_add (EcGroup, EcPointResult, EcPointA, EcPointB, BnCtx) ?
> -         EFI_SUCCESS : EFI_INVALID_PARAMETER;
> +         EFI_SUCCESS : EFI_PROTOCOL_ERROR;
>  }
> 
>  /**
> @@ -298,7 +292,7 @@ EcPointMul (
>    )
>  {
>    return EC_POINT_mul (EcGroup, EcPointResult, NULL, EcPoint, 
> BnPScalar,
> BnCtx) ?
> -         EFI_SUCCESS : EFI_INVALID_PARAMETER;
> +         EFI_SUCCESS : EFI_PROTOCOL_ERROR;
>  }
> 
>  /**
> @@ -320,7 +314,7 @@ EcPointInvert (
>    )
>  {
>    return EC_POINT_invert (EcGroup, EcPoint, BnCtx) ?
> -         EFI_SUCCESS : EFI_INVALID_PARAMETER;
> +         EFI_SUCCESS : EFI_PROTOCOL_ERROR;
>  }
> 
>  /**
> @@ -414,19 +408,20 @@ EcPointSetCompressedCoordinates (
>    )
>  {
>    return EC_POINT_set_compressed_coordinates (EcGroup, EcPoint, BnX, 
> YBit,
> BnCtx) ?
> -         EFI_SUCCESS : EFI_INVALID_PARAMETER;
> +         EFI_SUCCESS : EFI_PROTOCOL_ERROR;
>  }
> 
>  /**
>    Generate a key using ECDH algorithm. Please note, this function uses
>    pseudo random number generator. The caller must make sure 
> RandomSeed()
> -  funtion was properly called before.
> +  function was properly called before.
> 
>    @param[in]  Group    Identifying number for the ECC group (IANA "Group
> -                       Description" attribute registrty for RFC 2409).
> +                       Description" attribute registry for RFC 2409).
>    @param[out] PKey     Pointer to an object that will hold the ECDH key.
> 
>    @retval EFI_SUCCESS        On success.
> +  @retval EFI_UNSUPPORTED    ECC group not supported.
>    @retval EFI_PROTOCOL_ERROR On failure.
>  **/
>  EFI_STATUS
> @@ -508,8 +503,9 @@ EcDhKeyFree (
>    @param[in]  PKey     ECDH Key object.
>    @param[out] EcPoint  Properly initialized EC Point to hold the public key.
> 
> -  @retval EFI_SUCCESS        On success.
> -  @retval EFI_PROTOCOL_ERROR On failure.
> +  @retval EFI_SUCCESS           On success.
> +  @retval EFI_INVALID_PARAMETER EcPoint should be initialized properly.
> +  @retval EFI_PROTOCOL_ERROR    On failure.
>  **/
>  EFI_STATUS
>  EFIAPI
> @@ -553,15 +549,21 @@ out:
> 
>    @param[in]  PKey           ECDH Key object.
>    @param[in]  Group          Identifying number for the ECC group (IANA "Group
> -                             Description" attribute registrty for RFC 2409).
> -  @param[in]  EcPointPublic  Peer public key.
> +                             Description" attribute registry for RFC 2409).
> +  @param[in]  EcPointPublic  Peer public key. Certain sanity checks on the key
> +                             will be performed to confirm that it is valid.
>    @param[out] SecretSize     On success, holds secret size.
>    @param[out] Secret         On success, holds the derived secret.
>                               Should be freed by caller using FreePool()
>                               function.
> 
> -  @retval EFI_SUCCESS        On success.
> -  @retval EFI_PROTOCOL_ERROR On failure.
> +  @retval EFI_SUCCESS           On success.
> +  @retval EFI_UNSUPPORTED       ECC group not supported.
> +  @retval EFI_INVALID_PARAMETER One or more of the following 
> + conditions is
> TRUE:
> +                                Secret is NULL.
> +                                SecretSize is NULL.
> +                                Public key in EcPointPublic is invalid.
> +  @retval EFI_PROTOCOL_ERROR    On failure.
>  **/
>  EFI_STATUS
>  EFIAPI
> @@ -605,6 +607,11 @@ EcDhDeriveSecret (
>      goto fail;
>    }
> 
> +  if (!EC_KEY_check_key (EcKey)) {
> +    Status = EFI_INVALID_PARAMETER;
> +    goto fail;
> +  }
> +
>    Ctx = EVP_PKEY_CTX_new (PKey, NULL);
>    if ((Ctx == NULL) || (EVP_PKEY_derive_init (Ctx) != 1) ||
>        (EVP_PKEY_derive_set_peer (Ctx, PeerKey) != 1) || diff --git 
> a/CryptoPkg/Library/BaseCryptLib/Ec/CryptEcNull.c
> b/CryptoPkg/Library/BaseCryptLib/Ec/CryptEcNull.c
> index 2d7e5db464..e7fe378095 100644
> --- a/CryptoPkg/Library/BaseCryptLib/Ec/CryptEcNull.c
> +++ b/CryptoPkg/Library/BaseCryptLib/Ec/CryptEcNull.c
> @@ -15,7 +15,7 @@
>    using EcGroupFree() function.
> 
>    @param[in]  Group  Identifying number for the ECC group (IANA "Group
> -                     Description" attribute registrty for RFC 2409).
> +                     Description" attribute registry for RFC 2409).
> 
>    @retval EcGroup object  On success.
>    @retval NULL            On failure.
> @@ -38,8 +38,8 @@ EcGroupInit (
> 
>    @param[in]  EcGroup    EC group object.
>    @param[out] BnPrime    Group prime number.
> -  @param[out] BnA        A coofecient.
> -  @param[out] BnB        B coofecient.
> +  @param[out] BnA        A coefficient.
> +  @param[out] BnB        B coefficient.
>    @param[in]  BnCtx      BN context.
> 
>    @retval EFI_SUCCESS        On success.
> @@ -362,13 +362,14 @@ EcPointSetCompressedCoordinates (
>  /**
>    Generate a key using ECDH algorithm. Please note, this function uses
>    pseudo random number generator. The caller must make sure 
> RandomSeed()
> -  funtion was properly called before.
> +  function was properly called before.
> 
>    @param[in]  Group    Identifying number for the ECC group (IANA "Group
> -                       Description" attribute registrty for RFC 2409).
> +                       Description" attribute registry for RFC 2409).
>    @param[out] PKey     Pointer to an object that will hold the ECDH key.
> 
>    @retval EFI_SUCCESS        On success.
> +  @retval EFI_UNSUPPORTED    ECC group not supported.
>    @retval EFI_PROTOCOL_ERROR On failure.
>  **/
>  EFI_STATUS
> @@ -403,8 +404,9 @@ EcDhKeyFree (
>    @param[in]  PKey     ECDH Key object.
>    @param[out] EcPoint  Properly initialized EC Point to hold the public key.
> 
> -  @retval EFI_SUCCESS        On success.
> -  @retval EFI_PROTOCOL_ERROR On failure.
> +  @retval EFI_SUCCESS           On success.
> +  @retval EFI_INVALID_PARAMETER EcPoint should be initialized properly.
> +  @retval EFI_PROTOCOL_ERROR    On failure.
>  **/
>  EFI_STATUS
>  EFIAPI
> @@ -422,15 +424,20 @@ EcDhGetPubKey (
> 
>    @param[in]  PKey           ECDH Key object.
>    @param[in]  Group          Identifying number for the ECC group (IANA "Group
> -                             Description" attribute registrty for RFC 2409).
> +                             Description" attribute registry for RFC 2409).
>    @param[in]  EcPointPublic  Peer public key.
>    @param[out] SecretSize     On success, holds secret size.
>    @param[out] Secret         On success, holds the derived secret.
>                               Should be freed by caller using FreePool()
>                               function.
> 
> -  @retval EFI_SUCCESS        On success.
> -  @retval EFI_PROTOCOL_ERROR On failure.
> +  @retval EFI_SUCCESS           On success.
> +  @retval EFI_UNSUPPORTED       ECC group not supported.
> +  @retval EFI_INVALID_PARAMETER One or more of the following 
> + conditions is
> TRUE:
> +                                Secret is NULL.
> +                                SecretSize is NULL.
> +                                Public key in EcPointPublic is invalid.
> +  @retval EFI_PROTOCOL_ERROR    On failure.
>  **/
>  EFI_STATUS
>  EFIAPI
> diff --git a/CryptoPkg/Library/BaseCryptLibNull/Bn/CryptBnNull.c
> b/CryptoPkg/Library/BaseCryptLibNull/Bn/CryptBnNull.c
> index 4a27433a0e..4d2fa039df 100644
> --- a/CryptoPkg/Library/BaseCryptLibNull/Bn/CryptBnNull.c
> +++ b/CryptoPkg/Library/BaseCryptLibNull/Bn/CryptBnNull.c
> @@ -395,7 +395,6 @@ BigNumValueOne (
>    @param[out]  BnRes   The result.
> 
>    @retval EFI_SUCCESS          On success.
> -  @retval EFI_OUT_OF_RESOURCES In case of internal allocation failures.
>    @retval EFI_PROTOCOL_ERROR   Otherwise.
>  **/
>  EFI_STATUS
> @@ -487,6 +486,9 @@ BigNumContextFree (
> 
>    @param[in]   Bn     Big number to set.
>    @param[in]   Val    Value to set.
> +
> +  @retval EFI_SUCCESS          On success.
> +  @retval EFI_PROTOCOL_ERROR   Otherwise.
>  **/
>  EFI_STATUS
>  EFIAPI
> diff --git a/CryptoPkg/Library/BaseCryptLibNull/Ec/CryptEcNull.c
> b/CryptoPkg/Library/BaseCryptLibNull/Ec/CryptEcNull.c
> index 2d7e5db464..e7fe378095 100644
> --- a/CryptoPkg/Library/BaseCryptLibNull/Ec/CryptEcNull.c
> +++ b/CryptoPkg/Library/BaseCryptLibNull/Ec/CryptEcNull.c
> @@ -15,7 +15,7 @@
>    using EcGroupFree() function.
> 
>    @param[in]  Group  Identifying number for the ECC group (IANA "Group
> -                     Description" attribute registrty for RFC 2409).
> +                     Description" attribute registry for RFC 2409).
> 
>    @retval EcGroup object  On success.
>    @retval NULL            On failure.
> @@ -38,8 +38,8 @@ EcGroupInit (
> 
>    @param[in]  EcGroup    EC group object.
>    @param[out] BnPrime    Group prime number.
> -  @param[out] BnA        A coofecient.
> -  @param[out] BnB        B coofecient.
> +  @param[out] BnA        A coefficient.
> +  @param[out] BnB        B coefficient.
>    @param[in]  BnCtx      BN context.
> 
>    @retval EFI_SUCCESS        On success.
> @@ -362,13 +362,14 @@ EcPointSetCompressedCoordinates (
>  /**
>    Generate a key using ECDH algorithm. Please note, this function uses
>    pseudo random number generator. The caller must make sure 
> RandomSeed()
> -  funtion was properly called before.
> +  function was properly called before.
> 
>    @param[in]  Group    Identifying number for the ECC group (IANA "Group
> -                       Description" attribute registrty for RFC 2409).
> +                       Description" attribute registry for RFC 2409).
>    @param[out] PKey     Pointer to an object that will hold the ECDH key.
> 
>    @retval EFI_SUCCESS        On success.
> +  @retval EFI_UNSUPPORTED    ECC group not supported.
>    @retval EFI_PROTOCOL_ERROR On failure.
>  **/
>  EFI_STATUS
> @@ -403,8 +404,9 @@ EcDhKeyFree (
>    @param[in]  PKey     ECDH Key object.
>    @param[out] EcPoint  Properly initialized EC Point to hold the public key.
> 
> -  @retval EFI_SUCCESS        On success.
> -  @retval EFI_PROTOCOL_ERROR On failure.
> +  @retval EFI_SUCCESS           On success.
> +  @retval EFI_INVALID_PARAMETER EcPoint should be initialized properly.
> +  @retval EFI_PROTOCOL_ERROR    On failure.
>  **/
>  EFI_STATUS
>  EFIAPI
> @@ -422,15 +424,20 @@ EcDhGetPubKey (
> 
>    @param[in]  PKey           ECDH Key object.
>    @param[in]  Group          Identifying number for the ECC group (IANA "Group
> -                             Description" attribute registrty for RFC 2409).
> +                             Description" attribute registry for RFC 2409).
>    @param[in]  EcPointPublic  Peer public key.
>    @param[out] SecretSize     On success, holds secret size.
>    @param[out] Secret         On success, holds the derived secret.
>                               Should be freed by caller using FreePool()
>                               function.
> 
> -  @retval EFI_SUCCESS        On success.
> -  @retval EFI_PROTOCOL_ERROR On failure.
> +  @retval EFI_SUCCESS           On success.
> +  @retval EFI_UNSUPPORTED       ECC group not supported.
> +  @retval EFI_INVALID_PARAMETER One or more of the following 
> + conditions is
> TRUE:
> +                                Secret is NULL.
> +                                SecretSize is NULL.
> +                                Public key in EcPointPublic is invalid.
> +  @retval EFI_PROTOCOL_ERROR    On failure.
>  **/
>  EFI_STATUS
>  EFIAPI
> diff --git a/CryptoPkg/Library/BaseCryptLibOnProtocolPpi/CryptLib.c
> b/CryptoPkg/Library/BaseCryptLibOnProtocolPpi/CryptLib.c
> index 548116abb4..0410067c9d 100644
> --- a/CryptoPkg/Library/BaseCryptLibOnProtocolPpi/CryptLib.c
> +++ b/CryptoPkg/Library/BaseCryptLibOnProtocolPpi/CryptLib.c
> @@ -4069,7 +4069,6 @@ BigNumValueOne (
>    @param[out]  BnRes   The result.
> 
>    @retval EFI_SUCCESS          On success.
> -  @retval EFI_OUT_OF_RESOURCES In case of internal allocation failures.
>    @retval EFI_PROTOCOL_ERROR   Otherwise.
>  **/
>  EFI_STATUS
> @@ -4158,6 +4157,9 @@ BigNumContextFree (
> 
>    @param[in]   Bn     Big number to set.
>    @param[in]   Val    Value to set.
> +
> +  @retval EFI_SUCCESS          On success.
> +  @retval EFI_PROTOCOL_ERROR   Otherwise.
>  **/
>  EFI_STATUS
>  EFIAPI
> @@ -4199,7 +4201,7 @@ BigNumAddMod (
>    using EcGroupFree() function.
> 
>    @param[in]  Group  Identifying number for the ECC group (IANA "Group
> -                     Description" attribute registrty for RFC 2409).
> +                     Description" attribute registry for RFC 2409).
> 
>    @retval EcGroup object  On success.
>    @retval NULL            On failure.
> @@ -4221,8 +4223,8 @@ EcGroupInit (
> 
>    @param[in]  EcGroup    EC group object.
>    @param[out] BnPrime    Group prime number.
> -  @param[out] BnA        A coofecient.
> -  @param[out] BnB        B coofecient.
> +  @param[out] BnA        A coefficient.
> +  @param[out] BnB        B coefficient.
>    @param[in]  BnCtx      BN context.
> 
>    @retval EFI_SUCCESS        On success.
> @@ -4533,13 +4535,14 @@ EcPointSetCompressedCoordinates (
>  /**
>    Generate a key using ECDH algorithm. Please note, this function uses
>    pseudo random number generator. The caller must make sure 
> RandomSeed()
> -  funtion was properly called before.
> +  function was properly called before.
> 
>    @param[in]  Group    Identifying number for the ECC group (IANA "Group
> -                       Description" attribute registrty for RFC 2409).
> +                       Description" attribute registry for RFC 2409).
>    @param[out] PKey     Pointer to an object that will hold the ECDH key.
> 
>    @retval EFI_SUCCESS        On success.
> +  @retval EFI_UNSUPPORTED    ECC group not supported.
>    @retval EFI_PROTOCOL_ERROR On failure.
>  **/
>  EFI_STATUS
> @@ -4573,8 +4576,9 @@ EcDhKeyFree (
>    @param[in]  PKey     ECDH Key object.
>    @param[out] EcPoint  Properly initialized EC Point to hold the public key.
> 
> -  @retval EFI_SUCCESS        On success.
> -  @retval EFI_PROTOCOL_ERROR On failure.
> +  @retval EFI_SUCCESS           On success.
> +  @retval EFI_INVALID_PARAMETER EcPoint should be initialized properly.
> +  @retval EFI_PROTOCOL_ERROR    On failure.
>  **/
>  EFI_STATUS
>  EFIAPI
> @@ -4591,15 +4595,20 @@ EcDhGetPubKey (
> 
>    @param[in]  PKey           ECDH Key object.
>    @param[in]  Group          Identifying number for the ECC group (IANA "Group
> -                             Description" attribute registrty for RFC 2409).
> +                             Description" attribute registry for RFC 2409).
>    @param[in]  EcPointPublic  Peer public key.
>    @param[out] SecretSize     On success, holds secret size.
>    @param[out] Secret         On success, holds the derived secret.
>                               Should be freed by caller using FreePool()
>                               function.
> 
> -  @retval EFI_SUCCESS        On success.
> -  @retval EFI_PROTOCOL_ERROR On failure.
> +  @retval EFI_SUCCESS           On success.
> +  @retval EFI_UNSUPPORTED       ECC group not supported.
> +  @retval EFI_INVALID_PARAMETER One or more of the following 
> + conditions is
> TRUE:
> +                                Secret is NULL.
> +                                SecretSize is NULL.
> +                                Public key in EcPointPublic is invalid.
> +  @retval EFI_PROTOCOL_ERROR    On failure.
>  **/
>  EFI_STATUS
>  EFIAPI
> diff --git a/CryptoPkg/Private/Protocol/Crypto.h
> b/CryptoPkg/Private/Protocol/Crypto.h
> index 1b31714d77..1cf5d18cc3 100644
> --- a/CryptoPkg/Private/Protocol/Crypto.h
> +++ b/CryptoPkg/Private/Protocol/Crypto.h
> @@ -3863,7 +3863,6 @@ CONST VOID *
>    @param[out]  BnRes   The result, such that (BnA * BnB) % BnM.
> 
>    @retval EFI_SUCCESS          On success.
> -  @retval EFI_OUT_OF_RESOURCES In case of internal allocation failures.
>    @retval EFI_PROTOCOL_ERROR   Otherwise.
>  **/
>  typedef
> @@ -3935,6 +3934,9 @@ VOID
> 
>    @param[in]   Bn     Big number to set.
>    @param[in]   Val    Value to set.
> +
> +  @retval EFI_SUCCESS          On success.
> +  @retval EFI_PROTOCOL_ERROR   Otherwise.
>  **/
>  typedef
>  EFI_STATUS
> @@ -3970,7 +3972,7 @@ EFI_STATUS
>    using EcGroupFree() function.
> 
>    @param[in]  Group  Identifying number for the ECC group (IANA "Group
> -                     Description" attribute registrty for RFC 2409)
> +                     Description" attribute registry for RFC 2409)
> 
>    @retval EcGroup object  On success
>    @retval NULL            On failure
> @@ -3989,8 +3991,8 @@ VOID *
> 
>    @param[in]  EcGroup    EC group object
>    @param[out] BnPrime    Group prime number
> -  @param[out] BnA        A coofecient
> -  @param[out] BnB        B coofecient
> +  @param[out] BnA        A coefficient
> +  @param[out] BnB        B coefficient
>    @param[in]  BnCtx      BN context
> 
>    @retval EFI_SUCCESS        On success
> @@ -4260,13 +4262,14 @@ EFI_STATUS
>  /**
>    Generate a key using ECDH algorithm. Please note, this function uses
>    pseudo random number generator. The caller must make sure 
> RandomSeed()
> -  funtion was properly called before.
> +  function was properly called before.
> 
>    @param[in]  Group    Identifying number for the ECC group (IANA "Group
> -                       Description" attribute registrty for RFC 2409)
> +                       Description" attribute registry for RFC 2409)
>    @param[out] PKey     Pointer to an object that will hold the ECDH key
> 
>    @retval EFI_SUCCESS        On success
> +  @retval EFI_UNSUPPORTED    ECC group not supported.
>    @retval EFI_PROTOCOL_ERROR On failure  **/  typedef @@ -4294,8 
> +4297,9 @@ VOID
>    @param[in]  PKey     ECDH Key object
>    @param[out] EcPoint  Properly initialized EC Point to hold the 
> public key
> 
> -  @retval EFI_SUCCESS        On success
> -  @retval EFI_PROTOCOL_ERROR On failure
> +  @retval EFI_SUCCESS           On success
> +  @retval EFI_INVALID_PARAMETER EcPoint should be initialized properly.
> +  @retval EFI_PROTOCOL_ERROR    On failure
>  **/
>  typedef
>  EFI_STATUS
> @@ -4309,15 +4313,20 @@ EFI_STATUS
> 
>    @param[in]  PKey           ECDH Key object
>    @param[in]  Group          Identifying number for the ECC group (IANA "Group
> -                             Description" attribute registrty for RFC 2409)
> +                             Description" attribute registry for RFC
> + 2409)
>    @param[in]  EcPointPublic  Peer public key
>    @param[out] SecretSize     On success, holds secret size
>    @param[out] Secret         On success, holds the derived secret
>                               Should be freed by caller using FreePool()
>                               function.
> 
> -  @retval EFI_SUCCESS        On success
> -  @retval EFI_PROTOCOL_ERROR On failure
> +  @retval EFI_SUCCESS           On success.
> +  @retval EFI_UNSUPPORTED       ECC group not supported.
> +  @retval EFI_INVALID_PARAMETER One or more of the following 
> + conditions is
> TRUE:
> +                                Secret is NULL.
> +                                SecretSize is NULL.
> +                                Public key in EcPointPublic is invalid.
> +  @retval EFI_PROTOCOL_ERROR    On failure.
>  **/
>  typedef
>  EFI_STATUS
> --
> 2.31.1.windows.1


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2022-07-15  3:31 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-07-15  2:07 [staging/crypto-new-api PATCH] CryptoPkg: Fix issues from crypto code review yi1 li
2022-07-15  2:13 ` Heng Luo
2022-07-15  3:31   ` Tan, Ming

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox