qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH for-2.9 1/3] crypto: add standard des support


From: Gonglei (Arei)
Subject: Re: [Qemu-devel] [PATCH for-2.9 1/3] crypto: add standard des support
Date: Tue, 6 Dec 2016 01:23:31 +0000

>
> From: Daniel P. Berrange [mailto:address@hidden
> Sent: Tuesday, December 06, 2016 12:59 AM
> To: Gonglei (Arei)
> Cc: longpeng; address@hidden; address@hidden;
> address@hidden; 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.

Of course, if the QEMU crypto API doesn't provide single-DES support,
we should remove it from the cryptodev-builtin backend as well. ;)

Regards,
-Gonglei

reply via email to

[Prev in Thread] Current Thread [Next in Thread]