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 07:01:58 -0700 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C6AD288E56; Thu, 9 May 2019 14:01:46 +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 8C4B15E7D2; Thu, 9 May 2019 14:01:44 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 6/6] CryptoPkg/BaseCryptLib: Make HMAC_CTX size backward compatible To: devel@edk2.groups.io, xiaoyux.lu@intel.com Cc: Jian J Wang , Ting Ye 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: Date: Thu, 9 May 2019 16:01:43 +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: <1557379429-7527-6-git-send-email-xiaoyux.lu@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Thu, 09 May 2019 14:01:47 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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? > > 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. >