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

[Laszlo Ersek]

On 05/09/19 16:20, Wang, Jian J wrote:
> Laszlo,
> 
> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek at redhat.com]
>> Sent: Thursday, May 09, 2019 10:02 PM
>> To: devel at edk2.groups.io; Lu, XiaoyuX <xiaoyux.lu at intel.com>
>> Cc: Wang, Jian J <jian.j.wang at intel.com>; Ye, Ting <ting.ye at intel.com>
>> Subject: Re: [edk2-devel] [PATCH v2 6/6] CryptoPkg/BaseCryptLib: Make
>> HMAC_CTX size backward compatible
>>
>> On 05/09/19 07:23, Xiaoyu lu wrote:
>>> From: Xiaoyu Lu <xiaoyux.lu at 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?
>>
> 
> My apology. I forgot to file one. Just entered one 
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=1792

Much appreciated!
Laszlo

> 
> Regards,
> Jian
> 
>>>
>>> But for compatiblility, still updated HMAC_*_CTX_SIZE.
>>>
>>> Cc: Jian J Wang <jian.j.wang at intel.com>
>>> Cc: Ting Ye <ting.ye at intel.com>
>>> Signed-off-by: Xiaoyu Lu <xiaoyux.lu at 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 at 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.
>>>
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#40378): https://edk2.groups.io/g/devel/message/40378
Mute This Topic: https://groups.io/mt/31552213/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-