[edk2-devel] [PATCH v2 6/6] CryptoPkg/BaseCryptLib: Make HMAC_CTX size backward compatible
Wang, Jian J
jian.j.wang at intel.com
Thu May 9 14:20:54 UTC 2019
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
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 (#40364): https://edk2.groups.io/g/devel/message/40364
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]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list