Re: [Qemu-devel] [PATCH v1 RFC 08/34] crypto: introduce new module for computing hash digests

["Daniel P. Berrange" <berrange@redhat.com>]

On Fri, Apr 17, 2015 at 03:22:11PM +0100, Daniel P. Berrange wrote:
> Introduce a new crypto/ directory that will (eventually) contain
> all the cryptographic related code. This initially defines a
> wrapper for initializing gnutls and for computing hashes with
> gnutls. The former ensures that gnutls is guaranteed to be
> initialized exactly once in QEMU regardless of CLI args. The
> block quorum code currently fails to initialize gnutls so it
> only works by luck, if VNC server TLS is not requested. The
> hash APIs avoids the need to litter the rest of the code with
> preprocessor checks and simplifies callers by allocating the
> correct amount of memory for the requested hash.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  Makefile.objs            |   1 +
>  configure                |  46 +++++++++++
>  crypto/Makefile.objs     |   2 +
>  crypto/hash.c            | 202 +++++++++++++++++++++++++++++++++++++++++++++
>  crypto/init.c            |  62 ++++++++++++++
>  include/crypto/hash.h    | 189 ++++++++++++++++++++++++++++++++++++++++++
>  include/crypto/init.h    |  29 +++++++
>  tests/.gitignore         |   1 +
>  tests/Makefile           |   2 +
>  tests/test-crypto-hash.c | 209 +++++++++++++++++++++++++++++++++++++++++++++++
>  vl.c                     |   8 ++
>  11 files changed, 751 insertions(+)

> diff --git a/crypto/init.c b/crypto/init.c
> new file mode 100644
> index 0000000..8fd66d4
> --- /dev/null
> +++ b/crypto/init.c

> +
> +#include "crypto/init.h"
> +
> +#include <glib/gi18n.h>
> +
> +#ifdef CONFIG_GNUTLS
> +#include <gnutls/gnutls.h>
> +#include <gnutls/crypto.h>
> +
> +/* #define DEBUG_GNUTLS */
> +
> +#ifdef DEBUG_GNUTLS
> +static void qcrypto_gnutls_log(int level, const char *str)
> +{
> +    fprintf(stderr, "%d: %s", level, str);
> +}
> +#endif
> +
> +int qcrypto_init(Error **errp)
> +{
> +    int ret;
> +    ret = gnutls_global_init();
> +    if (ret < 0) {
> +        error_setg(errp,
> +                   _("Unable to initialize GNUTLS library: %s"),
> +                   gnutls_strerror(ret));
> +        return -1;
> +    }
> +#ifdef DEBUG_GNUTLS
> +    gnutls_global_set_log_level(10);
> +    gnutls_global_set_log_function(qcrypto_gnutls_log);
> +#endif
> +    return 0;
> +}
> +
> +#else /* ! CONFIG_GNUTLS */
> +
> +int qcrypto_init(Error **errp G_GNUC_UNUSED)
> +{
> +    return 0;
> +}
> +
> +#endif /* ! CONFIG_GNUTLS */

[snip]

> diff --git a/vl.c b/vl.c
> index 8aac4ee..6902253 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -119,6 +119,7 @@ int main(int argc, char **argv)
>  #include "qapi/opts-visitor.h"
>  #include "qom/object_interfaces.h"
>  #include "qapi-event.h"
> +#include "crypto/init.h"
>  
>  #define DEFAULT_RAM_SIZE 128
>  
> @@ -2787,6 +2788,7 @@ int main(int argc, char **argv, char **envp)
>      uint64_t ram_slots = 0;
>      FILE *vmstate_dump_file = NULL;
>      Error *main_loop_err = NULL;
> +    Error *err = NULL;
>  
>      qemu_init_cpu_loop();
>      qemu_mutex_lock_iothread();
> @@ -2829,6 +2831,12 @@ int main(int argc, char **argv, char **envp)
>  
>      runstate_init();
>  
> +    if (qcrypto_init(&err) < 0) {
> +        fprintf(stderr, "Cannot initialize crypto: %s\n",
> +                error_get_pretty(err));
> +        error_free(err);
> +        exit(1);
> +    }
>      rtc_clock = QEMU_CLOCK_HOST;
>  
>      QLIST_INIT (&vm_change_state_head);

I created the 'qcrypto_init()' method in this patch so that we have
a single clear place to initialize gnutls - currently we are doing
initialization only in VNC when TLS is requested. This won't fly
when more areas of the code will use GNUTLS APIs.

It occurs to me though that this patch is either wrong or incomplete,
as I'm only calling qcrypto_init() from vl.c. I would also need to
ensure it is invoked from qemu-io, qemu-img, qemu-nbd, and all the
test suite programs that use libqemutil.la too.

I'm thinking perhaps a better approach could be for the crypto related
APIs to call qcrypto_init() on an as-needed basis. The downside would
be that this could delay the point at which the user sees a gnutls
initialization failure to only after QEMU has been running for a while,
instead of being upfront at startup. The plus side is obviously that
we'd not need to update every binary program main() method.

I notice though that QEMU does not make use of pthread_once() for
global initializers. Is there any particular reason for this ? With
this crypto code it is not safe to rely on being single threaded,
since the crypto code can be invoked from I/O threads as well as
the main event loop. So ideally I would use a pthread_once() instead
of having a static 'bool is_initialized' protected by a pthread_mutex

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|