Re: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation for CryptAes.c


Zurcher, Christopher J
 

The HMAC functions do not need AES; my point was that the HMAC functions as we have them today are already a wrapper for the EVP interface (this is a function of OpenSSL that we cannot change). So if a module already includes the additional ~192KB EVP interface through the HMAC functions, it would not see any size hit by also including the updated AES interface.

The speed improvement for the AES functions are not intended to improve boot speed for most standard platforms. In fact most platforms today aren't even calling any AES functions.
The only reason for this patch is to satisfy BZ 2507, which was filed by Eugene at HP (and now owned by Bin) for what I assume is a platform-specific need for improved AES speed. I was already working on my other patch to enable the architecture-specific algorithms for SHA speed improvement (required for a Windows provisioning feature) and that patch was also satisfying most of the needs of BZ 2507, with the exception of the file in this patch which provides the path for AES to access the architecture-specific algorithms.

Thanks,
Christopher Zurcher

-----Original Message-----
From: Ard Biesheuvel <ard.biesheuvel@arm.com>
Sent: Monday, November 2, 2020 23:51
To: Zurcher, Christopher J <christopher.j.zurcher@intel.com>; Yao, Jiewen
<jiewen.yao@intel.com>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>;
Jiang, Guomin <guomin.jiang@intel.com>; Sung-Uk Bin <sunguk-bin@hp.com>;
Laszlo Ersek <lersek@redhat.com>
Subject: Re: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation for
CryptAes.c

On 11/2/20 11:36 PM, Zurcher, Christopher J wrote:
The size increase from adding the EVP interface to a module that does not
already include it (through the HMAC functions) is ~192KB.

Which HMAC function do we use in UEFI that needs AES? Adding 192 KB for
AES to each module that only uses HMAC-SHAxxx seems like a really bad
idea to me. Maybe the IEEE 802.11 drivers have some dependencies on
CBC-MAC for CCMP, but I don't think any wifi hardware exists today that
relies on software support for this, so using optimized code here makes
little sense.

Also, how does the 32% to 47% speed improvement translate to actual boot
speed improvements? A lot of the crypto is only applied to small
quantities of data, and only the algorithms that are applied to
arbitrary size chunks should be optimized in this way, imo.

Note that, while widely regarded as the fastest, the OpenSSL asm
implementations are not as robust as you might think, and they don't see
a lot of test coverage running in a bare metal context with elevated
privileges like we do in EDK2.

(I may have brought up some of these points before - apologies if
anything I say sounds redundant).



Intel documentation on the IA version of the feature lists speed
improvement of ~32% to ~47% depending on the operation and key size. Other
architectures may see different speed improvements. I have only tested this
file with OvmfPkg through QEMU.

I did not add this .c file to any INF file because it will add ~192KB to
any module that includes AES functionality and it should be up to the end
user if they want to include this file for the size tradeoff vs. the speed
gain for their particular architecture. Additionally as the only native
OpensslLib implementation available currently is for X64 architecture, any
other architecture would have a size increase with no speed improvement.

Thanks,
Christopher Zurcher

-----Original Message-----
From: Yao, Jiewen <jiewen.yao@intel.com>
Sent: Friday, October 30, 2020 18:10
To: Zurcher, Christopher J <christopher.j.zurcher@intel.com>;
devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
<xiaoyux.lu@intel.com>;
Jiang, Guomin <guomin.jiang@intel.com>; Sung-Uk Bin <sunguk-bin@hp.com>;
Ard
Biesheuvel <ard.biesheuvel@arm.com>
Subject: RE: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation
for
CryptAes.c

HI Zucher
I am not sure why you only add a .c file, but do not add this c to any INF
file. This seems a dummy C file.
I recommend you drop this and create a full patch to add C and update INF
file.

Since you are talking performance and size, do you have any data?
For example, how fast you have got? What is the size difference before and
after?
This can help other people make decision to choose which version.


Thank you
Yao Jiewen


-----Original Message-----
From: Christopher J Zurcher <christopher.j.zurcher@intel.com>
Sent: Thursday, October 29, 2020 2:43 AM
To: devel@edk2.groups.io
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
<jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>; Jiang,
Guomin
<guomin.jiang@intel.com>; Sung-Uk Bin <sunguk-bin@hp.com>; Ard
Biesheuvel <ard.biesheuvel@arm.com>
Subject: [PATCH 0/1] CryptoPkg/BaseCryptLib: Add EVP implementation for
CryptAes.c

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2507

This patch provides a drop-in replacement for CryptAes.c which utilizes
the EVP interface to OpenSSL. This enables access to the assembly-
optimized
algorithms.

This patch has been unit-tested in both VS and CLANG build environments
using both an X64 assembly-optimized implementation of OpensslLib and
the
standard implementation.

Usage of this file does not require an assembly-optimized implementation
of
OpensslLib to function; it does however require one to provide a speed
improvement.

The C-only AES implementation included by CryptAes.c is extremely small,
and since this file includes the EVP interface, it will significantly
increase the size of any module that includes it. As a result, I have not
replaced the original CryptAes.c as a default in any of the CryptLib
implementations.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
Cc: Guomin Jiang <guomin.jiang@intel.com>
Cc: Sung-Uk Bin <sunguk-bin@hp.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>

Christopher J Zurcher (1):
CryptoPkg/BaseCryptLib: Add EVP implementation for CryptAes.c

CryptoPkg/Library/BaseCryptLib/Cipher/CryptAesEvp.c | 262
++++++++++++++++++++
1 file changed, 262 insertions(+)
create mode 100644 CryptoPkg/Library/BaseCryptLib/Cipher/CryptAesEvp.c

--
2.28.0.windows.1

Join devel@edk2.groups.io to automatically receive all group messages.