Re: [Qemu-devel] [PATCH 05/10] crypto: add a gcrypt cipher implementation

[Daniel P. Berrange]

On Fri, May 29, 2015 at 11:53:39AM +0800, Gonglei wrote:
> On 2015/5/21 18:56, Daniel P. Berrange wrote:
> > If we are linking to gnutls already and gnutls is built against
> > gcrypt, then we should use gcrypt as a cipher backend in
> > preference to our built-in backend.
> > 
> > This will be used when linking against GNUTLS 1.x and many
> > GNUTLS 2.x versions.
> > 
> > Signed-off-by: Daniel P. Berrange <address@hidden>


> > +QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg,
> > +                                  QCryptoCipherMode mode,
> > +                                  const uint8_t *key, size_t nkey,
> > +                                  Error **errp)
> > +{
> > +    QCryptoCipher *cipher;
> > +    gcry_cipher_hd_t handle;
> > +    gcry_error_t err;
> > +    int gcryalg, gcrymode;
> > +
> > +    switch (mode) {
> > +    case QCRYPTO_CIPHER_MODE_ECB:
> > +        gcrymode = GCRY_CIPHER_MODE_ECB;
> > +        break;
> > +    case QCRYPTO_CIPHER_MODE_CBC:
> > +        gcrymode = GCRY_CIPHER_MODE_CBC;
> > +        break;
> > +    default:
> > +        error_setg(errp, _("Unsupported cipher mode %d"), mode);
> > +        return NULL;
> > +    }
> > +
> > +    switch (alg) {
> > +    case QCRYPTO_CIPHER_ALG_DES_RFB:
> > +        gcryalg = GCRY_CIPHER_DES;
> > +        break;
> > +
> > +    case QCRYPTO_CIPHER_ALG_AES:
> > +        if (nkey == 16) {
> > +            gcryalg = GCRY_CIPHER_AES128;
> > +        } else if (nkey == 24) {
> > +            gcryalg = GCRY_CIPHER_AES192;
> > +        } else if (nkey == 32) {
> > +            gcryalg = GCRY_CIPHER_AES256;
> > +        } else {
> > +            error_setg(errp, _("Expected key size 16, 24 or 32 not %zu"),
> > +                       nkey);
> > +            return NULL;
> > +        }
> > +        break;
> > +
> > +    default:
> > +        error_setg(errp, _("Unsupported cipher algorithm %d"), alg);
> > +        return NULL;
> > +    }
> > +
> > +    cipher = g_new0(QCryptoCipher, 1);
> > +    cipher->alg = alg;
> > +    cipher->mode = mode;
> > +
> > +    err = gcry_cipher_open(&handle, gcryalg, gcrymode, 0);
> > +    if (err != 0) {
> > +        error_setg(errp, _("Cannot initialize cipher: %s"),
> > +                   gcry_strerror(err));
> > +        goto error;
> > +    }
> > +
> > +    if (cipher->alg == QCRYPTO_CIPHER_ALG_DES_RFB) {
> > +        /* We're using standard DES cipher from gcrypt, so we need
> > +         * to munge the key so that the results are the same as the
> > +         * bizarre RFB variant of DES :-)
> > +         */
> > +        uint8_t *rfbkey = qcrypto_cipher_munge_des_rfb_key(key, nkey);
> > +        err = gcry_cipher_setkey(handle, rfbkey, nkey);
> > +        g_free(rfbkey);
> > +    } else {
> > +        err = gcry_cipher_setkey(handle, key, nkey);
> > +    }
> > +    if (err != 0) {
> > +        error_setg(errp, _("Cannot set key: %s"),
> > +                   gcry_strerror(err));
> > +        goto error;
> > +    }
> > +
> > +    cipher->opaque = handle;
> > +    return cipher;
> > +
> > + error:
> > +    gcry_cipher_close(handle);
> > +    g_free(cipher);
> > +    return NULL;
> > +}
> > +
> > +
> > +void qcrypto_cipher_free(QCryptoCipher *cipher)
> > +{
> > +    if (!cipher) {
> > +        return;
> > +    }
> > +    gcry_cipher_close(cipher->opaque);
> 
> Maybe it's better cast cipher->opaque to gcry_cipher_hd_t like other free 
> functions.
> 
> gcry_cipher_hd_t handle = cipher->opaque;

Yes, that would make it a little clearer to follow.


> > +static struct gcry_thread_cbs qcrypto_gcrypt_thread_impl = {
> > +    (GCRY_THREAD_OPTION_PTHREAD | (GCRY_THREAD_OPTION_VERSION << 8)),
> > +    NULL,
> > +    qcrypto_gcrypt_mutex_init,
> > +    qcrypto_gcrypt_mutex_destroy,
> > +    qcrypto_gcrypt_mutex_lock,
> > +    qcrypto_gcrypt_mutex_unlock,
> > +    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL
> > +};
> > +#endif /* QCRYPTO_INIT_GCRYPT */
> > +
> >  int qcrypto_init(Error **errp)
> >  {
> >      int ret;
> > @@ -49,6 +127,18 @@ int qcrypto_init(Error **errp)
> >      gnutls_global_set_log_level(10);
> >      gnutls_global_set_log_function(qcrypto_gnutls_log);
> >  #endif
> > +
> > +#ifdef CONFIG_GNUTLS_GCRYPT
> > +    if (!gcry_check_version(GCRYPT_VERSION)) {
> > +        error_setg(errp, "%s", _("Unable to initialize gcrypt"));
> > +        return -1;
> 
> Missing to call gnutls_global_deinit().

The gnutls_global_init/deinit functions are not threadsafe, so
although it would be safe todo in this case, as a general rule
it is preferrable to avoid ever using the gnutls_global_deinit()
function in threaded apps, as you never know what background
threads are using gnutls.

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