On Tue, Dec 06, 2016 at 01:23:31AM +0000, Gonglei (Arei) wrote: > > > > From: Daniel P. Berrange [mailto:berra...@redhat.com] > > Sent: Tuesday, December 06, 2016 12:59 AM > > To: Gonglei (Arei) > > Cc: longpeng; ebl...@redhat.com; arm...@redhat.com; > > qemu-devel@nongnu.org; Wubin (H); Zhoujian (jay, Euler) > > Subject: Re: [PATCH for-2.9 1/3] crypto: add standard des support > > > > On Mon, Dec 05, 2016 at 09:29:59AM +0000, Gonglei (Arei) wrote: > > > > > > > > > switch (alg) { > > > > > + case QCRYPTO_CIPHER_ALG_DES: > > > > > case QCRYPTO_CIPHER_ALG_DES_RFB: > > > > > case QCRYPTO_CIPHER_ALG_AES_128: > > > > > case QCRYPTO_CIPHER_ALG_AES_192: > > > > > @@ -256,11 +257,17 @@ QCryptoCipher > > > > *qcrypto_cipher_new(QCryptoCipherAlgorithm alg, > > > > > ctx = g_new0(QCryptoCipherNettle, 1); > > > > > > > > > > switch (alg) { > > > > > + case QCRYPTO_CIPHER_ALG_DES: > > > > > case QCRYPTO_CIPHER_ALG_DES_RFB: > > > > > ctx->ctx = g_new0(struct des_ctx, 1); > > > > > - rfbkey = qcrypto_cipher_munge_des_rfb_key(key, nkey); > > > > > - des_set_key(ctx->ctx, rfbkey); > > > > > - g_free(rfbkey); > > > > > + > > > > > + if (alg == QCRYPTO_CIPHER_ALG_DES_RFB) { > > > > > + rfbkey = qcrypto_cipher_munge_des_rfb_key(key, nkey); > > > > > + des_set_key(ctx->ctx, rfbkey); > > > > > + g_free(rfbkey); > > > > > + } else { > > > > > + des_set_key(ctx->ctx, key); > > > > > + } > > > > > > > > > > ctx->alg_encrypt_native = des_encrypt_native; > > > > > ctx->alg_decrypt_native = des_decrypt_native; > > > > > diff --git a/crypto/cipher.c b/crypto/cipher.c > > > > > index a9bca41..00d9682 100644 > > > > > --- a/crypto/cipher.c > > > > > +++ b/crypto/cipher.c > > > > > @@ -27,6 +27,7 @@ static size_t > > > > alg_key_len[QCRYPTO_CIPHER_ALG__MAX] = { > > > > > [QCRYPTO_CIPHER_ALG_AES_128] = 16, > > > > > [QCRYPTO_CIPHER_ALG_AES_192] = 24, > > > > > [QCRYPTO_CIPHER_ALG_AES_256] = 32, > > > > > + [QCRYPTO_CIPHER_ALG_DES] = 8, > > > > > [QCRYPTO_CIPHER_ALG_DES_RFB] = 8, > > > > > [QCRYPTO_CIPHER_ALG_CAST5_128] = 16, > > > > > [QCRYPTO_CIPHER_ALG_SERPENT_128] = 16, > > > > > @@ -41,6 +42,7 @@ static size_t > > > > alg_block_len[QCRYPTO_CIPHER_ALG__MAX] = { > > > > > [QCRYPTO_CIPHER_ALG_AES_128] = 16, > > > > > [QCRYPTO_CIPHER_ALG_AES_192] = 16, > > > > > [QCRYPTO_CIPHER_ALG_AES_256] = 16, > > > > > + [QCRYPTO_CIPHER_ALG_DES] = 8, > > > > > [QCRYPTO_CIPHER_ALG_DES_RFB] = 8, > > > > > [QCRYPTO_CIPHER_ALG_CAST5_128] = 8, > > > > > [QCRYPTO_CIPHER_ALG_SERPENT_128] = 16, > > > > > @@ -107,7 +109,8 @@ > > > > qcrypto_cipher_validate_key_length(QCryptoCipherAlgorithm alg, > > > > > } > > > > > > > > > > if (mode == QCRYPTO_CIPHER_MODE_XTS) { > > > > > - if (alg == QCRYPTO_CIPHER_ALG_DES_RFB) { > > > > > + if (alg == QCRYPTO_CIPHER_ALG_DES_RFB > > > > > + || alg == QCRYPTO_CIPHER_ALG_DES) { > > > > > error_setg(errp, "XTS mode not compatible with > > DES-RFB"); > > > > > return false; > > > > > } > > > > > diff --git a/qapi/crypto.json b/qapi/crypto.json > > > > > index 5c9d7d4..d403ab9 100644 > > > > > --- a/qapi/crypto.json > > > > > +++ b/qapi/crypto.json > > > > > @@ -75,7 +75,7 @@ > > > > > { 'enum': 'QCryptoCipherAlgorithm', > > > > > 'prefix': 'QCRYPTO_CIPHER_ALG', > > > > > 'data': ['aes-128', 'aes-192', 'aes-256', > > > > > - 'des-rfb', > > > > > + 'des-rfb', 'des', > > > > > > > > Can we call this '3des' to make it clear that this is Triple-DES and not > > > > the single-DES (which des-rfb is) > > > > > > > Actually the current des is not triple-DES, just the single-DES, and > > > des-rfb in > > QEMU is just a variant of > > > single DES, which change the standard key by calling > > qcrypto_cipher_munge_des_rfb_key(). > > > > > > I think we can add the 3des support as well in the next step. > > > > > > The current single-DES in the patch set is ok to me. :) > > > > Per my othre reply in this thread, > > I saw that, thanks for your information, Daniel. > > > I don't think we should be supporting > > single-DES at all in QEMU / cryptodev. So IMHO, the correct fix is to > > remove the single-DES support from cryptodev entirely > > > The cryptodev-builtin is one kind of cryptodev backends. It provides the > real crypto capability for virtio crypto device. > > I don't think we should artificially remove one algorithm support if > the frontend driver (users) wants to use it, though the algorithm is > unsafe.
IIUC the cryptodev hardware is ultimately about allowing the guest to offload crypto operations to the host, potentialy using hardware acceleration. If the cryptodev backend doesn't support a particular algorithm, the guest is still capable of using its own built-in support for that algorithm. I see no compelling reason to provide host offload / acceleration for single-DES. Just kill this obsolete algorithm from cryptodev and in the unlikely event that a guest really does want single-DES it can use its built-in impl instead. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|