Re: [PATCH v3 1/2] CryptoPkg/BaseCryptLib: replace HmacXxxInit API with HmacXxxSetKey

["Laszlo Ersek" <lersek@redhat.com>]

Hi Jian,

On 01/16/20 07:10, Jian J Wang wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1792
> 
> HmacXxxInit() is supposed to be initialize user supplied buffer as HMAC
> context, as well as user supplied key. Currently it has no real use cases.
> 
> Due to BZ1792, the user has no way to get correct size of context buffer
> after it's fixed, and then cannot make use of HmacXxxInit to initialize
> it.
> 
> So it's decided to replace it with HmacXxxSetKey to keep the functionality
> of supplying a key to HMAC, but drop all other initialization works. The
> user can still get HMAC context via HmacXxxNew interface, which hides the
> details about the context.
> 
> Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  CryptoPkg/Include/Library/BaseCryptLib.h      | 94 ++++++-------------
>  .../Library/BaseCryptLib/Hmac/CryptHmacMd5.c  | 27 ++----
>  .../BaseCryptLib/Hmac/CryptHmacMd5Null.c      |  8 +-
>  .../Library/BaseCryptLib/Hmac/CryptHmacSha1.c | 27 ++----
>  .../BaseCryptLib/Hmac/CryptHmacSha1Null.c     |  8 +-
>  .../BaseCryptLib/Hmac/CryptHmacSha256.c       | 27 ++----
>  .../BaseCryptLib/Hmac/CryptHmacSha256Null.c   |  8 +-
>  .../BaseCryptLibNull/Hmac/CryptHmacMd5Null.c  |  8 +-
>  .../BaseCryptLibNull/Hmac/CryptHmacSha1Null.c |  8 +-
>  .../Hmac/CryptHmacSha256Null.c                |  8 +-
>  10 files changed, 84 insertions(+), 139 deletions(-)
> 
> diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h b/CryptoPkg/Include/Library/BaseCryptLib.h
> index 8fe303a0b3..c5803ea673 100644
> --- a/CryptoPkg/Include/Library/BaseCryptLib.h
> +++ b/CryptoPkg/Include/Library/BaseCryptLib.h

[...]

> @@ -1175,23 +1175,6 @@ HmacMd5Final (
>    OUT     UINT8  *HmacValue
>    );
>  
> -/**
> -  Retrieves the size, in bytes, of the context buffer required for HMAC-SHA1 operations.
> -  (NOTE: This API is deprecated.
> -         Use HmacSha1New() / HmacSha1Free() for HMAC-SHA1 Context operations.)
> -
> -  If this interface is not supported, then return zero.
> -
> -  @return  The size, in bytes, of the context buffer required for HMAC-SHA1 operations.
> -  @retval  0   This interface is not supported.
> -
> -**/
> -UINTN
> -EFIAPI
> -HmacSha1GetContextSize (
> -  VOID
> -  );
> -
>  /**
>    Allocates and initializes one HMAC_CTX context for subsequent HMAC-SHA1 use.
>  

(1) This hunk belongs to patch#2. In other words,
HmacSha1GetContextSize() should not be removed from the lib class header
in patch#1.

[...]

> @@ -1325,23 +1308,6 @@ HmacSha1Final (
>    OUT     UINT8  *HmacValue
>    );
>  
> -/**
> -  Retrieves the size, in bytes, of the context buffer required for HMAC-SHA256 operations.
> -  (NOTE: This API is deprecated.
> -         Use HmacSha256New() / HmacSha256Free() for HMAC-SHA256 Context operations.)
> -
> -  If this interface is not supported, then return zero.
> -
> -  @return  The size, in bytes, of the context buffer required for HMAC-SHA256 operations.
> -  @retval  0   This interface is not supported.
> -
> -**/
> -UINTN
> -EFIAPI
> -HmacSha256GetContextSize (
> -  VOID
> -  );
> -
>  /**
>    Allocates and initializes one HMAC_CTX context for subsequent HMAC-SHA256 use.
>  

(2) This hunk belongs to patch#2. In other words,
HmacSha256GetContextSize() should not be removed from the lib class
header in patch#1.


With the above two points addressed:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo