[edk2-devel] [PATCH 0/8] CryptoPkg: Retire the deprecate function
Gao, Zhichao
zhichao.gao at intel.com
Tue Apr 14 04:36:54 UTC 2020
Hi All,
Thanks for the comments.
Here is the summary and the change I plan to do. Anything incorrect or lacking, please help to point out:
1. do not use the #if in c code: Using the pcd to assert and return error status in the deprecated function.
2. remove the hmac_sha1 and hmac_md5 as there is no usage in the edk2 scope(trunk + platform + non-osi)
3. don't change the structure controlled by structure PCD:
It is implemented for the modularization update and suggested to not remove any function to keep the offset value of the stucture.
So I would change make sure all the deprecated function to be disabled in all profile config but keep md5 and sha1 enable because it is required for iSCSI, TPM1.2 and other functions.
After all the changes for the deprecated function done, update the version for next build.
4. about the sha1, it should process in 2 phase:
(2) send patch set1 to the related platform that required the sha1 to enable the sha1 with the structure pcd PcdCryptoServiceFamilyEnable
(3) send patch set2 to use the pcd to disable the sha1 in default
*Here may be some issues:
1. modularization bin driver would always support md5 and sha1 unless the pcd value is enable or disable. That means protocol solution would act different with the lib source solution.
2. unable the find the deprecated usage of md5 and sha1 during build time.
Thanks,
Zhichao
> -----Original Message-----
> From: Yao, Jiewen <jiewen.yao at intel.com>
> Sent: Tuesday, March 31, 2020 8:35 AM
> To: Kinney, Michael D <michael.d.kinney at intel.com>; Fu, Siyuan
> <siyuan.fu at intel.com>; devel at edk2.groups.io; Gao, Zhichao
> <zhichao.gao at intel.com>; Matthew Carlson <macarl at microsoft.com>; Sean
> Brogan <sean.brogan at microsoft.com>
> Cc: Wang, Jian J <jian.j.wang at intel.com>; Lu, XiaoyuX <xiaoyux.lu at intel.com>;
> Maciej Rabeda <maciej.rabeda at linux.intel.com>; Wu, Jiaxin
> <jiaxin.wu at intel.com>
> Subject: RE: [edk2-devel] [PATCH 0/8] CryptoPkg: Retire the deprecate function
>
> Hi Mike
> "But what we really want to know is if another library or module us using a
> deprecated lib function."
>
> The protocol is internal API, the library is external API.
>
> What we are discussing now is how to handle internal API.
>
> For external API, this patch removed the deprecated lib API and the build will be
> broken.
> It is already satisfied...
>
> Thank You
> Yao Jiewen
>
>
> > -----Original Message-----
> > From: Kinney, Michael D <michael.d.kinney at intel.com>
> > Sent: Tuesday, March 31, 2020 1:31 AM
> > To: Fu, Siyuan <siyuan.fu at intel.com>; Yao, Jiewen
> > <jiewen.yao at intel.com>; devel at edk2.groups.io; Gao, Zhichao
> > <zhichao.gao at intel.com>; Kinney, Michael D
> > <michael.d.kinney at intel.com>; Matthew Carlson <macarl at microsoft.com>;
> > Sean Brogan <sean.brogan at microsoft.com>
> > Cc: Wang, Jian J <jian.j.wang at intel.com>; Lu, XiaoyuX
> > <xiaoyux.lu at intel.com>; Maciej Rabeda <maciej.rabeda at linux.intel.com>;
> > Wu, Jiaxin <jiaxin.wu at intel.com>
> > Subject: RE: [edk2-devel] [PATCH 0/8] CryptoPkg: Retire the deprecate
> > function
> >
> > Hi,
> >
> > I would prefer we only extend the protocol interface and never remove
> > any fields from the Protocol/PPI structures so no offsets are ever
> > changed.
> >
> > The main remaining issue is how a platform developer knows if they are
> > using a deprecated interface. The best method is to know at build
> > time with a compiler failure and the other option is to know at
> > runtime with ASSERT()/ REPORT_STATUS_CODE(). If the field in the
> > protocol structure is renamed, then the library wrapper around the
> > protocol will not build. But what we really want to know is if
> > another library or module us using a deprecated lib function.
> >
> > Laszlo had a suggestion in the review of adding modules to the
> > CryptoPkg for the binary versions of these modules to provide
> > libraries that matched the enabled functions in the binaries so a
> > platform build would fail at compile time. There is very little
> > difference between disabling a service and permanently deprecating a
> > services from a platform detection perspective. Perhaps we should
> > explore Laszlo's ideas more to see if we can address both use cases
> > and always maintain the Protocol/PPI structures in a backwards
> > compatible manner and always keep the same GUID and only increase the
> > Version value when the Protocol/PPI structure is extended.
> >
> > Thanks,
> >
> > Mike
> >
> > > -----Original Message-----
> > > From: Fu, Siyuan <siyuan.fu at intel.com>
> > > Sent: Sunday, March 29, 2020 8:05 PM
> > > To: Yao, Jiewen <jiewen.yao at intel.com>; Kinney, Michael D
> > > <michael.d.kinney at intel.com>; devel at edk2.groups.io; Gao, Zhichao
> > > <zhichao.gao at intel.com>
> > > Cc: Wang, Jian J <jian.j.wang at intel.com>; Lu, XiaoyuX
> > > <xiaoyux.lu at intel.com>; Maciej Rabeda
> > > <maciej.rabeda at linux.intel.com>; Wu, Jiaxin <jiaxin.wu at intel.com>
> > > Subject: RE: [edk2-devel] [PATCH 0/8] CryptoPkg: Retire the
> > > deprecate function
> > >
> > > Jiewen,
> > >
> > > Same as you. I prefer update version (#1) for adding API, and change
> > > protocol GUID (#2)for deprecate unsecure API.
> > >
> > > Best Regards
> > > Siyuan
> > >
> > > > -----Original Message-----
> > > > From: Yao, Jiewen <jiewen.yao at intel.com>
> > > > Sent: 2020年3月30日 10:47
> > > > To: Fu, Siyuan <siyuan.fu at intel.com>; Kinney, Michael D
> > > > <michael.d.kinney at intel.com>; devel at edk2.groups.io;
> > > Gao, Zhichao
> > > > <zhichao.gao at intel.com>
> > > > Cc: Wang, Jian J <jian.j.wang at intel.com>; Lu, XiaoyuX
> > > > <xiaoyux.lu at intel.com>; Maciej Rabeda
> > > <maciej.rabeda at linux.intel.com>;
> > > > Wu, Jiaxin <jiaxin.wu at intel.com>
> > > > Subject: RE: [edk2-devel] [PATCH 0/8] CryptoPkg: Retire
> > > the deprecate
> > > > function
> > > >
> > > > Thanks Siyuan.
> > > > Good, then I think we are aligned.
> > > > I agree with you that it is bad example because the
> > > neither version nor GUID
> > > > is updated.
> > > >
> > > > We can have at least two options here:
> > > > 1) Update version
> > > > We can change old API to be "VOID *Reserved" or
> > > "UNSUPPORT_FUNC
> > > > Reserved" in the EDKII_CRYPTO_PROTOCOL.
> > > >
> > > > I really do not want to see something like
> > > "EDKII_CRYPTO_MD4_INIT
> > > > Md4Init" still existing, because it may let people
> > > think we are still support
> > > > MD4 and use it somewhere.
> > > >
> > > > 2) Update GUID
> > > > Then we can remove the "EDKII_CRYPTO_MD4_INIT Md4Init"
> > > completely.
> > > > Of course, we can still update version although it is
> > > optional.
> > > >
> > > >
> > > > For adding new API, I will definitely prefer #1.
> > > >
> > > > For deprecating old API, if we choose #1, we need add
> > > 17 reserved fields in
> > > > this protocol for MD4, 3DES and RC4.
> > > > If we decide to deprecate HMAC_MD5/HMAC_SHA1 because of
> > > no usage,
> > > > then we need have a protocol with 156 fields, and 29 of
> > > them are reserved.
> > > > As such, I prefer #2 here, unless we have strong reason
> > > to keep 29 reserved
> > > > fields in this protocol.
> > > >
> > > >
> > > > Thank you
> > > > Yao Jiewen
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Fu, Siyuan <siyuan.fu at intel.com>
> > > > > Sent: Monday, March 30, 2020 10:17 AM
> > > > > To: Yao, Jiewen <jiewen.yao at intel.com>; Kinney,
> > > Michael D
> > > > > <michael.d.kinney at intel.com>; devel at edk2.groups.io;
> > > Gao, Zhichao
> > > > > <zhichao.gao at intel.com>
> > > > > Cc: Wang, Jian J <jian.j.wang at intel.com>; Lu, XiaoyuX
> > > > <xiaoyux.lu at intel.com>;
> > > > > Maciej Rabeda <maciej.rabeda at linux.intel.com>; Wu,
> > > Jiaxin
> > > > > <jiaxin.wu at intel.com>
> > > > > Subject: RE: [edk2-devel] [PATCH 0/8] CryptoPkg:
> > > Retire the deprecate
> > > > function
> > > > >
> > > > > Hi, Jiewen
> > > > >
> > > > > I agree with all the strategy you listed for the
> > > Modulization FW update,
> > > > and
> > > > > Mike's
> > > > > for compatibility maintenance. While I have a little
> > > different
> > > > understanding
> > > > > about
> > > > > "permanent binary compatibility". Mainly about what
> > > kind of
> > > > "compatibility" we
> > > > > have to provide.
> > > > >
> > > > > I don't think "compatibility " means we cannot
> > > deprecate any old API.
> > > > Instead of
> > > > > that, I think the goal could be:
> > > > > - If an old binary is using the deprecated API, it
> > > should be able to fail
> > > > gracefully.
> > > > > - If an old binary is NOT using the deprecated API,
> > > it should not be
> > > > impacted and
> > > > > able to work as before.
> > > > >
> > > > > So how we deprecate an API from this internal
> > > protocol is important. The
> > > > > current
> > > > > patch shows a bad example, it removes member
> > > functions from the
> > > > protocol
> > > > > structure, without changing the protocol GUID or
> > > version number. In such
> > > > case,
> > > > > an old binary consumer has no method to know if it's
> > > working with an old
> > > > > protocol
> > > > > or a new one, and may call into incorrect function
> > > even it doesn't use any
> > > > of the
> > > > > deprecated APIs. That's something I want to avoid.
> > > > >
> > > > > Best Regards
> > > > > Siyuan
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Yao, Jiewen <jiewen.yao at intel.com>
> > > > > > Sent: 2020年3月28日 7:43
> > > > > > To: Kinney, Michael D <michael.d.kinney at intel.com>;
> > > > devel at edk2.groups.io;
> > > > > > Fu, Siyuan <siyuan.fu at intel.com>; Gao, Zhichao
> > > <zhichao.gao at intel.com>
> > > > > > Cc: Wang, Jian J <jian.j.wang at intel.com>; Lu,
> > > XiaoyuX
> > > > > > <xiaoyux.lu at intel.com>; Maciej Rabeda
> > > > <maciej.rabeda at linux.intel.com>;
> > > > > > Wu, Jiaxin <jiaxin.wu at intel.com>
> > > > > > Subject: RE: [edk2-devel] [PATCH 0/8] CryptoPkg:
> > > Retire the deprecate
> > > > > > function
> > > > > >
> > > > > > Thanks Mike.
> > > > > > I understand the *private protocol* now. I think it
> > > is OK to put
> > > > > > CryptoProtocol into Private dir to sever that
> > > purpose.
> > > > > > And for private API, I don’t have concern on the
> > > data structure, since it is
> > > > > > invisible.
> > > > > >
> > > > > > Siyuan provided below usage and concern:
> > > > > > "The goal to provide a driver version of crypto
> > > service is to support
> > > > > > modulization
> > > > > > FW update, which means the crypto driver may NOT be
> > > updated together
> > > > > > with
> > > > > > 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
> > > > be
> > > > > > messed."
> > > > > >
> > > > > > This usage might become a problem, when we want to
> > > deprecate an API
> > > > and
> > > > > > keep binary compatibility at same time.
> > > > > > (To be specific, I am not worried about source
> > > compatibility, because we
> > > > can
> > > > > > update both producer and consumer.
> > > > > > I am not worried about adding API, because there
> > > will be no issue on
> > > > > > appending a function at the end.)
> > > > > >
> > > > > > Take below as an example:
> > > > > > Firmware Version 100 uses Crypto Version 100.
> > > > > > We want to deprecate a private API and change to a
> > > new one. So, we
> > > > > > upgrade Crypto to Version 101 and update Firmware
> > > to Version 101.
> > > > > > Of course, we need change *all other consumers* and
> > > rebuild everything
> > > > > > make sure it works correctly.
> > > > > > However, it is hard to support this in
> > > "modulization FW update", because
> > > > we
> > > > > > have no chance to update the binary of firmware
> > > version 100.
> > > > > >
> > > > > > If we have to keep *permanent binary
> > > compatibility*, then we cannot
> > > > > > deprecate any old API, just because that will break
> > > old consumer.
> > > > > > That brings much validation burden, because you
> > > have to test every
> > > > update
> > > > > > in master with old binaries, besides the latest
> > > binaries.
> > > > > > That also brings maintenance burden for the unused
> > > old API. The only
> > > > > > consumer is in the old binary and invisible.
> > > > > > I don’t believe that is what we want.
> > > > > >
> > > > > > Modulization FW update is good feature. And we can
> > > have different
> > > > strategy
> > > > > > for that besides keeping permanent binary
> > > compatibility.
> > > > > > 1) Modulization FW update can be limited a range of
> > > version. At some
> > > > point,
> > > > > > you have to update the whole FW, because there are
> > > too many changes
> > > > or
> > > > > > incompatible binary changes. The cadence of full
> > > update can be longer
> > > > than
> > > > > > the one of partial update. For example, Linux or
> > > windows are making
> > > > > > incompatible change in major version and only keep
> > > compatibility in
> > > > minor
> > > > > > version.
> > > > > > 2) A project can branch the production launch
> > > firmware, and only keep
> > > > > > binary compatibility and support the modulization
> > > FW update within this
> > > > > > branch. As such, the big update in master won't
> > > impact this branch. If a
> > > > > > production may choose to resync to master, at that
> > > time a full firmware
> > > > > > update is required. I guess most people are using
> > > this way in a real
> > > > > > production.
> > > > > >
> > > > > > Thought?
> > > > > >
> > > > > > Thank you
> > > > > > Yao Jiewen
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Kinney, Michael D
> > > <michael.d.kinney at intel.com>
> > > > > > > Sent: Saturday, March 28, 2020 12:38 AM
> > > > > > > To: devel at edk2.groups.io; Yao, Jiewen
> > > <jiewen.yao at intel.com>; Fu,
> > > > Siyuan
> > > > > > > <siyuan.fu at intel.com>; Gao, Zhichao
> > > <zhichao.gao at intel.com>; Kinney,
> > > > > > > Michael D <michael.d.kinney at intel.com>
> > > > > > > Cc: Wang, Jian J <jian.j.wang at intel.com>; Lu,
> > > XiaoyuX
> > > > > > <xiaoyux.lu at intel.com>;
> > > > > > > Maciej Rabeda <maciej.rabeda at linux.intel.com>;
> > > Wu, Jiaxin
> > > > > > > <jiaxin.wu at intel.com>
> > > > > > > Subject: RE: [edk2-devel] [PATCH 0/8] CryptoPkg:
> > > Retire the deprecate
> > > > > > function
> > > > > > >
> > > > > > > Jiewen,
> > > > > > >
> > > > > > > The purpose of private includes is to keep
> > > modules/lib
> > > > > > > in *other* packages from using interfaces that
> > > are the
> > > > > > > package with the private interface does not want
> > > other
> > > > > > > packages to use and does not want to have to
> > > coordinate
> > > > > > > with other packages if that package owner decides
> > > to
> > > > > > > make changes to those private interfaces.
> > > > > > >
> > > > > > > For modules/libs within package that do use
> > > private
> > > > > > > includes, the package owner gets to decide how to maintain
> > > > > > > the interfaces in the private includes
> > > to
> > > > > > > support those modules/libs.
> > > > > > >
> > > > > > > For example, the CryptoPkg has modules that are intended to
> > > > > > > be used as pre-built binaries, so the CryptoPkg owner needs
> > > > > > > to make sure the
> > > maintenance
> > > > > > > of the private includes supports both the source
> > > and
> > > > > > > binary use cases.
> > > > > > >
> > > > > > > The private Protocol/PPI interfaces in the
> > > CryptoPkg
> > > > > > > were designed to support future expansion. The
> > > first
> > > > > > > API in the Protocol/PPI is GetVersion(). The
> > > version
> > > > > > > value returned can be used to have different
> > > layouts
> > > > > > > of fields in the Protocol/PPI. In order to
> > > support
> > > > > > > backwards compatibility, APIs are added to the
> > > end
> > > > > > > of the Protocol/PPI structure as the version
> > > value
> > > > > > > is increased. You will notice that there is an
> > > X509
> > > > > > > service that was added further down than the
> > > initial
> > > > > > > grouping. This is just an example of how the
> > > CryptoPkg
> > > > > > > is maintaining a private interface for binary use
> > > cases.
> > > > > > > Other packages may choose alternate techniques.
> > > > > > >
> > > > > > > Thanks,
> > > > > > >
> > > > > > > Mike
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: devel at edk2.groups.io
> > > <devel at edk2.groups.io> On
> > > > > > > > Behalf Of Yao, Jiewen
> > > > > > > > Sent: Thursday, March 26, 2020 9:59 PM
> > > > > > > > To: Fu, Siyuan <siyuan.fu at intel.com>;
> > > > > > > > devel at edk2.groups.io; Gao, Zhichao <zhichao.gao at intel.com>
> > > > > > > > Cc: Wang, Jian J <jian.j.wang at intel.com>; Lu,
> > > XiaoyuX
> > > > > > > > <xiaoyux.lu at intel.com>; Maciej Rabeda
> > > > > > > > <maciej.rabeda at linux.intel.com>; Wu, Jiaxin
> > > > > > > > <jiaxin.wu at intel.com>
> > > > > > > > Subject: Re: [edk2-devel] [PATCH 0/8]
> > > CryptoPkg: Retire
> > > > > > > > the deprecate function
> > > > > > > >
> > > > > > > > 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 internal 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 compatibility 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 - just 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 protocol interface is unstable.
> > > > > > > >
> > > > > > > > Today, we have defined SHA2 set, and
> > > deprecating SHA1 and
> > > > > > > > older one. Tomorrow 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/Incl
> > > > > > > > ude/Protocol/Hash2.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 cipher algo.
> > > > > > > > For example,
> > > > > > > >
> > > https://www.openssl.org/docs/man1.1.1/man3/EVP_EncryptIni
> > > > > > > > t_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 can 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 <siyuan.fu at intel.com>
> > > > > > > > > Sent: Friday, March 27, 2020 11:07 AM
> > > > > > > > > To: Yao, Jiewen <jiewen.yao at intel.com>;
> > > > > > > > devel at edk2.groups.io; Gao, Zhichao
> > > > > > > > > <zhichao.gao at intel.com>
> > > > > > > > > Cc: Wang, Jian J <jian.j.wang at intel.com>; Lu,
> > > XiaoyuX
> > > > > > > > <xiaoyux.lu at intel.com>;
> > > > > > > > > Maciej Rabeda
> > > <maciej.rabeda at linux.intel.com>; Wu,
> > > > > > > > Jiaxin
> > > > > > > > > <jiaxin.wu at intel.com>
> > > > > > > > > Subject: RE: [edk2-devel] [PATCH 0/8]
> > > CryptoPkg: Retire
> > > > > > > > the deprecate function
> > > > > > > > >
> > > > > > > > > Hi, Jiewen
> > > > > > > > >
> > > > > > > > > Although the protocol is private, a
> > > corresponding
> > > > > > > > BaseCryptoLib instance is
> > > > > > > > > not private, like PeiCryptLib.inf,
> > > RuntimeCryptLib,
> > > > > > > > etc. These library instances
> > > > > > > > > will be static linked to the consumer driver,
> > > for
> > > > > > > > example an iSCSI network driver.
> > > > > > > > > So actually it's not a "private" change
> > > inside
> > > > > > > > CryptoPkg.
> > > > > > > > >
> > > > > > > > > The goal to provide a driver version of
> > > crypto service
> > > > > > > > is to support modulization
> > > > > > > > > FW update, which means the crypto driver may
> > > NOT be
> > > > > > > > updated together with
> > > > > > > > > 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 be
> > > > > > > > > messed.
> > > > > > > > >
> > > > > > > > > Best Regards
> > > > > > > > > Siyuan
> > > > > > > > >
> > > > > > > > > > -----Original Message-----
> > > > > > > > > > From: Yao, Jiewen <jiewen.yao at intel.com>
> > > > > > > > > > Sent: 2020年3月27日 10:58
> > > > > > > > > > To: devel at edk2.groups.io; Fu, Siyuan
> > > > > > > > <siyuan.fu at intel.com>; Gao, Zhichao
> > > > > > > > > > <zhichao.gao at intel.com>
> > > > > > > > > > Cc: Wang, Jian J <jian.j.wang at intel.com>;
> > > Lu, XiaoyuX
> > > > > > > > > <xiaoyux.lu at intel.com>;
> > > > > > > > > > Maciej Rabeda
> > > <maciej.rabeda at linux.intel.com>; Wu,
> > > > > > > > Jiaxin
> > > > > > > > > > <jiaxin.wu at intel.com>
> > > > > > > > > > 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/P
> > > > > > > > rivate/Protocol/C
> > > > > > > > > > rypto.h
> > > > > > > > > >
> > > > > > > > > > Why we cannot change?
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > -----Original Message-----
> > > > > > > > > > > From: devel at edk2.groups.io
> > > <devel at edk2.groups.io>
> > > > > > > > On Behalf Of Siyuan,
> > > > > > > > > Fu
> > > > > > > > > > > Sent: Friday, March 27, 2020 10:47 AM
> > > > > > > > > > > To: Gao, Zhichao <zhichao.gao at intel.com>;
> > > > > > > > devel at edk2.groups.io
> > > > > > > > > > > Cc: Wang, Jian J <jian.j.wang at intel.com>;
> > > Lu,
> > > > > > > > XiaoyuX
> > > > > > > > > > <xiaoyux.lu at intel.com>;
> > > > > > > > > > > Maciej Rabeda
> > > <maciej.rabeda at linux.intel.com>; Wu,
> > > > > > > > Jiaxin
> > > > > > > > > > > <jiaxin.wu at intel.com>
> > > > > > > > > > > Subject: Re: [edk2-devel] [PATCH 0/8]
> > > CryptoPkg:
> > > > > > > > Retire the deprecate
> > > > > > > > > > function
> > > > > > > > > > >
> > > > > > > > > > > Hi, Zhichao
> > > > > > > > > > >
> > > > > > > > > > > We should never move/delete a member
> > > field of a
> > > > > > > > previous defined protocol
> > > > > > > > > > > Interface. Instead, these protocol APIs
> > > shall be
> > > > > > > > kept and return an error code
> > > > > > > > > > > If the function is retired. Otherwise the
> > > consumer
> > > > > > > > driver may call into an
> > > > > > > > > > > Incorrect function if it's build with
> > > different
> > > > > > > > codebase/PCD settings with the
> > > > > > > > > > > Crypto PEI/DXE/SMM driver.
> > > > > > > > > > > This comment applies to all the
> > > > > > > > EDKII_CRYPTO_PROTOCOL related changes
> > > > > > > > > in
> > > > > > > > > > > your patch set.
> > > > > > > > > > >
> > > > > > > > > > > Best Regards
> > > > > > > > > > > Siyuan
> > > > > > > > > > >
> > > > > > > > > > > > -----Original Message-----
> > > > > > > > > > > > From: Gao, Zhichao
> > > <zhichao.gao at intel.com>
> > > > > > > > > > > > Sent: 2020年3月27日 9:56
> > > > > > > > > > > > To: devel at edk2.groups.io
> > > > > > > > > > > > Cc: Wang, Jian J
> > > <jian.j.wang at intel.com>; Lu,
> > > > > > > > XiaoyuX
> > > > > > > > > > > <xiaoyux.lu at intel.com>;
> > > > > > > > > > > > Maciej Rabeda
> > > <maciej.rabeda at linux.intel.com>;
> > > > > > > > Wu, Jiaxin
> > > > > > > > > > > > <jiaxin.wu at intel.com>; Fu, Siyuan
> > > > > > > > <siyuan.fu at intel.com>
> > > > > > > > > > > > Subject: [PATCH 0/8] CryptoPkg: Retire
> > > the
> > > > > > > > deprecate function
> > > > > > > > > > > >
> > > > > > > > > > > > REF:
> > > > > > > >
> > > https://bugzilla.tianocore.org/show_bug.cgi?id=1682
> > > > > > > > > > > > REF:
> > > > > > > >
> > > https://bugzilla.tianocore.org/show_bug.cgi?id=1898
> > > > > > > > > > > >
> > > > > > > > > > > > MD4, AR4, Tdes, Aes Ecb mode, MD5 and
> > > SHA1 is not
> > > > > > > > secure any longer.
> > > > > > > > > > > > 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 compatibility.
> > > > > > > > > > > > So add two pcds to control the MD5 and
> > > SHA1
> > > > > > > > enablement. Set the pcds
> > > > > > > > > > > > default value to false to indicate they
> > > are
> > > > > > > > deprecated.
> > > > > > > > > > > >
> > > > > > > > > > > > NetWorkPkg's iSCSI driver would consume
> > > the MD5
> > > > > > > > function, so change
> > > > > > > > > > > > the md5 pcd to TURE when iSCSI is
> > > enabled.
> > > > > > > > > > > >
> > > > > > > > > > > > Cc: Jian J Wang <jian.j.wang at intel.com>
> > > > > > > > > > > > Cc: Xiaoyu Lu <xiaoyux.lu at intel.com>
> > > > > > > > > > > > Cc: Maciej Rabeda
> > > <maciej.rabeda at linux.intel.com>
> > > > > > > > > > > > Cc: Jiaxin Wu <jiaxin.wu at intel.com>
> > > > > > > > > > > > Cc: Siyuan Fu <siyuan.fu at intel.com>
> > > > > > > > > > > > Signed-off-by: Zhichao Gao
> > > > > > > > <zhichao.gao at intel.com>
> > > > > > > > > > > >
> > > > > > > > > > > > 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 function
> > > > > > > > > > > > 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/CryptArc4.c
> > > > > > > > > > > > delete mode 100644
> > > > > > > > > >
> > > CryptoPkg/Library/BaseCryptLib/Cipher/CryptArc4Null.c
> > > > > > > > > > > > delete mode 100644
> > > > > > > >
> > > CryptoPkg/Library/BaseCryptLib/Cipher/CryptTdes.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
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#57296): https://edk2.groups.io/g/devel/message/57296
Mute This Topic: https://groups.io/mt/72579461/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