| Message ID | 1480928380-161760-2-git-send-email-longpeng2@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Dec 05, 2016 at 04:59:38PM +0800, Longpeng(Mike) wrote: > This patch add standart DES support. > > Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com> > --- > crypto/cipher-gcrypt.c | 3 +++ > crypto/cipher-nettle.c | 13 ++++++++++--- > crypto/cipher.c | 5 ++++- > qapi/crypto.json | 2 +- > 4 files changed, 18 insertions(+), 5 deletions(-) > > diff --git a/crypto/cipher-gcrypt.c b/crypto/cipher-gcrypt.c > index c550db9..7ca049c 100644 > --- a/crypto/cipher-gcrypt.c > +++ b/crypto/cipher-gcrypt.c > @@ -28,6 +28,7 @@ bool qcrypto_cipher_supports(QCryptoCipherAlgorithm alg, > QCryptoCipherMode mode) > { > 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: > @@ -95,6 +96,7 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg, > } > > switch (alg) { > + case QCRYPTO_CIPHER_ALG_DES: > case QCRYPTO_CIPHER_ALG_DES_RFB: > gcryalg = GCRY_CIPHER_DES; > break; > @@ -200,6 +202,7 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg, > case QCRYPTO_CIPHER_ALG_TWOFISH_256: > ctx->blocksize = 16; > break; > + case QCRYPTO_CIPHER_ALG_DES: > case QCRYPTO_CIPHER_ALG_CAST5_128: > ctx->blocksize = 8; > break; > diff --git a/crypto/cipher-nettle.c b/crypto/cipher-nettle.c > index cd094cd..81cc634 100644 > --- a/crypto/cipher-nettle.c > +++ b/crypto/cipher-nettle.c > @@ -196,6 +196,7 @@ bool qcrypto_cipher_supports(QCryptoCipherAlgorithm alg, > QCryptoCipherMode mode) > { > 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) > 'cast5-128', > 'serpent-128', 'serpent-192', 'serpent-256', > 'twofish-128', 'twofish-192', 'twofish-256']} Regards, Daniel
> > > 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. :) Regards, -Gonglei
Hi Daniel, On 2016/12/5 17:18, Daniel P. Berrange wrote: > On Mon, Dec 05, 2016 at 04:59:38PM +0800, Longpeng(Mike) wrote: ...... >> 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) > As the comment in qapi/crypto.json said: @des-rfb: RFB specific variant of single DES. This patch just add the standard single-DES support, not the triple-DES, so I think maybe "des" is suitable. And we will add "3des" in the near-future. >> 'cast5-128', >> 'serpent-128', 'serpent-192', 'serpent-256', >> 'twofish-128', 'twofish-192', 'twofish-256']} > > Regards, > Daniel
On Mon, Dec 05, 2016 at 07:11:37PM +0800, Longpeng (Mike) wrote: > Hi Daniel, > > On 2016/12/5 17:18, Daniel P. Berrange wrote: > > > On Mon, Dec 05, 2016 at 04:59:38PM +0800, Longpeng(Mike) wrote: > ...... > >> 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) > > > > As the comment in qapi/crypto.json said: > @des-rfb: RFB specific variant of single DES. > > This patch just add the standard single-DES support, not the triple-DES, so I > think maybe "des" is suitable. Oh I missed that - QEMU should not support single-DES at all for cryptodev IMHO. Single DES has been cryptographically broken/useless for *decades* - way back in 1999, the EFF built a machine that could brute force single-DES in a mere 56 hours. Triple-DES is the bare minimum that's acceptable and even that should only be for legacy usage which can't use a more modern cipher like AES Regards, Daniel
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 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 Regards, Daniel
On 12/05/2016 02:59 AM, Longpeng(Mike) wrote: > This patch add standart DES support. s/standart/standard/ > > Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com> > --- > +++ 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', Missing documentation that includes a (since 2.9) blurb > 'cast5-128', > 'serpent-128', 'serpent-192', 'serpent-256', > 'twofish-128', 'twofish-192', 'twofish-256']} >
> > From: Daniel P. Berrange [mailto:berrange@redhat.com] > Sent: Tuesday, December 06, 2016 12:59 AM > To: Gonglei (Arei) > Cc: longpeng; eblake@redhat.com; armbru@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. 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
On Tue, Dec 06, 2016 at 01:23:31AM +0000, Gonglei (Arei) wrote: > > > > From: Daniel P. Berrange [mailto:berrange@redhat.com] > > Sent: Tuesday, December 06, 2016 12:59 AM > > To: Gonglei (Arei) > > Cc: longpeng; eblake@redhat.com; armbru@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
> > > > > > > } > > > > > > 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. > Make sense. And I don't want to support single-DES in the virtio-crypto frontend driver as well. The guest will use the software realization. Thanks, -Gonglei
Hi Eric, On 2016/12/6 3:15, Eric Blake wrote: > On 12/05/2016 02:59 AM, Longpeng(Mike) wrote: >> This patch add standart DES support. > > s/standart/standard/ > >> >> Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com> >> --- > >> +++ 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', > > Missing documentation that includes a (since 2.9) blurb > Thanks, but I decide to disable standard single-DES in cryptodev, as suggested by Daniel. >> 'cast5-128', >> 'serpent-128', 'serpent-192', 'serpent-256', >> 'twofish-128', 'twofish-192', 'twofish-256']} >> >
diff --git a/crypto/cipher-gcrypt.c b/crypto/cipher-gcrypt.c index c550db9..7ca049c 100644 --- a/crypto/cipher-gcrypt.c +++ b/crypto/cipher-gcrypt.c @@ -28,6 +28,7 @@ bool qcrypto_cipher_supports(QCryptoCipherAlgorithm alg, QCryptoCipherMode mode) { 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: @@ -95,6 +96,7 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg, } switch (alg) { + case QCRYPTO_CIPHER_ALG_DES: case QCRYPTO_CIPHER_ALG_DES_RFB: gcryalg = GCRY_CIPHER_DES; break; @@ -200,6 +202,7 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg, case QCRYPTO_CIPHER_ALG_TWOFISH_256: ctx->blocksize = 16; break; + case QCRYPTO_CIPHER_ALG_DES: case QCRYPTO_CIPHER_ALG_CAST5_128: ctx->blocksize = 8; break; diff --git a/crypto/cipher-nettle.c b/crypto/cipher-nettle.c index cd094cd..81cc634 100644 --- a/crypto/cipher-nettle.c +++ b/crypto/cipher-nettle.c @@ -196,6 +196,7 @@ bool qcrypto_cipher_supports(QCryptoCipherAlgorithm alg, QCryptoCipherMode mode) { 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', 'cast5-128', 'serpent-128', 'serpent-192', 'serpent-256', 'twofish-128', 'twofish-192', 'twofish-256']}
This patch add standart DES support. Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com> --- crypto/cipher-gcrypt.c | 3 +++ crypto/cipher-nettle.c | 13 ++++++++++--- crypto/cipher.c | 5 ++++- qapi/crypto.json | 2 +- 4 files changed, 18 insertions(+), 5 deletions(-)