qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] block/crypto: do not require number of threads upfront
@ 2024-05-27 15:58 Stefan Hajnoczi
  2024-05-27 15:58 ` [PATCH 1/2] block/crypto: create ciphers on demand Stefan Hajnoczi
                   ` (4 more replies)
  0 siblings, 5 replies; 7+ messages in thread
From: Stefan Hajnoczi @ 2024-05-27 15:58 UTC (permalink / raw)
  To: qemu-devel
  Cc: Hanna Reitz, qemu-block, Kevin Wolf, Daniel P. Berrangé,
	Stefan Hajnoczi

The block layer does not know how many threads will perform I/O. It is possible
to exceed the number of threads that is given to qcrypto_block_open() and this
can trigger an assertion failure in qcrypto_block_pop_cipher().

This patch series removes the n_threads argument and instead handles an
arbitrary number of threads.
---
Is it secure to store the key in QCryptoBlock? In this series I assumed the
answer is yes since the QCryptoBlock's cipher state is equally sensitive, but
I'm not familiar with this code or a crypto expert.

Stefan Hajnoczi (2):
  block/crypto: create ciphers on demand
  crypto/block: drop qcrypto_block_open() n_threads argument

 crypto/blockpriv.h             |  13 ++--
 include/crypto/block.h         |   2 -
 block/crypto.c                 |   1 -
 block/qcow.c                   |   2 +-
 block/qcow2.c                  |   5 +-
 crypto/block-luks.c            |   4 +-
 crypto/block-qcow.c            |   8 +--
 crypto/block.c                 | 116 ++++++++++++++++++++-------------
 tests/unit/test-crypto-block.c |   4 --
 9 files changed, 85 insertions(+), 70 deletions(-)

-- 
2.45.1



^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/2] block/crypto: create ciphers on demand
  2024-05-27 15:58 [PATCH 0/2] block/crypto: do not require number of threads upfront Stefan Hajnoczi
@ 2024-05-27 15:58 ` Stefan Hajnoczi
  2024-05-27 15:58 ` [PATCH 2/2] crypto/block: drop qcrypto_block_open() n_threads argument Stefan Hajnoczi
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 7+ messages in thread
From: Stefan Hajnoczi @ 2024-05-27 15:58 UTC (permalink / raw)
  To: qemu-devel
  Cc: Hanna Reitz, qemu-block, Kevin Wolf, Daniel P. Berrangé,
	Stefan Hajnoczi, Qing Wang

Ciphers are pre-allocated by qcrypto_block_init_cipher() depending on
the given number of threads. The -device
virtio-blk-pci,iothread-vq-mapping= feature allows users to assign
multiple IOThreads to a virtio-blk device, but the association between
the virtio-blk device and the block driver happens after the block
driver is already open.

When the number of threads given to qcrypto_block_init_cipher() is
smaller than the actual number of threads at runtime, the
block->n_free_ciphers > 0 assertion in qcrypto_block_pop_cipher() can
fail.

Get rid of qcrypto_block_init_cipher() n_thread's argument and allocate
ciphers on demand.

Reported-by: Qing Wang <qinwang@redhat.com>
Buglink: https://issues.redhat.com/browse/RHEL-36159
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 crypto/blockpriv.h  |  12 +++--
 crypto/block-luks.c |   3 +-
 crypto/block-qcow.c |   2 +-
 crypto/block.c      | 113 ++++++++++++++++++++++++++------------------
 4 files changed, 79 insertions(+), 51 deletions(-)

diff --git a/crypto/blockpriv.h b/crypto/blockpriv.h
index 836f3b4726..4bf6043d5d 100644
--- a/crypto/blockpriv.h
+++ b/crypto/blockpriv.h
@@ -32,8 +32,14 @@ struct QCryptoBlock {
     const QCryptoBlockDriver *driver;
     void *opaque;
 
-    QCryptoCipher **ciphers;
-    size_t n_ciphers;
+    /* Cipher parameters */
+    QCryptoCipherAlgorithm alg;
+    QCryptoCipherMode mode;
+    uint8_t *key;
+    size_t nkey;
+
+    QCryptoCipher **free_ciphers;
+    size_t max_free_ciphers;
     size_t n_free_ciphers;
     QCryptoIVGen *ivgen;
     QemuMutex mutex;
@@ -130,7 +136,7 @@ int qcrypto_block_init_cipher(QCryptoBlock *block,
                               QCryptoCipherAlgorithm alg,
                               QCryptoCipherMode mode,
                               const uint8_t *key, size_t nkey,
-                              size_t n_threads, Error **errp);
+                              Error **errp);
 
 void qcrypto_block_free_cipher(QCryptoBlock *block);
 
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 3ee928fb5a..3357852c0a 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -1262,7 +1262,6 @@ qcrypto_block_luks_open(QCryptoBlock *block,
                                       luks->cipher_mode,
                                       masterkey,
                                       luks->header.master_key_len,
-                                      n_threads,
                                       errp) < 0) {
             goto fail;
         }
@@ -1456,7 +1455,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
     /* Setup the block device payload encryption objects */
     if (qcrypto_block_init_cipher(block, luks_opts.cipher_alg,
                                   luks_opts.cipher_mode, masterkey,
-                                  luks->header.master_key_len, 1, errp) < 0) {
+                                  luks->header.master_key_len, errp) < 0) {
         goto error;
     }
 
diff --git a/crypto/block-qcow.c b/crypto/block-qcow.c
index 4d7cf36a8f..02305058e3 100644
--- a/crypto/block-qcow.c
+++ b/crypto/block-qcow.c
@@ -75,7 +75,7 @@ qcrypto_block_qcow_init(QCryptoBlock *block,
     ret = qcrypto_block_init_cipher(block, QCRYPTO_CIPHER_ALG_AES_128,
                                     QCRYPTO_CIPHER_MODE_CBC,
                                     keybuf, G_N_ELEMENTS(keybuf),
-                                    n_threads, errp);
+                                    errp);
     if (ret < 0) {
         ret = -ENOTSUP;
         goto fail;
diff --git a/crypto/block.c b/crypto/block.c
index 506ea1d1a3..ba6d1cebc7 100644
--- a/crypto/block.c
+++ b/crypto/block.c
@@ -20,6 +20,7 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qemu/lockable.h"
 #include "blockpriv.h"
 #include "block-qcow.h"
 #include "block-luks.h"
@@ -57,6 +58,8 @@ QCryptoBlock *qcrypto_block_open(QCryptoBlockOpenOptions *options,
 {
     QCryptoBlock *block = g_new0(QCryptoBlock, 1);
 
+    qemu_mutex_init(&block->mutex);
+
     block->format = options->format;
 
     if (options->format >= G_N_ELEMENTS(qcrypto_block_drivers) ||
@@ -76,8 +79,6 @@ QCryptoBlock *qcrypto_block_open(QCryptoBlockOpenOptions *options,
         return NULL;
     }
 
-    qemu_mutex_init(&block->mutex);
-
     return block;
 }
 
@@ -92,6 +93,8 @@ QCryptoBlock *qcrypto_block_create(QCryptoBlockCreateOptions *options,
 {
     QCryptoBlock *block = g_new0(QCryptoBlock, 1);
 
+    qemu_mutex_init(&block->mutex);
+
     block->format = options->format;
 
     if (options->format >= G_N_ELEMENTS(qcrypto_block_drivers) ||
@@ -111,8 +114,6 @@ QCryptoBlock *qcrypto_block_create(QCryptoBlockCreateOptions *options,
         return NULL;
     }
 
-    qemu_mutex_init(&block->mutex);
-
     return block;
 }
 
@@ -227,37 +228,42 @@ QCryptoCipher *qcrypto_block_get_cipher(QCryptoBlock *block)
      * This function is used only in test with one thread (it's safe to skip
      * pop/push interface), so it's enough to assert it here:
      */
-    assert(block->n_ciphers <= 1);
-    return block->ciphers ? block->ciphers[0] : NULL;
+    assert(block->max_free_ciphers <= 1);
+    return block->free_ciphers ? block->free_ciphers[0] : NULL;
 }
 
 
-static QCryptoCipher *qcrypto_block_pop_cipher(QCryptoBlock *block)
+static QCryptoCipher *qcrypto_block_pop_cipher(QCryptoBlock *block,
+                                               Error **errp)
 {
-    QCryptoCipher *cipher;
-
-    qemu_mutex_lock(&block->mutex);
-
-    assert(block->n_free_ciphers > 0);
-    block->n_free_ciphers--;
-    cipher = block->ciphers[block->n_free_ciphers];
-
-    qemu_mutex_unlock(&block->mutex);
-
-    return cipher;
+    /* Usually there is a free cipher available */
+    WITH_QEMU_LOCK_GUARD(&block->mutex) {
+        if (block->n_free_ciphers > 0) {
+            block->n_free_ciphers--;
+            return block->free_ciphers[block->n_free_ciphers];
+        }
+    }
+
+    /* Otherwise allocate a new cipher */
+    return qcrypto_cipher_new(block->alg, block->mode, block->key,
+                              block->nkey, errp);
 }
 
 
 static void qcrypto_block_push_cipher(QCryptoBlock *block,
                                       QCryptoCipher *cipher)
 {
-    qemu_mutex_lock(&block->mutex);
+    QEMU_LOCK_GUARD(&block->mutex);
 
-    assert(block->n_free_ciphers < block->n_ciphers);
-    block->ciphers[block->n_free_ciphers] = cipher;
+    if (block->n_free_ciphers == block->max_free_ciphers) {
+        block->max_free_ciphers++;
+        block->free_ciphers = g_renew(QCryptoCipher *,
+                                      block->free_ciphers,
+                                      block->max_free_ciphers);
+    }
+
+    block->free_ciphers[block->n_free_ciphers] = cipher;
     block->n_free_ciphers++;
-
-    qemu_mutex_unlock(&block->mutex);
 }
 
 
@@ -265,24 +271,31 @@ int qcrypto_block_init_cipher(QCryptoBlock *block,
                               QCryptoCipherAlgorithm alg,
                               QCryptoCipherMode mode,
                               const uint8_t *key, size_t nkey,
-                              size_t n_threads, Error **errp)
+                              Error **errp)
 {
-    size_t i;
+    QCryptoCipher *cipher;
 
-    assert(!block->ciphers && !block->n_ciphers && !block->n_free_ciphers);
+    assert(!block->free_ciphers && !block->max_free_ciphers &&
+           !block->n_free_ciphers);
 
-    block->ciphers = g_new0(QCryptoCipher *, n_threads);
+    /* Stash away cipher parameters for qcrypto_block_pop_cipher() */
+    block->alg = alg;
+    block->mode = mode;
+    block->key = g_memdup2(key, nkey);
+    block->nkey = nkey;
 
-    for (i = 0; i < n_threads; i++) {
-        block->ciphers[i] = qcrypto_cipher_new(alg, mode, key, nkey, errp);
-        if (!block->ciphers[i]) {
-            qcrypto_block_free_cipher(block);
-            return -1;
-        }
-        block->n_ciphers++;
-        block->n_free_ciphers++;
+    /*
+     * Create a new cipher to validate the parameters now. This reduces the
+     * chance of cipher creation failing at I/O time.
+     */
+    cipher = qcrypto_block_pop_cipher(block, errp);
+    if (!cipher) {
+        g_free(block->key);
+        block->key = NULL;
+        return -1;
     }
 
+    qcrypto_block_push_cipher(block, cipher);
     return 0;
 }
 
@@ -291,19 +304,23 @@ void qcrypto_block_free_cipher(QCryptoBlock *block)
 {
     size_t i;
 
-    if (!block->ciphers) {
+    g_free(block->key);
+    block->key = NULL;
+
+    if (!block->free_ciphers) {
         return;
     }
 
-    assert(block->n_ciphers == block->n_free_ciphers);
+    /* All popped ciphers were eventually pushed back */
+    assert(block->n_free_ciphers == block->max_free_ciphers);
 
-    for (i = 0; i < block->n_ciphers; i++) {
-        qcrypto_cipher_free(block->ciphers[i]);
+    for (i = 0; i < block->max_free_ciphers; i++) {
+        qcrypto_cipher_free(block->free_ciphers[i]);
     }
 
-    g_free(block->ciphers);
-    block->ciphers = NULL;
-    block->n_ciphers = block->n_free_ciphers = 0;
+    g_free(block->free_ciphers);
+    block->free_ciphers = NULL;
+    block->max_free_ciphers = block->n_free_ciphers = 0;
 }
 
 QCryptoIVGen *qcrypto_block_get_ivgen(QCryptoBlock *block)
@@ -311,7 +328,7 @@ QCryptoIVGen *qcrypto_block_get_ivgen(QCryptoBlock *block)
     /* ivgen should be accessed under mutex. However, this function is used only
      * in test with one thread, so it's enough to assert it here:
      */
-    assert(block->n_ciphers <= 1);
+    assert(block->max_free_ciphers <= 1);
     return block->ivgen;
 }
 
@@ -446,7 +463,10 @@ int qcrypto_block_decrypt_helper(QCryptoBlock *block,
                                  Error **errp)
 {
     int ret;
-    QCryptoCipher *cipher = qcrypto_block_pop_cipher(block);
+    QCryptoCipher *cipher = qcrypto_block_pop_cipher(block, errp);
+    if (!cipher) {
+        return -1;
+    }
 
     ret = do_qcrypto_block_cipher_encdec(cipher, block->niv, block->ivgen,
                                          &block->mutex, sectorsize, offset, buf,
@@ -465,7 +485,10 @@ int qcrypto_block_encrypt_helper(QCryptoBlock *block,
                                  Error **errp)
 {
     int ret;
-    QCryptoCipher *cipher = qcrypto_block_pop_cipher(block);
+    QCryptoCipher *cipher = qcrypto_block_pop_cipher(block, errp);
+    if (!cipher) {
+        return -1;
+    }
 
     ret = do_qcrypto_block_cipher_encdec(cipher, block->niv, block->ivgen,
                                          &block->mutex, sectorsize, offset, buf,
-- 
2.45.1



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/2] crypto/block: drop qcrypto_block_open() n_threads argument
  2024-05-27 15:58 [PATCH 0/2] block/crypto: do not require number of threads upfront Stefan Hajnoczi
  2024-05-27 15:58 ` [PATCH 1/2] block/crypto: create ciphers on demand Stefan Hajnoczi
@ 2024-05-27 15:58 ` Stefan Hajnoczi
  2024-05-29 16:50 ` [PATCH 0/2] block/crypto: do not require number of threads upfront Kevin Wolf
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 7+ messages in thread
From: Stefan Hajnoczi @ 2024-05-27 15:58 UTC (permalink / raw)
  To: qemu-devel
  Cc: Hanna Reitz, qemu-block, Kevin Wolf, Daniel P. Berrangé,
	Stefan Hajnoczi

The n_threads argument is no longer used since the previous commit.
Remove it.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 crypto/blockpriv.h             | 1 -
 include/crypto/block.h         | 2 --
 block/crypto.c                 | 1 -
 block/qcow.c                   | 2 +-
 block/qcow2.c                  | 5 ++---
 crypto/block-luks.c            | 1 -
 crypto/block-qcow.c            | 6 ++----
 crypto/block.c                 | 3 +--
 tests/unit/test-crypto-block.c | 4 ----
 9 files changed, 6 insertions(+), 19 deletions(-)

diff --git a/crypto/blockpriv.h b/crypto/blockpriv.h
index 4bf6043d5d..b8f77cb5eb 100644
--- a/crypto/blockpriv.h
+++ b/crypto/blockpriv.h
@@ -59,7 +59,6 @@ struct QCryptoBlockDriver {
                 QCryptoBlockReadFunc readfunc,
                 void *opaque,
                 unsigned int flags,
-                size_t n_threads,
                 Error **errp);
 
     int (*create)(QCryptoBlock *block,
diff --git a/include/crypto/block.h b/include/crypto/block.h
index 92e823c9f2..5b5d039800 100644
--- a/include/crypto/block.h
+++ b/include/crypto/block.h
@@ -76,7 +76,6 @@ typedef enum {
  * @readfunc: callback for reading data from the volume
  * @opaque: data to pass to @readfunc
  * @flags: bitmask of QCryptoBlockOpenFlags values
- * @n_threads: allow concurrent I/O from up to @n_threads threads
  * @errp: pointer to a NULL-initialized error object
  *
  * Create a new block encryption object for an existing
@@ -113,7 +112,6 @@ QCryptoBlock *qcrypto_block_open(QCryptoBlockOpenOptions *options,
                                  QCryptoBlockReadFunc readfunc,
                                  void *opaque,
                                  unsigned int flags,
-                                 size_t n_threads,
                                  Error **errp);
 
 typedef enum {
diff --git a/block/crypto.c b/block/crypto.c
index 21eed909c1..4eed3ffa6a 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -363,7 +363,6 @@ static int block_crypto_open_generic(QCryptoBlockFormat format,
                                        block_crypto_read_func,
                                        bs,
                                        cflags,
-                                       1,
                                        errp);
 
     if (!crypto->block) {
diff --git a/block/qcow.c b/block/qcow.c
index ca8e1d5ec8..c2f89db055 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -211,7 +211,7 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
                 cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
             }
             s->crypto = qcrypto_block_open(crypto_opts, "encrypt.",
-                                           NULL, NULL, cflags, 1, errp);
+                                           NULL, NULL, cflags, errp);
             if (!s->crypto) {
                 ret = -EINVAL;
                 goto fail;
diff --git a/block/qcow2.c b/block/qcow2.c
index 956128b409..10883a2494 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -321,7 +321,7 @@ qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
             }
             s->crypto = qcrypto_block_open(s->crypto_opts, "encrypt.",
                                            qcow2_crypto_hdr_read_func,
-                                           bs, cflags, QCOW2_MAX_THREADS, errp);
+                                           bs, cflags, errp);
             if (!s->crypto) {
                 return -EINVAL;
             }
@@ -1701,8 +1701,7 @@ qcow2_do_open(BlockDriverState *bs, QDict *options, int flags,
                 cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
             }
             s->crypto = qcrypto_block_open(s->crypto_opts, "encrypt.",
-                                           NULL, NULL, cflags,
-                                           QCOW2_MAX_THREADS, errp);
+                                           NULL, NULL, cflags, errp);
             if (!s->crypto) {
                 ret = -EINVAL;
                 goto fail;
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 3357852c0a..5b777c15d3 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -1189,7 +1189,6 @@ qcrypto_block_luks_open(QCryptoBlock *block,
                         QCryptoBlockReadFunc readfunc,
                         void *opaque,
                         unsigned int flags,
-                        size_t n_threads,
                         Error **errp)
 {
     QCryptoBlockLUKS *luks = NULL;
diff --git a/crypto/block-qcow.c b/crypto/block-qcow.c
index 02305058e3..42e9556e42 100644
--- a/crypto/block-qcow.c
+++ b/crypto/block-qcow.c
@@ -44,7 +44,6 @@ qcrypto_block_qcow_has_format(const uint8_t *buf G_GNUC_UNUSED,
 static int
 qcrypto_block_qcow_init(QCryptoBlock *block,
                         const char *keysecret,
-                        size_t n_threads,
                         Error **errp)
 {
     char *password;
@@ -100,7 +99,6 @@ qcrypto_block_qcow_open(QCryptoBlock *block,
                         QCryptoBlockReadFunc readfunc G_GNUC_UNUSED,
                         void *opaque G_GNUC_UNUSED,
                         unsigned int flags,
-                        size_t n_threads,
                         Error **errp)
 {
     if (flags & QCRYPTO_BLOCK_OPEN_NO_IO) {
@@ -115,7 +113,7 @@ qcrypto_block_qcow_open(QCryptoBlock *block,
             return -1;
         }
         return qcrypto_block_qcow_init(block, options->u.qcow.key_secret,
-                                       n_threads, errp);
+                                       errp);
     }
 }
 
@@ -135,7 +133,7 @@ qcrypto_block_qcow_create(QCryptoBlock *block,
         return -1;
     }
     /* QCow2 has no special header, since everything is hardwired */
-    return qcrypto_block_qcow_init(block, options->u.qcow.key_secret, 1, errp);
+    return qcrypto_block_qcow_init(block, options->u.qcow.key_secret, errp);
 }
 
 
diff --git a/crypto/block.c b/crypto/block.c
index ba6d1cebc7..3bcc4270c3 100644
--- a/crypto/block.c
+++ b/crypto/block.c
@@ -53,7 +53,6 @@ QCryptoBlock *qcrypto_block_open(QCryptoBlockOpenOptions *options,
                                  QCryptoBlockReadFunc readfunc,
                                  void *opaque,
                                  unsigned int flags,
-                                 size_t n_threads,
                                  Error **errp)
 {
     QCryptoBlock *block = g_new0(QCryptoBlock, 1);
@@ -73,7 +72,7 @@ QCryptoBlock *qcrypto_block_open(QCryptoBlockOpenOptions *options,
     block->driver = qcrypto_block_drivers[options->format];
 
     if (block->driver->open(block, options, optprefix,
-                            readfunc, opaque, flags, n_threads, errp) < 0)
+                            readfunc, opaque, flags, errp) < 0)
     {
         g_free(block);
         return NULL;
diff --git a/tests/unit/test-crypto-block.c b/tests/unit/test-crypto-block.c
index 6cfc817a92..42cfab6067 100644
--- a/tests/unit/test-crypto-block.c
+++ b/tests/unit/test-crypto-block.c
@@ -303,7 +303,6 @@ static void test_block(gconstpointer opaque)
                              test_block_read_func,
                              &header,
                              0,
-                             1,
                              NULL);
     g_assert(blk == NULL);
 
@@ -312,7 +311,6 @@ static void test_block(gconstpointer opaque)
                              test_block_read_func,
                              &header,
                              QCRYPTO_BLOCK_OPEN_NO_IO,
-                             1,
                              &error_abort);
 
     g_assert(qcrypto_block_get_cipher(blk) == NULL);
@@ -327,7 +325,6 @@ static void test_block(gconstpointer opaque)
                              test_block_read_func,
                              &header,
                              0,
-                             1,
                              &error_abort);
     g_assert(blk);
 
@@ -384,7 +381,6 @@ test_luks_bad_header(gconstpointer data)
                              test_block_read_func,
                              &buf,
                              0,
-                             1,
                              &err);
     g_assert(!blk);
     g_assert(err);
-- 
2.45.1



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 0/2] block/crypto: do not require number of threads upfront
  2024-05-27 15:58 [PATCH 0/2] block/crypto: do not require number of threads upfront Stefan Hajnoczi
  2024-05-27 15:58 ` [PATCH 1/2] block/crypto: create ciphers on demand Stefan Hajnoczi
  2024-05-27 15:58 ` [PATCH 2/2] crypto/block: drop qcrypto_block_open() n_threads argument Stefan Hajnoczi
@ 2024-05-29 16:50 ` Kevin Wolf
  2024-05-29 18:10   ` Stefan Hajnoczi
  2024-06-03 12:37 ` Daniel P. Berrangé
  2024-06-03 16:04 ` Kevin Wolf
  4 siblings, 1 reply; 7+ messages in thread
From: Kevin Wolf @ 2024-05-29 16:50 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: qemu-devel, Hanna Reitz, qemu-block, Daniel P. Berrangé

Am 27.05.2024 um 17:58 hat Stefan Hajnoczi geschrieben:
> The block layer does not know how many threads will perform I/O. It is possible
> to exceed the number of threads that is given to qcrypto_block_open() and this
> can trigger an assertion failure in qcrypto_block_pop_cipher().
> 
> This patch series removes the n_threads argument and instead handles an
> arbitrary number of threads.
> ---
> Is it secure to store the key in QCryptoBlock? In this series I assumed the
> answer is yes since the QCryptoBlock's cipher state is equally sensitive, but
> I'm not familiar with this code or a crypto expert.

I would assume the same, but I'm not merging this yet because I think
you said you'd like to have input from danpb?

Reviewed-by: Kevin Wolf <kwolf@redhat.com>



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 0/2] block/crypto: do not require number of threads upfront
  2024-05-29 16:50 ` [PATCH 0/2] block/crypto: do not require number of threads upfront Kevin Wolf
@ 2024-05-29 18:10   ` Stefan Hajnoczi
  0 siblings, 0 replies; 7+ messages in thread
From: Stefan Hajnoczi @ 2024-05-29 18:10 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: qemu-devel, Hanna Reitz, qemu-block, Daniel P. Berrangé

[-- Attachment #1: Type: text/plain, Size: 940 bytes --]

On Wed, May 29, 2024 at 06:50:34PM +0200, Kevin Wolf wrote:
> Am 27.05.2024 um 17:58 hat Stefan Hajnoczi geschrieben:
> > The block layer does not know how many threads will perform I/O. It is possible
> > to exceed the number of threads that is given to qcrypto_block_open() and this
> > can trigger an assertion failure in qcrypto_block_pop_cipher().
> > 
> > This patch series removes the n_threads argument and instead handles an
> > arbitrary number of threads.
> > ---
> > Is it secure to store the key in QCryptoBlock? In this series I assumed the
> > answer is yes since the QCryptoBlock's cipher state is equally sensitive, but
> > I'm not familiar with this code or a crypto expert.
> 
> I would assume the same, but I'm not merging this yet because I think
> you said you'd like to have input from danpb?
> 
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Yes, please wait until Dan comments.

Thanks,
Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 0/2] block/crypto: do not require number of threads upfront
  2024-05-27 15:58 [PATCH 0/2] block/crypto: do not require number of threads upfront Stefan Hajnoczi
                   ` (2 preceding siblings ...)
  2024-05-29 16:50 ` [PATCH 0/2] block/crypto: do not require number of threads upfront Kevin Wolf
@ 2024-06-03 12:37 ` Daniel P. Berrangé
  2024-06-03 16:04 ` Kevin Wolf
  4 siblings, 0 replies; 7+ messages in thread
From: Daniel P. Berrangé @ 2024-06-03 12:37 UTC (permalink / raw)
  To: Stefan Hajnoczi; +Cc: qemu-devel, Hanna Reitz, qemu-block, Kevin Wolf

On Mon, May 27, 2024 at 11:58:49AM -0400, Stefan Hajnoczi wrote:
> The block layer does not know how many threads will perform I/O. It is possible
> to exceed the number of threads that is given to qcrypto_block_open() and this
> can trigger an assertion failure in qcrypto_block_pop_cipher().
> 
> This patch series removes the n_threads argument and instead handles an
> arbitrary number of threads.
> ---
> Is it secure to store the key in QCryptoBlock? In this series I assumed the
> answer is yes since the QCryptoBlock's cipher state is equally sensitive, but
> I'm not familiar with this code or a crypto expert.

Yes, its a case of ....  this is undesirable, but we do it everywhere
already, so this isn't making it any worse.

For both patches

Acked-by: Daniel P. Berrangé <berrange@redhat.com>



With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 0/2] block/crypto: do not require number of threads upfront
  2024-05-27 15:58 [PATCH 0/2] block/crypto: do not require number of threads upfront Stefan Hajnoczi
                   ` (3 preceding siblings ...)
  2024-06-03 12:37 ` Daniel P. Berrangé
@ 2024-06-03 16:04 ` Kevin Wolf
  4 siblings, 0 replies; 7+ messages in thread
From: Kevin Wolf @ 2024-06-03 16:04 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: qemu-devel, Hanna Reitz, qemu-block, Daniel P. Berrangé

Am 27.05.2024 um 17:58 hat Stefan Hajnoczi geschrieben:
> The block layer does not know how many threads will perform I/O. It is possible
> to exceed the number of threads that is given to qcrypto_block_open() and this
> can trigger an assertion failure in qcrypto_block_pop_cipher().
> 
> This patch series removes the n_threads argument and instead handles an
> arbitrary number of threads.

Thanks, applied to the block branch.

Kevin



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2024-06-03 16:05 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-27 15:58 [PATCH 0/2] block/crypto: do not require number of threads upfront Stefan Hajnoczi
2024-05-27 15:58 ` [PATCH 1/2] block/crypto: create ciphers on demand Stefan Hajnoczi
2024-05-27 15:58 ` [PATCH 2/2] crypto/block: drop qcrypto_block_open() n_threads argument Stefan Hajnoczi
2024-05-29 16:50 ` [PATCH 0/2] block/crypto: do not require number of threads upfront Kevin Wolf
2024-05-29 18:10   ` Stefan Hajnoczi
2024-06-03 12:37 ` Daniel P. Berrangé
2024-06-03 16:04 ` Kevin Wolf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).