From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by mx.groups.io with SMTP id smtpd.web10.5937.1585285155429192234 for ; Thu, 26 Mar 2020 21:59:15 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.100, mailfrom: jiewen.yao@intel.com) IronPort-SDR: RQBm60Ln1V5NesHblCrj/kcpOF73L3PFYMOEI+1/4hUGhKvbvAFwdsPKhQUR7S03lGuRexFmA+ KASTEnw+K2aw== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 26 Mar 2020 21:59:14 -0700 IronPort-SDR: kFbFtlps6YK5SAlj3d05MCCCjl3MGxyLSvBrXxgqQFWxr8phW+TrXVGhCt6VZl17baeujnswop nyGXiva/xCpA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.72,311,1580803200"; d="scan'208";a="240869822" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga008.fm.intel.com with ESMTP; 26 Mar 2020 21:59:14 -0700 Received: from fmsmsx102.amr.corp.intel.com (10.18.124.200) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 26 Mar 2020 21:59:14 -0700 Received: from shsmsx106.ccr.corp.intel.com (10.239.4.159) by FMSMSX102.amr.corp.intel.com (10.18.124.200) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 26 Mar 2020 21:59:13 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.50]) by SHSMSX106.ccr.corp.intel.com ([169.254.10.86]) with mapi id 14.03.0439.000; Fri, 27 Mar 2020 12:59:12 +0800 From: "Yao, Jiewen" To: "Fu, Siyuan" , "devel@edk2.groups.io" , "Gao, Zhichao" CC: "Wang, Jian J" , "Lu, XiaoyuX" , Maciej Rabeda , "Wu, Jiaxin" Subject: Re: [edk2-devel] [PATCH 0/8] CryptoPkg: Retire the deprecate function Thread-Topic: [edk2-devel] [PATCH 0/8] CryptoPkg: Retire the deprecate function Thread-Index: AQHWA9r3vfbc0WLdPUSI70pQ90E/bahbNjmAgACIDrD//31yAIAAhwdg Date: Fri, 27 Mar 2020 04:59:10 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503F9A08EF@shsmsx102.ccr.corp.intel.com> References: <20200327015629.2588-1-zhichao.gao@intel.com> <74D8A39837DF1E4DA445A8C0B3885C503F9A0490@shsmsx102.ccr.corp.intel.com> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: jiewen.yao@intel.com Content-Language: en-US Content-Type: text/plain; charset="iso-2022-jp" Content-Transfer-Encoding: quoted-printable Thanks Siyun. I think probably we need discuss this more. 1) About private v.s. public. The benefit for private include is to isolate external interface and inter= nal interface. A package can keep updating its private interface without impact any other= packages. However, in this case, a private interface update will bring binary compat= ibility issue with other package. I am not sure it is acceptable or not. Mike Do you have any comment? Is that the design goal of private interface - ju= st keep source code compatibility, but not binary compatiblity? 2) About the protocol itself. One concern I have is that we *hardcode* the algorithm in protocol. We keeps adding new algorithm and removing old one. That means this protoc= ol interface is unstable. Today, we have defined SHA2 set, and deprecating SHA1 and older one. Tomor= row we may need add SHA3 set. Today, we have RSAPKCS1_15. Tomorrow we will have RSAPSS. Some other new set of algorithms might be added later, such as AEAD, GMAC. For a protocol definition, I think we need *abstract the action*, but not = *algorithm*. One good example is the UEFI HASH2 Protocol. https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Protocol/Hash= 2.h It just tells you do the hash. You may add new algorithm GUID. Another good example is inside of openssl. Now it is using EVP style ciphe= r algo. For example, https://www.openssl.org/docs/man1.1.1/man3/EVP_EncryptInit_ex= .html https://www.openssl.org/docs/man1.1.0/man3/EVP_CIPHER_CTX_ctrl.html The cipher itself is input as parameter. The benefit is that, if we want to deprecate an algorithm, the interface c= an be unchanged. Just the internal implementation can be changed. The current PCD mechanism can still be applied to internal implementation. Can we get a chance to revisit/redesign the protocol API, when we move to = public include? Thank you Yao Jiewen > -----Original Message----- > From: Fu, Siyuan > Sent: Friday, March 27, 2020 11:07 AM > To: Yao, Jiewen ; devel@edk2.groups.io; Gao, Zhich= ao > > Cc: Wang, Jian J ; Lu, XiaoyuX ; > Maciej Rabeda ; Wu, Jiaxin > > Subject: RE: [edk2-devel] [PATCH 0/8] CryptoPkg: Retire the deprecate fu= nction >=20 > Hi, Jiewen >=20 > Although the protocol is private, a corresponding BaseCryptoLib instance= is > not private, like PeiCryptLib.inf, RuntimeCryptLib, etc. These library i= nstances > will be static linked to the consumer driver, for example an iSCSI netwo= rk driver. > So actually it's not a "private" change inside CryptoPkg. >=20 > The goal to provide a driver version of crypto service is to support mod= ulization > FW update, which means the crypto driver may NOT be updated together wit= h > its consumer. A platform may choose to update the crypto service driver = to a > new version with this patch, then all the BaseCryptoLib consumers will b= e > messed. >=20 > Best Regards > Siyuan >=20 > > -----Original Message----- > > From: Yao, Jiewen > > Sent: 2020=1B$BG/=1B(B3=1B$B7n=1B(B27=1B$BF|=1B(B 10:58 > > To: devel@edk2.groups.io; Fu, Siyuan ; Gao, Zhich= ao > > > > Cc: Wang, Jian J ; Lu, XiaoyuX > ; > > Maciej Rabeda ; Wu, Jiaxin > > > > Subject: RE: [edk2-devel] [PATCH 0/8] CryptoPkg: Retire the deprecate > function > > > > EDKII_CRYPTO_PROTOCOL is *private*. > > > https://github.com/tianocore/edk2/blob/master/CryptoPkg/Private/Protocol= /C > > rypto.h > > > > Why we cannot change? > > > > > > > > > -----Original Message----- > > > From: devel@edk2.groups.io On Behalf Of Siyua= n, > Fu > > > Sent: Friday, March 27, 2020 10:47 AM > > > To: Gao, Zhichao ; devel@edk2.groups.io > > > Cc: Wang, Jian J ; Lu, XiaoyuX > > ; > > > Maciej Rabeda ; Wu, Jiaxin > > > > > > Subject: Re: [edk2-devel] [PATCH 0/8] CryptoPkg: Retire the deprecat= e > > function > > > > > > Hi, Zhichao > > > > > > We should never move/delete a member field of a previous defined pro= tocol > > > Interface. Instead, these protocol APIs shall be kept and return an = error code > > > If the function is retired. Otherwise the consumer driver may call i= nto an > > > Incorrect function if it's build with different codebase/PCD setting= s with the > > > Crypto PEI/DXE/SMM driver. > > > This comment applies to all the EDKII_CRYPTO_PROTOCOL related change= s > in > > > your patch set. > > > > > > Best Regards > > > Siyuan > > > > > > > -----Original Message----- > > > > From: Gao, Zhichao > > > > Sent: 2020=1B$BG/=1B(B3=1B$B7n=1B(B27=1B$BF|=1B(B 9:56 > > > > To: devel@edk2.groups.io > > > > Cc: Wang, Jian J ; Lu, XiaoyuX > > > ; > > > > Maciej Rabeda ; Wu, Jiaxin > > > > ; Fu, Siyuan > > > > Subject: [PATCH 0/8] CryptoPkg: Retire the deprecate function > > > > > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1682 > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1898 > > > > > > > > MD4, AR4, Tdes, Aes Ecb mode, MD5 and SHA1 is not secure any longe= r. > > > > They are all deprecated. Edk2 would not support them any longer. > > > > So remove them. > > > > But uefi spec want to keep MD5 and SHA1 for backwards compatibilit= y. > > > > So add two pcds to control the MD5 and SHA1 enablement. Set the pc= ds > > > > default value to false to indicate they are deprecated. > > > > > > > > NetWorkPkg's iSCSI driver would consume the MD5 function, so chang= e > > > > the md5 pcd to TURE when iSCSI is enabled. > > > > > > > > Cc: Jian J Wang > > > > Cc: Xiaoyu Lu > > > > Cc: Maciej Rabeda > > > > Cc: Jiaxin Wu > > > > Cc: Siyuan Fu > > > > Signed-off-by: Zhichao Gao > > > > > > > > Zhichao Gao (8): > > > > CryptoPkg/BaseCrpytLib: Retire MD4 algorithm > > > > CryptoPkg/BaseCryptLib: Retire ARC4 algorithm > > > > CryptoPkg/BaseCryptLib: Retire the Tdes algorithm > > > > CryptoPkg/BaseCryptLib: Retire Aes Ecb mode algorithm > > > > CryptoPkg/dec: Add pcds to avoid building the deprecated functio= n > > > > NetWorkPkg/Pcd.inc: Enable the MD5 for iSCSI > > > > Crypto/BaseCryptLib: Using pcd to control MD5 enablement > > > > CryptoPkg/BaseCryptLib: Use Pcd to control the SHA1 enablement > > > > > > > > CryptoPkg/CryptoPkg.dec | 11 + > > > > CryptoPkg/CryptoPkg.uni | 11 + > > > > CryptoPkg/Driver/Crypto.c | 634 +------------= ----- > > > > CryptoPkg/Include/Library/BaseCryptLib.h | 548 -------------= -- > > > > .../Library/BaseCryptLib/BaseCryptLib.inf | 9 +- > > > > .../Library/BaseCryptLib/Cipher/CryptAes.c | 114 ---- > > > > .../BaseCryptLib/Cipher/CryptAesNull.c | 52 -- > > > > .../Library/BaseCryptLib/Cipher/CryptArc4.c | 205 ------ > > > > .../BaseCryptLib/Cipher/CryptArc4Null.c | 124 ---- > > > > .../Library/BaseCryptLib/Cipher/CryptTdes.c | 364 ---------- > > > > .../BaseCryptLib/Cipher/CryptTdesNull.c | 160 ----- > > > > .../Library/BaseCryptLib/Hash/CryptMd4.c | 223 ------ > > > > .../Library/BaseCryptLib/Hash/CryptMd4Null.c | 143 ---- > > > > .../Library/BaseCryptLib/Hash/CryptMd5.c | 5 +- > > > > .../Library/BaseCryptLib/Hmac/CryptHmacMd5.c | 3 + > > > > .../BaseCryptLib/Hmac/CryptHmacMd5Null.c | 3 + > > > > .../Library/BaseCryptLib/Hmac/CryptHmacSha1.c | 3 + > > > > .../BaseCryptLib/Hmac/CryptHmacSha1Null.c | 3 + > > > > .../Library/BaseCryptLib/PeiCryptLib.inf | 13 +- > > > > .../BaseCryptLib/Pk/CryptPkcs5Pbkdf2.c | 3 + > > > > .../Library/BaseCryptLib/Pk/CryptRsaBasic.c | 5 + > > > > .../Library/BaseCryptLib/Pk/CryptRsaExt.c | 5 + > > > > .../Library/BaseCryptLib/RuntimeCryptLib.inf | 13 +- > > > > .../Library/BaseCryptLib/SmmCryptLib.inf | 13 +- > > > > .../BaseCryptLibNull/BaseCryptLibNull.inf | 3 - > > > > .../BaseCryptLibNull/Cipher/CryptAesNull.c | 54 +- > > > > .../BaseCryptLibNull/Cipher/CryptArc4Null.c | 124 ---- > > > > .../BaseCryptLibNull/Cipher/CryptTdesNull.c | 160 ----- > > > > .../BaseCryptLibNull/Hash/CryptMd4Null.c | 143 ---- > > > > .../BaseCryptLibNull/Hash/CryptMd5Null.c | 3 + > > > > .../BaseCryptLibNull/Hmac/CryptHmacMd5Null.c | 3 + > > > > .../BaseCryptLibNull/Hmac/CryptHmacSha1Null.c | 4 +- > > > > .../BaseCryptLibOnProtocolPpi/CryptLib.c | 604 +------------= ---- > > > > .../Library/BaseHashApiLib/BaseHashApiLib.c | 12 + > > > > .../Library/BaseHashApiLib/BaseHashApiLib.inf | 1 + > > > > CryptoPkg/Private/Protocol/Crypto.h | 583 +------------= --- > > > > NetworkPkg/NetworkPcds.dsc.inc | 5 +- > > > > 37 files changed, 145 insertions(+), 4221 deletions(-) > > > > delete mode 100644 CryptoPkg/Library/BaseCryptLib/Cipher/CryptArc= 4.c > > > > delete mode 100644 > > CryptoPkg/Library/BaseCryptLib/Cipher/CryptArc4Null.c > > > > delete mode 100644 CryptoPkg/Library/BaseCryptLib/Cipher/CryptTde= s.c > > > > delete mode 100644 > > CryptoPkg/Library/BaseCryptLib/Cipher/CryptTdesNull.c > > > > delete mode 100644 CryptoPkg/Library/BaseCryptLib/Hash/CryptMd4.c > > > > delete mode 100644 > CryptoPkg/Library/BaseCryptLib/Hash/CryptMd4Null.c > > > > delete mode 100644 > > > > CryptoPkg/Library/BaseCryptLibNull/Cipher/CryptArc4Null.c > > > > delete mode 100644 > > > > CryptoPkg/Library/BaseCryptLibNull/Cipher/CryptTdesNull.c > > > > delete mode 100644 > > > CryptoPkg/Library/BaseCryptLibNull/Hash/CryptMd4Null.c > > > > > > > > -- > > > > 2.21.0.windows.1 > > > > > > > > >=20