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

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

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]
>>>>>> -=-=-=-=-=-=
>>>>>>
>>>>>