> -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of > PierreGondois > Sent: Friday, July 1, 2022 11:23 PM > To: Yao, Jiewen <jiewen....@intel.com>; devel@edk2.groups.io > Cc: Sami Mujawar <sami.muja...@arm.com>; Leif Lindholm > <quic_llind...@quicinc.com>; Ard Biesheuvel <ardb+tianoc...@kernel.org>; > Rebecca Cran <rebe...@bsdio.com>; Kinney, Michael D > <michael.d.kin...@intel.com>; Gao, Liming <gaolim...@byosoft.com.cn>; > Edward Pickup <edward.pic...@arm.com> > Subject: Re: [edk2-devel] [PATCH RESEND v1 5/7] MdePkg/AesLib: Definition for > AES library class interface > > > > On 7/1/22 16:40, Yao, Jiewen wrote: > > Please allow me to clarify my understanding: > > > > 1) You want to promote DrbgLib to MdePkg. -- That is a different topic. We > should discuss that in other thread. > > Now, let’s assume it is OK. > > > > 2) You want to use AES as an implementation for DrbgLib. > > That is also reasonable. > > > > Please note: MdePkg only requires the library interface to be > > self-contained. > But not the library instance. > > > > Assuming you are working on ARM solution. It is legal that: > > DrbgLib.h (interface) -> MdePkg. > > AesLib.h (interface) -> ArmPkg > > AesLib (instance) -> ArmPkg > > DrbgLibAes (instance) -> ArmPkg. > > I don't think this option is possible as the interface definition would be in > ArmPkg, > making MdePkg dependent on ArmPkg.
[Jiewen] Why MdePkg depends on ArmPkg??? MdePkg only have library API. Why your DrbgLib.h includes AES information? If so, I would recommend you need fix the DrbgLib.h. > > > > (or) > > DrbgLib.h (interface) -> MdePkg. > > DrbgLibAes (instance) -> ArmPkg. (you can put AES implementation here > directly, without AesLib.h) > > I agree this option is possible, but I think it would be inefficient as the > only Arm > (or arch) > specific parts of the DrbgLib are: > - the Trng implementation > - the Aes implementation > Both are defined as libraries used by the DrbgLib. The rest of the DrbgLib > code is > common to all architectures. > > The above explains how/why the DrbgLib is modularized. If the DrbgLib was put > in the SecurityPkg (I think this would fit), there would be no need to have > the > AesLib in the MdePkg. Would the distribution below fit for you ? > > DrbgLib.h (interface) -> SecurityPkg > DrbgLib (instance) -> SecurityPkg (note: DrbgLibAes != DrbgLib) > AesLib.h (interface) -> CryptoPkg > AesLib (instance) -> ArmPkg or CryptoPkg [Jiewen] I have expressed my concern on AesLib.h public API definition, if it is in MdePkg, or CryptoPkg. In firmware, most program just wants to get a Random value. We already have RngLib and BaseCryptoLib. I think it is enough for the consumer. Adding more public APIs just confuses people. For producer, you want to build multiple layers, that is fine. I would suggest to not expose such complexity to the consumer. It could be limited in your internal implementation. So far, I feel it is an overdesign to expose AesLib.h, because I don’t see the use other use case besides DrbgLib. Even if you want to add AES instruction to BaseCryptoLib, you can add the ARM version directly. I still don’t see the value to have AesLib.h. Thank you Yao Jiewen > > Regards, > Pierre > > > > > I don’t see the need put AesLib.h to MdePkg. > > And I don’t have comment for ArmPkg. > > > > Thank you > > Yao Jiewen > > > > > >> -----Original Message----- > >> From: Pierre Gondois <pierre.gond...@arm.com> > >> Sent: Friday, July 1, 2022 9:59 PM > >> To: Yao, Jiewen <jiewen....@intel.com>; devel@edk2.groups.io > >> Cc: Sami Mujawar <sami.muja...@arm.com>; Leif Lindholm > >> <quic_llind...@quicinc.com>; Ard Biesheuvel <ardb+tianoc...@kernel.org>; > >> Rebecca Cran <rebe...@bsdio.com>; Kinney, Michael D > >> <michael.d.kin...@intel.com>; Gao, Liming <gaolim...@byosoft.com.cn>; > >> Edward Pickup <edward.pic...@arm.com> > >> Subject: Re: [edk2-devel] [PATCH RESEND v1 5/7] MdePkg/AesLib: Definition > for > >> AES library class interface > >> > >> Hello Jiewen, > >> > >> On 7/1/22 13:55, Yao, Jiewen wrote: > >>> I have two concern: > >>> > >>> 1) I am worried that this API might be misused. Usually, a crypto API > >>> should > be > >> secure enough to avoid misuse. For example, if a program wants to use AES > >> encryption, it must NOT use this AES API. Instead it must use AES_CCB + MAC > or > >> AES_GCM. (or equivalent) > >>> I doubt if this is right direction to expose this publicly in MdePkg. > >>> > >>> 2) I am not sure how this API will be used in CryptoLib. > >>> Ideally, an EDKII program should use crypto lib API for any crypto > >>> function. > >>> However, I do not understand how that is done. > >>> > >> > >> The reason the AesLib was put in MdePkg: > >> - The DrbgLib was thought to be generic enough to be in MdePkg > >> (this is arguable). > >> - The MdePkg must be self-contained (i.e. not use libraries/modules > >> defined in other packages). Thus if an AesLib is created, it must be > >> in the MdePkg. > >> I don't mind moving the DrbgLib (and the AesLib) to another package if > >> this is the common agreement. > >> > >> Why a single block AesLib should be created: > >> - The DrbgLib requires to have Aes single block encryption. A software > >> implementation of Aes is also available (and used) at [2] in the > >> SecurityPkg. This implementation is limited to a module scope. > >> Thus, there is a need create a common library for this. > >> - I agree that this AesLib should not be mistaken with something else > >> (cf your comment about AES_CCB + MAC or AES_GCM). However, the new > >> interface needed is for a single block encryption. So adding these > >> new functions to: > >> CryptoPkg/Include/Library/BaseCryptLib.h > >> won't make it safer. > >> > >> Please let me know if there are still concerns, > >> Regards, > >> Pierre > >> > >> Note: > >> The functions in AesLib are equivalent to the ones in [4]. > >> > >> [1] https://edk2.groups.io/g/devel/files/Designs/2021/0116/EDKII%20- > >> %20Proposed%20update%20to%20RNG%20implementation.pdf > >> [2] > >> > https://github.com/tianocore/edk2/blob/f966093f5bb88e6fccac8e0b9eeca6c73 > >> > aef0c35/SecurityPkg/RandomNumberGenerator/RngDxe/Rand/AesCore.c#L215 > >> [3] > >> > https://github.com/tianocore/edk2/blob/f966093f5bb88e6fccac8e0b9eeca6c73 > >> aef0c35/CryptoPkg/Include/Library/BaseCryptLib.h#L1128 > >> [4] https://github.com/openssl/openssl/blob/master/crypto/aes/aes_core.c > >> > >> > >>> > >>> I think it is good idea to enable ARM AES hardware accelerator. > >>> And I would like to see a total solution. > >>> > >>> It will be great, if you also submit the cryptopkg patch to help me > understand > >> how to achieve that. > >>> > >>> Thank you > >>> Yao Jiewen > >>> > >>> > >>>> -----Original Message----- > >>>> From: Pierre Gondois <pierre.gond...@arm.com> > >>>> Sent: Friday, July 1, 2022 5:49 PM > >>>> To: Yao, Jiewen <jiewen....@intel.com>; devel@edk2.groups.io > >>>> Cc: Sami Mujawar <sami.muja...@arm.com>; Leif Lindholm > >>>> <quic_llind...@quicinc.com>; Ard Biesheuvel > <ardb+tianoc...@kernel.org>; > >>>> Rebecca Cran <rebe...@bsdio.com>; Kinney, Michael D > >>>> <michael.d.kin...@intel.com>; Gao, Liming <gaolim...@byosoft.com.cn>; > >>>> Edward Pickup <edward.pic...@arm.com> > >>>> Subject: Re: [edk2-devel] [PATCH RESEND v1 5/7] MdePkg/AesLib: > Definition > >> for > >>>> AES library class interface > >>>> > >>>> Hello Yao, > >>>> > >>>> On 6/30/22 02:29, Yao, Jiewen wrote: > >>>>> Hi > >>>>> 1) Would you please educate me, how this library be used in cryptolib? - > >>>> > >> > https://github.com/tianocore/edk2/blob/master/CryptoPkg/Include/Library/Bas > >>>> eCryptLib.h#L1091 > >>>>> > >>>>> Currently, we have AES_CBC. We are going to add AES_GCM in near > future. > >>>>> > >>>> > >>>> We are currently looking forward to do that. Just to be sure, the > >>>> AesInit() function pointed above is for AesCbcEncrypt(), which can > >>>> encrypt a buffer. > >>>> The AesInitCtx() in this file is for a single block encryption. So > >>>> there should be nothing preventing from implementing CBC (or other) > >>>> encryption based on the Aes block encryption added by this patch-set. > >>>> > >>>>> 2) For Intel AES_NI, we added support in OpensslLib directly - > >>>> > >> > https://github.com/tianocore/edk2/tree/master/CryptoPkg/Library/OpensslLib/ > >>>> X64, can ARM use the similar model? > >>>>> > >>>> > >>>> We also need to have a look at this. However this might be a bit more > >>>> difficult if we want to avoid Openssl license. > >>>> > >>>>> 3) Do you have chance to take a look if this interface is good enough to > >>>> implement Intel AES_NI instruction? > >>>>> > >>>> > >>>> We have not looked at the AES_NI instruction, but the interface > >>>> definition should be generic enough to accept any implementation. > >>>> Please tell us if you think this requires modification. > >>>> > >>>> Regards, > >>>> Pierre > >>>> > >>>>> Thank you > >>>>> Yao Jiewen > >>>>> > >>>>>> -----Original Message----- > >>>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of > >>>>>> PierreGondois > >>>>>> Sent: Thursday, June 30, 2022 3:14 AM > >>>>>> To: devel@edk2.groups.io > >>>>>> Cc: Sami Mujawar <sami.muja...@arm.com>; Leif Lindholm > >>>>>> <quic_llind...@quicinc.com>; Ard Biesheuvel > >> <ardb+tianoc...@kernel.org>; > >>>>>> Rebecca Cran <rebe...@bsdio.com>; Kinney, Michael D > >>>>>> <michael.d.kin...@intel.com>; Gao, Liming > <gaolim...@byosoft.com.cn>; > >>>>>> Edward Pickup <edward.pic...@arm.com> > >>>>>> Subject: [edk2-devel] [PATCH RESEND v1 5/7] MdePkg/AesLib: Definition > >> for > >>>> AES > >>>>>> library class interface > >>>>>> > >>>>>> From: Pierre Gondois <pierre.gond...@arm.com> > >>>>>> > >>>>>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3970 > >>>>>> > >>>>>> The FIPS PUB 197: "Advanced Encryption Standard (AES)" > >>>>>> details the AES algorithm. Add a library to allow > >>>>>> different architecture specific implementations. > >>>>>> > >>>>>> Signed-off-by: Pierre Gondois <pierre.gond...@arm.com> > >>>>>> --- > >>>>>> MdePkg/Include/Library/AesLib.h | 104 > >>>> ++++++++++++++++++++++++++++++++ > >>>>>> MdePkg/MdePkg.dec | 4 ++ > >>>>>> 2 files changed, 108 insertions(+) > >>>>>> create mode 100644 MdePkg/Include/Library/AesLib.h > >>>>>> > >>>>>> diff --git a/MdePkg/Include/Library/AesLib.h > >>>> b/MdePkg/Include/Library/AesLib.h > >>>>>> new file mode 100644 > >>>>>> index 000000000000..bc3408bb249b > >>>>>> --- /dev/null > >>>>>> +++ b/MdePkg/Include/Library/AesLib.h > >>>>>> @@ -0,0 +1,104 @@ > >>>>>> +/** @file > >>>>>> + AES library. > >>>>>> + > >>>>>> + Copyright (c) 2022, Arm Limited. All rights reserved.<BR> > >>>>>> + > >>>>>> + SPDX-License-Identifier: BSD-2-Clause-Patent > >>>>>> + > >>>>>> + @par Reference(s): > >>>>>> + - FIPS 197 November 26, 2001: > >>>>>> + Specification for the ADVANCED ENCRYPTION STANDARD (AES) > >>>>>> +**/ > >>>>>> + > >>>>>> +#ifndef AES_LIB_H_ > >>>>>> +#define AES_LIB_H_ > >>>>>> + > >>>>>> +/// Key size in bytes. > >>>>>> +#define AES_KEY_SIZE_128 16 > >>>>>> +#define AES_KEY_SIZE_192 24 > >>>>>> +#define AES_KEY_SIZE_256 32 > >>>>>> +#define AES_BLOCK_SIZE 16 > >>>>>> + > >>>>>> +/* > >>>>>> + The Key Expansion generates a total of Nb (Nr + 1) words with: > >>>>>> + - Nb = 4: > >>>>>> + Number of columns (32-bit words) comprising the State > >>>>>> + - Nr = 10, 12, or 14: > >>>>>> + Number of rounds. > >>>>>> + */ > >>>>>> +#define AES_MAX_KEYLENGTH_U32 (4 * (14 + 1)) > >>>>>> + > >>>>>> +/** A context holding information to for AES encryption/decryption. > >>>>>> + */ > >>>>>> +typedef struct { > >>>>>> + /// Expanded encryption key. > >>>>>> + UINT32 ExpEncKey[AES_MAX_KEYLENGTH_U32]; > >>>>>> + /// Expanded decryption key. > >>>>>> + UINT32 ExpDecKey[AES_MAX_KEYLENGTH_U32]; > >>>>>> + /// Key size, in bytes. > >>>>>> + /// Must be one of 16|24|32. > >>>>>> + UINT32 KeySize; > >>>>>> +} AES_CTX; > >>>>>> + > >>>>>> +/** Encrypt an AES block. > >>>>>> + > >>>>>> + Buffers are little-endian. Overlapping is not checked. > >>>>>> + > >>>>>> + @param [in] AesCtx AES context. > >>>>>> + AesCtx is initialized with AesInitCtx (). > >>>>>> + @param [in] InBlock Input Block. The block to cipher. > >>>>>> + @param [out] OutBlock Output Block. The ciphered block. > >>>>>> + > >>>>>> + @retval RETURN_SUCCESS Success. > >>>>>> + @retval RETURN_INVALID_PARAMETER Invalid parameter. > >>>>>> + @retval RETURN_UNSUPPORTED Unsupported. > >>>>>> +**/ > >>>>>> +RETURN_STATUS > >>>>>> +EFIAPI > >>>>>> +AesEncrypt ( > >>>>>> + IN AES_CTX *AesCtx, > >>>>>> + IN UINT8 CONST *InBlock, > >>>>>> + OUT UINT8 *OutBlock > >>>>>> + ); > >>>>>> + > >>>>>> +/** Decrypt an AES block. > >>>>>> + > >>>>>> + Buffers are little-endian. Overlapping is not checked. > >>>>>> + > >>>>>> + @param [in] AesCtx AES context. > >>>>>> + AesCtx is initialized with AesInitCtx (). > >>>>>> + @param [in] InBlock Input Block. The block to de-cipher. > >>>>>> + @param [out] OutBlock Output Block. The de-ciphered block. > >>>>>> + > >>>>>> + @retval RETURN_SUCCESS Success. > >>>>>> + @retval RETURN_INVALID_PARAMETER Invalid parameter. > >>>>>> + @retval RETURN_UNSUPPORTED Unsupported. > >>>>>> +**/ > >>>>>> +RETURN_STATUS > >>>>>> +EFIAPI > >>>>>> +AesDecrypt ( > >>>>>> + IN AES_CTX *AesCtx, > >>>>>> + IN UINT8 CONST *InBlock, > >>>>>> + OUT UINT8 *OutBlock > >>>>>> + ); > >>>>>> + > >>>>>> +/** Initialize an AES_CTX structure. > >>>>>> + > >>>>>> + @param [in] Key AES key. Buffer of KeySize bytes. > >>>>>> + The buffer is little endian. > >>>>>> + @param [in] KeySize Size of the key. Must be one of > >>>>>> 128|192|256. > >>>>>> + @param [in, out] AesCtx AES context to initialize. > >>>>>> + > >>>>>> + @retval RETURN_SUCCESS Success. > >>>>>> + @retval RETURN_INVALID_PARAMETER Invalid parameter. > >>>>>> + @retval RETURN_UNSUPPORTED Unsupported. > >>>>>> +**/ > >>>>>> +RETURN_STATUS > >>>>>> +EFIAPI > >>>>>> +AesInitCtx ( > >>>>>> + IN UINT8 *Key, > >>>>>> + IN UINT32 KeySize, > >>>>>> + IN OUT AES_CTX *AesCtx > >>>>>> + ); > >>>>>> + > >>>>>> +#endif // AES_LIB_H_ > >>>>>> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec > >>>>>> index 7ff26e22f915..078ae9323ba6 100644 > >>>>>> --- a/MdePkg/MdePkg.dec > >>>>>> +++ b/MdePkg/MdePkg.dec > >>>>>> @@ -280,6 +280,10 @@ [LibraryClasses] > >>>>>> # > >>>>>> TrngLib|Include/Library/TrngLib.h > >>>>>> > >>>>>> + ## @libraryclass Provides AES encryption/decryption services. > >>>>>> + # > >>>>>> + AesLib|Include/Library/AesLib.h > >>>>>> + > >>>>>> [LibraryClasses.IA32, LibraryClasses.X64, LibraryClasses.AARCH64] > >>>>>> ## @libraryclass Provides services to generate random number. > >>>>>> # > >>>>>> -- > >>>>>> 2.25.1 > >>>>>> > >>>>>> > >>>>>> > >>>>>> -=-=-=-=-=-= > >>>>>> Groups.io Links: You receive all messages sent to this group. > >>>>>> View/Reply Online (#90895): > >> https://edk2.groups.io/g/devel/message/90895 > >>>>>> Mute This Topic: https://groups.io/mt/92072168/1772286 > >>>>>> Group Owner: devel+ow...@edk2.groups.io > >>>>>> Unsubscribe: https://edk2.groups.io/g/devel/unsub > >> [jiewen....@intel.com] > >>>>>> -=-=-=-=-=-= > >>>>>> > >>>>> > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#90996): https://edk2.groups.io/g/devel/message/90996 Mute This Topic: https://groups.io/mt/92072168/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
