Re: [edk2-devel] [PATCH RESEND v1 5/7] MdePkg/AesLib: Definition for AES library class interface

["PierreGondois" <pierre.gondois@arm.com>]

On 7/1/22 18:11, Yao, Jiewen wrote:
> 
> 
>> -----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.yao@intel.com>; devel@edk2.groups.io
>> Cc: Sami Mujawar <sami.mujawar@arm.com>; Leif Lindholm
>> <quic_llindhol@quicinc.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
>> Rebecca Cran <rebecca@bsdio.com>; Kinney, Michael D
>> <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>;
>> Edward Pickup <Edward.Pickup@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.

Yes right, there would be indeed no dependency between the MdePkg and ArmPkg,
the above case is perfectly correct.

> 
> 
> 
>>>
>>> (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.

To continue the discussion on one thread, please see the answer to:
https://edk2.groups.io/g/devel/message/91009

Regards,
Pierre

> 
> 
> 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.gondois@arm.com>
>>>> Sent: Friday, July 1, 2022 9:59 PM
>>>> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
>>>> Cc: Sami Mujawar <sami.mujawar@arm.com>; Leif Lindholm
>>>> <quic_llindhol@quicinc.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
>>>> Rebecca Cran <rebecca@bsdio.com>; Kinney, Michael D
>>>> <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>;
>>>> Edward Pickup <Edward.Pickup@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.gondois@arm.com>
>>>>>> Sent: Friday, July 1, 2022 5:49 PM
>>>>>> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
>>>>>> Cc: Sami Mujawar <sami.mujawar@arm.com>; Leif Lindholm
>>>>>> <quic_llindhol@quicinc.com>; Ard Biesheuvel
>> <ardb+tianocore@kernel.org>;
>>>>>> Rebecca Cran <rebecca@bsdio.com>; Kinney, Michael D
>>>>>> <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>;
>>>>>> Edward Pickup <Edward.Pickup@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.mujawar@arm.com>; Leif Lindholm
>>>>>>>> <quic_llindhol@quicinc.com>; Ard Biesheuvel
>>>> <ardb+tianocore@kernel.org>;
>>>>>>>> Rebecca Cran <rebecca@bsdio.com>; Kinney, Michael D
>>>>>>>> <michael.d.kinney@intel.com>; Gao, Liming
>> <gaoliming@byosoft.com.cn>;
>>>>>>>> Edward Pickup <Edward.Pickup@arm.com>
>>>>>>>> Subject: [edk2-devel] [PATCH RESEND v1 5/7] MdePkg/AesLib: Definition
>>>> for
>>>>>> AES
>>>>>>>> library class interface
>>>>>>>>
>>>>>>>> From: Pierre Gondois <Pierre.Gondois@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.gondois@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+owner@edk2.groups.io
>>>>>>>> Unsubscribe: https://edk2.groups.io/g/devel/unsub
>>>> [jiewen.yao@intel.com]
>>>>>>>> -=-=-=-=-=-=
>>>>>>>>
>>>>>>>
>>
>>
>> 
>>
>