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

["Yao, Jiewen" <jiewen.yao@intel.com>]

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



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