From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Thu, 09 May 2019 14:34:17 -0700 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8AC51307B963; Thu, 9 May 2019 21:34:16 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-234.rdu2.redhat.com [10.10.120.234]) by smtp.corp.redhat.com (Postfix) with ESMTP id 37BFC5C296; Thu, 9 May 2019 21:34:15 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 6/6] CryptoPkg/BaseCryptLib: Make HMAC_CTX size backward compatible To: "Wang, Jian J" , "devel@edk2.groups.io" , "Lu, XiaoyuX" Cc: "Ye, Ting" References: <1557379429-7527-1-git-send-email-xiaoyux.lu@intel.com> <1557379429-7527-6-git-send-email-xiaoyux.lu@intel.com> From: "Laszlo Ersek" Message-ID: <92f50ac5-20b5-abac-f0a5-fbe1beed549b@redhat.com> Date: Thu, 9 May 2019 23:34:14 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.47]); Thu, 09 May 2019 21:34:16 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 05/09/19 16:20, Wang, Jian J wrote: > Laszlo, > > >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Thursday, May 09, 2019 10:02 PM >> To: devel@edk2.groups.io; Lu, XiaoyuX >> Cc: Wang, Jian J ; Ye, Ting >> 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 >>> >>> 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 , I *also* >> asked for referencing , >> 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 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 >>> Cc: Ting Ye >>> Signed-off-by: Xiaoyu Lu >>> --- >>> 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 >>> >>> -#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 >> >> 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 >>> >>> -#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 >>> >>> -#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. >>> >