Re: [Qemu-devel] [PATCH v4 00/10] Consolidate crypto APIs & implementations

[Gonglei <arei.gonglei@huawei.com>]

On 2015/7/7 18:03, Paolo Bonzini wrote:
> 
> 
> On 01/07/2015 19:10, Daniel P. Berrange wrote:
>> This small series covers the crypto consolidation patches
>> I previously posted:
>>
>> RFC: https://lists.nongnu.org/archive/html/qemu-devel/2015-04/msg02038.html
>>  v1: https://lists.nongnu.org/archive/html/qemu-devel/2015-05/msg04267.html
>>  v2: https://lists.nongnu.org/archive/html/qemu-devel/2015-06/msg00601.html
>>  v3: https://lists.nongnu.org/archive/html/qemu-devel/2015-06/msg05059.html
>>
>> Currently there are 5 main places in QEMU which use some
>> form of cryptographic hash or cipher algorithm. These are
>> the quorum block driver (hash), qcow{1,2} block driver (cipher),
>> VNC password auth (cipher), VNC websockets (hash) and some
>> of the CPU instruction emulation (cipher).
>>
>> For ciphers the code is using the in-tree implementations
>> of AES and/or the RFB cripple-DES. While there is nothing
>> broken about these implementations, it is none the less
>> desirable to be able to use the GNUTLS provided impls in
>> cases where we are already linking to GNUTLS. This will
>> allow QEMU to use FIPS certified implementations, which
>> have been well audited, have some protection against
>> side-channel leakage and are generally actively maintained
>> by people knowledgable about encryption.
>>
>> For hash digests the code is already using GNUTLS APIs.
>>
>> With the TLS work, and possible future improved block device
>> encryption, there will be more general purpose crypto APIs
>> needed in QEMU.
>>
>> It is undesirable to continue to litter the code with
>> countless #ifdef WITH_GNUTLS conditionals, as it makes
>> it increasingly hard to understand the code.
>>
>> The goal of this series is to thus consolidate all the
>> crypto code into a single logical place in QEMU - the
>> source in $GIT/crypto and heads in $GIT/include/crypto
>> The code in this location will provide QEMU internal
>> APIs for hash digests, ciphers, and later TLS and block
>> encryption primitives. The implementations will be
>> backed by GNUTLS, and either libgcrypt or nettle depending
>> on which of these GNUTLS is linking to. In the case where
>> GNUTLS is disabled at build time, we'll still keep the
>> built-in AES & RFB-cripple-DES implementations available
>> so we have no regression vs today's level of support.
>>
>> The callers of the crypto code can now be unconditionally
>> compiled and, if needed, they can check the availability
>> of algorithms they want at runtime and report clear errors
>> to the CLI or QMP if not available. This is a minor
>> difference in behaviour for the quorum block driver which
>> would previously be disabled at compile time if gnutls
>> was not available.
>>
>> A future posting will include the TLS crypto APIs.
>>
>> I have not attempted to convert the CPU emulation code to
>> use the new crypto APIs, since that code appears to have
>> quite specific need for access to the low level internal
>> stages of the AES algorithm. So I've left it using the
>> QEMU built-in AES code.
>>
>> I've added myself in the MAINTAINERS file for the new
>> directories, since it was't clear if anyone else on the
>> existing QEMU maintainer list had any interest / knowledge
>> in maintaining the crypto related pieces.
>>
>> Changes since v3:
>>
>>   - Removed need for crypto-internal.h file which was
>>     missing from v3 patches sent.
>>   - Resolve conflicts with error reporting & main loop
>>     API changes / cleanup on master
>>
>> Changes since v2:
>>
>>   - Remove _(..) gettext markers from error messages
>>   - Fix array bounds check in hash module (Richard Henderson)
>>   - Fix null dereference in freeing of gcrypt cipher impl
>>     (Gonglei)
>>
>> Changes since v1:
>>
>>   - Add explicit algorithm constants for each AES key size,
>>     instead of inferring it from array length
>>   - Share code for munging des rfb key bit order
>>   - Share code for validating key array size vs algorithm
>>   - Refactor built-in cipher impl to reduce number of big
>>     switch statements
>>   - Fix uninitialized 'Error *err' var
>>   - Add comments in places where error reporting should be
>>
>> Daniel P. Berrange (10):
>>   crypto: introduce new module for computing hash digests
>>   crypto: move built-in AES implementation into crypto/
>>   crypto: move built-in D3DES implementation into crypto/
>>   crypto: introduce generic cipher API & built-in implementation
>>   crypto: add a gcrypt cipher implementation
>>   crypto: add a nettle cipher implementation
>>   block: convert quorum blockdrv to use crypto APIs
>>   ui: convert VNC websockets to use crypto APIs
>>   block: convert qcow/qcow2 to use generic cipher API
>>   ui: convert VNC to use generic cipher API
>>
>>  MAINTAINERS                           |   7 +
>>  Makefile.objs                         |   1 +
>>  block/Makefile.objs                   |   2 +-
>>  block/qcow.c                          | 102 ++++++---
>>  block/qcow2-cluster.c                 |  46 +++-
>>  block/qcow2.c                         |  96 ++++----
>>  block/qcow2.h                         |  13 +-
>>  block/quorum.c                        |  41 ++--
>>  configure                             | 160 +++++++++-----
>>  crypto/Makefile.objs                  |   5 +
>>  {util => crypto}/aes.c                |   2 +-
>>  crypto/cipher-builtin.c               | 398 ++++++++++++++++++++++++++++++++++
>>  crypto/cipher-gcrypt.c                | 195 +++++++++++++++++
>>  crypto/cipher-nettle.c                | 206 ++++++++++++++++++
>>  crypto/cipher.c                       |  75 +++++++
>>  ui/d3des.c => crypto/desrfb.c         |   2 +-
>>  crypto/hash.c                         | 200 +++++++++++++++++
>>  crypto/init.c                         | 150 +++++++++++++
>>  include/{qemu => crypto}/aes.h        |   0
>>  include/crypto/cipher.h               | 210 ++++++++++++++++++
>>  ui/d3des.h => include/crypto/desrfb.h |   0
>>  include/crypto/hash.h                 | 189 ++++++++++++++++
>>  include/crypto/init.h                 |  29 +++
>>  target-arm/crypto_helper.c            |   2 +-
>>  target-i386/fpu_helper.c              |   1 -
>>  target-i386/ops_sse.h                 |   2 +-
>>  target-ppc/int_helper.c               |   2 +-
>>  tests/.gitignore                      |   2 +
>>  tests/Makefile                        |   4 +
>>  tests/test-crypto-cipher.c            | 290 +++++++++++++++++++++++++
>>  tests/test-crypto-hash.c              | 209 ++++++++++++++++++
>>  ui/Makefile.objs                      |   4 +-
>>  ui/vnc-ws.c                           |  22 +-
>>  ui/vnc-ws.h                           |   2 -
>>  ui/vnc.c                              | 119 +++++-----
>>  ui/vnc.h                              |   8 -
>>  util/Makefile.objs                    |   2 +-
>>  vl.c                                  |   7 +
>>  38 files changed, 2541 insertions(+), 264 deletions(-)
>>  create mode 100644 crypto/Makefile.objs
>>  rename {util => crypto}/aes.c (99%)
>>  create mode 100644 crypto/cipher-builtin.c
>>  create mode 100644 crypto/cipher-gcrypt.c
>>  create mode 100644 crypto/cipher-nettle.c
>>  create mode 100644 crypto/cipher.c
>>  rename ui/d3des.c => crypto/desrfb.c (99%)
>>  create mode 100644 crypto/hash.c
>>  create mode 100644 crypto/init.c
>>  rename include/{qemu => crypto}/aes.h (100%)
>>  create mode 100644 include/crypto/cipher.h
>>  rename ui/d3des.h => include/crypto/desrfb.h (100%)
>>  create mode 100644 include/crypto/hash.h
>>  create mode 100644 include/crypto/init.h
>>  create mode 100644 tests/test-crypto-cipher.c
>>  create mode 100644 tests/test-crypto-hash.c
>>
> 
> Since there were no more objections, I'm sending a pull request for this
> and assuming that further changes will be handled by you.
> 
> Paolo
> 
Good :)

For series:
Reviewed-by: Gonglei <arei.gonglei@huawei.com>

Regards,
-Gonglei