Re: [edk2-devel] [PATCH v2 6/6] CryptoPkg/BaseCryptLib: Make HMAC_CTX size backward compatible

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

On 05/09/19 07:23, Xiaoyu lu wrote:
> From: Xiaoyu Lu <xiaoyux.lu@intel.com>
> 
> Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=1089
> 
> OpenSSL internally redefines the size of HMAC_CTX at
> crypto/hmac/hmac_lcl.h(OpenSSL commit e0810e35).

In my review at <https://edk2.groups.io/g/devel/message/39837>, I *also*
asked for referencing <https://github.com/openssl/openssl/pull/4338>,
because that PULL request discussion provides the rationale for the
change. Well, whatever.

> We should not use it directly and should remove relevant
> functions(Hmac*GetContextSize).

In the same review, in bullet (2), I asked that the v2 commit message
please reference the *new* TianoCore BZ -- the one that tracks the
HMAC_xxx_CTX_SIZE / Hmac*GetContextSize() removal.

Jiaxin said in <https://edk2.groups.io/g/devel/message/39840> that he'd
file such a BZ. Do we have that BZ now? Can we please mention it in the
commit message?

> 
> But for compatiblility, still updated HMAC_*_CTX_SIZE.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Ting Ye <ting.ye@intel.com>
> Signed-off-by: Xiaoyu Lu <xiaoyux.lu@intel.com>
> ---
>  CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c    | 8 ++++++--
>  CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c   | 9 +++++++--
>  CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c | 8 ++++++--
>  3 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c
> index 3134806..3a90e03 100644
> --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c
> +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c
> @@ -9,8 +9,12 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  #include "InternalCryptLib.h"
>  #include <openssl/hmac.h>
>  
> -#define HMAC_MD5_CTX_SIZE    sizeof(void *) * 4 + sizeof(unsigned int) + \
> -                             sizeof(unsigned char) * HMAC_MAX_MD_CBLOCK
> +/**
> + NOTE: OpenSSL redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h
> +       #define HMAC_MAX_MD_CBLOCK_SIZE     144
> +**/
> +#define HMAC_MD5_CTX_SIZE    (sizeof(void *) * 4 + sizeof(unsigned int) + \
> +                             sizeof(unsigned char) * 144)
>  
>  /**
>    Retrieves the size, in bytes, of the context buffer required for HMAC-MD5 operations.

In my review linked above, I asked for the comment to be removed. I
guess you disagreed, ultimately.

I agree that the new comment is acceptable. However, it does not conform
to the edk2 coding style. We should use the // format, for comments like
these.

Summary:
- please file the new BZ, and mention it too, in the commit message
- please clean up the comment style.

With both of those fixed, you can add

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

Thanks
Laszlo

> diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c
> index bbe3df4..a8e8d44 100644
> --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c
> +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c
> @@ -9,8 +9,13 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  #include "InternalCryptLib.h"
>  #include <openssl/hmac.h>
>  
> -#define HMAC_SHA1_CTX_SIZE   sizeof(void *) * 4 + sizeof(unsigned int) + \
> -                             sizeof(unsigned char) * HMAC_MAX_MD_CBLOCK
> +/**
> + NOTE: OpenSSL redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h
> +       #define HMAC_MAX_MD_CBLOCK_SIZE     144
> +
> +**/
> +#define  HMAC_SHA1_CTX_SIZE   (sizeof(void *) * 4 + sizeof(unsigned int) + \
> +                             sizeof(unsigned char) * 144)
>  
>  /**
>    Retrieves the size, in bytes, of the context buffer required for HMAC-SHA1 operations.
> diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c
> index ac9084f..fec60b1 100644
> --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c
> +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c
> @@ -9,8 +9,12 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  #include "InternalCryptLib.h"
>  #include <openssl/hmac.h>
>  
> -#define HMAC_SHA256_CTX_SIZE   sizeof(void *) * 4 + sizeof(unsigned int) + \
> -                               sizeof(unsigned char) * HMAC_MAX_MD_CBLOCK
> +/**
> + NOTE: OpenSSL redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h
> +       #define HMAC_MAX_MD_CBLOCK_SIZE     144
> +**/
> +#define HMAC_SHA256_CTX_SIZE    (sizeof(void *) * 4 + sizeof(unsigned int) + \
> +                             sizeof(unsigned char) * 144)
>  
>  /**
>    Retrieves the size, in bytes, of the context buffer required for HMAC-SHA256 operations.
>