Laszlo,
> -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Tuesday, April 30, 2019 2:31 AM > To: devel@edk2.groups.io; Lu, XiaoyuX <xiaoyux...@intel.com> > Cc: Wang, Jian J <jian.j.w...@intel.com>; Ye, Ting <ting...@intel.com> > Subject: Re: [edk2-devel] [PATCH 3/3] CryptoPkg/BaseCryptLib: updata > HMAC_ctx size > > On 04/29/19 10:15, Xiaoyu lu wrote: > > From: Xiaoyu Lu <xiaoyux...@intel.com> > > > > Openssl internally redefines the size of HMAC_CTX, > > but there is no external definition. > > So add an additional nubmer. > > > > Cc: Jian J Wang <jian.j.w...@intel.com> > > Cc: Ting Ye <ting...@intel.com> > > Signed-off-by: Xiaoyu Lu <xiaoyux...@intel.com> > > --- > > CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c | 11 ++++++++++- > > CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c | 12 ++++++++++-- > > CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c | 12 ++++++++++- > - > > 3 files changed, 30 insertions(+), 5 deletions(-) > > > > diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c > b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c > > index 3134806..3ffb8e2 100644 > > --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c > > +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c > > @@ -9,8 +9,17 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > #include "InternalCryptLib.h" > > #include <openssl/hmac.h> > > > > +// > > +// NOTE: HMAC_MAX_MD_CBLOCK is deprecated. > > +// #define HMAC_MAX_MD_CBLOCK 128 > > +// Openssl redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h > > +// #define HMAC_MAX_MD_CBLOCK_SIZE 144 > > +// But we need to compatible with previous API. > > +// So fix it with correct size 144-128 = 16. > > +// > > #define HMAC_MD5_CTX_SIZE sizeof(void *) * 4 + sizeof(unsigned int) + \ > > - sizeof(unsigned char) * HMAC_MAX_MD_CBLOCK > > + sizeof(unsigned char) * (HMAC_MAX_MD_CBLOCK + > > 16) > > + > > > > /** > > Retrieves the size, in bytes, of the context buffer required for HMAC-MD5 > operations. > > diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c > b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c > > index bbe3df4..e59602e 100644 > > --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c > > +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c > > @@ -9,8 +9,16 @@ 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: HMAC_MAX_MD_CBLOCK is deprecated. > > +// #define HMAC_MAX_MD_CBLOCK 128 > > +// Openssl redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h > > +// #define HMAC_MAX_MD_CBLOCK_SIZE 144 > > +// But we need to compatible with previous API. > > +// So fix it with correct size 144-128 = 16. > > +// > > +#define HMAC_SHA1_CTX_SIZE sizeof(void *) * 4 + sizeof(unsigned int) + \ > > + sizeof(unsigned char) * (HMAC_MAX_MD_CBLOCK + > > 16) > > > > /** > > 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..8d0570b 100644 > > --- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c > > +++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c > > @@ -9,8 +9,16 @@ 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: HMAC_MAX_MD_CBLOCK is deprecated. > > +// #define HMAC_MAX_MD_CBLOCK 128 > > +// Openssl redefines the size of HMAC_CTX at crypto/hmac/hmac_lcl.h > > +// #define HMAC_MAX_MD_CBLOCK_SIZE 144 > > +// But we need to compatible with previous API. > > +// So fix it with correct size 144-128 = 16. > > +// > > +#define HMAC_SHA256_CTX_SIZE sizeof(void *) * 4 + sizeof(unsigned int) + > \ > > + sizeof(unsigned char) * (HMAC_MAX_MD_CBLOCK + > > 16) > > > > /** > > Retrieves the size, in bytes, of the context buffer required for > > HMAC-SHA256 > operations. > > > > I believe I disagree with this patch. > > The issue is well described here: > > https://github.com/openssl/openssl/pull/4338 > > And the real solution, for edk2, is apparently described in edk2 > already: > > [CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c]: > > > /** > > Retrieves the size, in bytes, of the context buffer required for HMAC-MD5 > operations. > > (NOTE: This API is deprecated. > > Use HmacMd5New() / HmacMd5Free() for HMAC-MD5 Context > operations.) > > > > @return The size, in bytes, of the context buffer required for HMAC-MD5 > operations. > > > > **/ > > UINTN > > EFIAPI > > HmacMd5GetContextSize ( > > VOID > > ) > > { > > // > > // Retrieves the OpenSSL HMAC-MD5 Context Size > > // NOTE: HMAC_CTX object was made opaque in openssl-1.1.x, here we just > use the > > // fixed size as a workaround to make this API work for > > compatibility. > > // We should retire HmacMd5GetContextSize() in future, and use > HmacMd5New() > > // and HmacMd5Free() for context allocation and release. > > // > > return (UINTN) HMAC_MD5_CTX_SIZE; > > } > > [CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c]: > > > /** > > 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.) > > > > @return The size, in bytes, of the context buffer required for HMAC-SHA1 > operations. > > > > **/ > > UINTN > > EFIAPI > > HmacSha1GetContextSize ( > > VOID > > ) > > { > > // > > // Retrieves the OpenSSL HMAC-SHA1 Context Size > > // NOTE: HMAC_CTX object was made opaque in openssl-1.1.x, here we just > use the > > // fixed size as a workaround to make this API work for > > compatibility. > > // We should retire HmacSha15GetContextSize() in future, and use > HmacSha1New() > > // and HmacSha1Free() for context allocation and release. > > // > > return (UINTN) HMAC_SHA1_CTX_SIZE; > > } > > [CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha256.c]: > > > /** > > 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.) > > > > @return The size, in bytes, of the context buffer required for > > HMAC-SHA256 > operations. > > > > **/ > > UINTN > > EFIAPI > > HmacSha256GetContextSize ( > > VOID > > ) > > { > > // > > // Retrieves the OpenSSL HMAC-SHA256 Context Size > > // NOTE: HMAC_CTX object was made opaque in openssl-1.1.x, here we just > use the > > // fixed size as a workaround to make this API work for > > compatibility. > > // We should retire HmacSha256GetContextSize() in future, and use > HmacSha256New() > > // and HmacSha256Free() for context allocation and release. > > // > > return (UINTN)HMAC_SHA256_CTX_SIZE; > > } > > Is there any reason why we can't clean up this code first, in a separate > patch series, before moving to OpenSSL 1.1.1? > > Because, the "NOTE"s are more than two years old now... They come from > commit 4c270243995a ("CryptoPkg: Update HMAC Wrapper with opaque > HMAC_CTX object.", 2017-03-29). > > This has been our technical debt ever since, violating the OpenSSL > effort to hide implementation details. I think we should eliminate this > debt, rather than make it worse. > > (Obviously this is just my opinion, and I'm not a CryptoPkg > co-maintainer.) > I agree that we need to clean up the deprecated macros and api. The problem is that XxxGetContextSize() are public APIs and there're dozens of modules which still call them. This can't be done in short time. What about we file a BZ to track this issue and target to next stable tag? Regards, Jian > Thanks > Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#39803): https://edk2.groups.io/g/devel/message/39803 Mute This Topic: https://groups.io/mt/31381055/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
