From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web10.72071.1656940602971450602 for ; Mon, 04 Jul 2022 06:16:43 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: pierre.gondois@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id D00C323A; Mon, 4 Jul 2022 06:16:42 -0700 (PDT) Received: from [10.57.41.108] (unknown [10.57.41.108]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 5B3313F66F; Mon, 4 Jul 2022 06:16:40 -0700 (PDT) Message-ID: Date: Mon, 4 Jul 2022 15:16:19 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.1 Subject: Re: [edk2-devel] [PATCH RESEND v1 5/7] MdePkg/AesLib: Definition for AES library class interface To: "Yao, Jiewen" , "devel@edk2.groups.io" Cc: Sami Mujawar , Leif Lindholm , Ard Biesheuvel , Rebecca Cran , "Kinney, Michael D" , "Gao, Liming" , Edward Pickup References: <20220629191355.2618844-1-Pierre.Gondois@arm.com> <20220629191355.2618844-6-Pierre.Gondois@arm.com> <499390b1-0916-2f25-3831-b1e2d406bc49@arm.com> From: "PierreGondois" In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable On 7/1/22 18:11, Yao, Jiewen wrote: >=20 >=20 >> -----Original Message----- >> From: devel@edk2.groups.io On Behalf Of >> PierreGondois >> Sent: Friday, July 1, 2022 11:23 PM >> To: Yao, Jiewen ; devel@edk2.groups.io >> Cc: Sami Mujawar ; Leif Lindholm >> ; Ard Biesheuvel ; >> Rebecca Cran ; Kinney, Michael D >> ; Gao, Liming ; >> Edward Pickup >> Subject: Re: [edk2-devel] [PATCH RESEND v1 5/7] MdePkg/AesLib: Definitio= n 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=E2=80=99s 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-cont= ained. >> 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. >=20 > [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. Yes right, there would be indeed no dependency between the MdePkg and ArmPk= g, the above case is perfectly correct. >=20 >=20 >=20 >>> >>> (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 DrbgL= ib code is >> common to all architectures. >> >> The above explains how/why the DrbgLib is modularized. If the DrbgLib wa= s put >> in the SecurityPkg (I think this would fit), there would be no need to h= ave the >> AesLib in the MdePkg. Would the distribution below fit for you ? >> >> DrbgLib.h (interface) -> SecurityPkg >> DrbgLib (instance) -> SecurityPkg (note: DrbgLibAes !=3D DrbgLib) >> AesLib.h (interface) -> CryptoPkg >> AesLib (instance) -> ArmPkg or CryptoPkg >=20 > [Jiewen] I have expressed my concern on AesLib.h public API definition, i= f it is in MdePkg, or CryptoPkg. >=20 > In firmware, most program just wants to get a Random value. We already ha= ve RngLib and BaseCryptoLib. > I think it is enough for the consumer. Adding more public APIs just confu= ses people. >=20 > 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. >=20 > So far, I feel it is an overdesign to expose AesLib.h, because I don=E2= =80=99t 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=E2=80=99t see the value to have AesLib.h= . To continue the discussion on one thread, please see the answer to: https://edk2.groups.io/g/devel/message/91009 Regards, Pierre >=20 >=20 > Thank you > Yao Jiewen >=20 >=20 >> >> Regards, >> Pierre >> >>> >>> I don=E2=80=99t see the need put AesLib.h to MdePkg. >>> And I don=E2=80=99t have comment for ArmPkg. >>> >>> Thank you >>> Yao Jiewen >>> >>> >>>> -----Original Message----- >>>> From: Pierre Gondois >>>> Sent: Friday, July 1, 2022 9:59 PM >>>> To: Yao, Jiewen ; devel@edk2.groups.io >>>> Cc: Sami Mujawar ; Leif Lindholm >>>> ; Ard Biesheuvel ; >>>> Rebecca Cran ; Kinney, Michael D >>>> ; Gao, Liming ; >>>> Edward Pickup >>>> Subject: Re: [edk2-devel] [PATCH RESEND v1 5/7] MdePkg/AesLib: Definit= ion >> 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 fu= nction. >>>>> 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 ne= w >>>> interface needed is for a single block encryption. So adding thes= e >>>> 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 >>>>>> Sent: Friday, July 1, 2022 5:49 PM >>>>>> To: Yao, Jiewen ; devel@edk2.groups.io >>>>>> Cc: Sami Mujawar ; Leif Lindholm >>>>>> ; Ard Biesheuvel >> ; >>>>>> Rebecca Cran ; Kinney, Michael D >>>>>> ; Gao, Liming = ; >>>>>> Edward Pickup >>>>>> 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 cryptol= ib? - >>>>>> >>>> >> 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/OpensslL= ib/ >>>>>> X64, can ARM use the similar model? >>>>>>> >>>>>> >>>>>> We also need to have a look at this. However this might be a bit mor= e >>>>>> difficult if we want to avoid Openssl license. >>>>>> >>>>>>> 3) Do you have chance to take a look if this interface is good enou= gh 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 On Behalf Of >>>>>>>> PierreGondois >>>>>>>> Sent: Thursday, June 30, 2022 3:14 AM >>>>>>>> To: devel@edk2.groups.io >>>>>>>> Cc: Sami Mujawar ; Leif Lindholm >>>>>>>> ; Ard Biesheuvel >>>> ; >>>>>>>> Rebecca Cran ; Kinney, Michael D >>>>>>>> ; Gao, Liming >> ; >>>>>>>> Edward Pickup >>>>>>>> Subject: [edk2-devel] [PATCH RESEND v1 5/7] MdePkg/AesLib: Definit= ion >>>> for >>>>>> AES >>>>>>>> library class interface >>>>>>>> >>>>>>>> From: Pierre Gondois >>>>>>>> >>>>>>>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3970 >>>>>>>> >>>>>>>> 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 >>>>>>>> --- >>>>>>>> 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.
>>>>>>>> + >>>>>>>> + 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 =3D 4: >>>>>>>> + Number of columns (32-bit words) comprising the State >>>>>>>> + - Nr =3D 10, 12, or 14: >>>>>>>> + Number of rounds. >>>>>>>> + */ >>>>>>>> +#define AES_MAX_KEYLENGTH_U32 (4 * (14 + 1)) >>>>>>>> + >>>>>>>> +/** A context holding information to for AES encryption/decryptio= n. >>>>>>>> + */ >>>>>>>> +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.AARC= H64] >>>>>>>> ## @libraryclass Provides services to generate random num= ber. >>>>>>>> # >>>>>>>> -- >>>>>>>> 2.25.1 >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> -=3D-=3D-=3D-=3D-=3D-=3D >>>>>>>> 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+owner@edk2.groups.io >>>>>>>> Unsubscribe: https://edk2.groups.io/g/devel/unsub >>>> [jiewen.yao@intel.com] >>>>>>>> -=3D-=3D-=3D-=3D-=3D-=3D >>>>>>>> >>>>>>> >> >> >>=20 >> >=20