[0/2] block/crypto: do not require number of threads upfront

Message

Stefan Hajnoczi May 27, 2024, 3:58 p.m. UTC

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(-)

Comments

Kevin Wolf May 29, 2024, 4:50 p.m. UTC | #1

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 <[e-mail]>

Stefan Hajnoczi May 29, 2024, 6:10 p.m. UTC | #2

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 <[e-mail]>

Yes, please wait until Dan comments.

Thanks,
Stefan

Daniel P. Berrangé June 3, 2024, 12:37 p.m. UTC | #3

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é <[e-mail]>



With regards,
Daniel

Kevin Wolf June 3, 2024, 4:04 p.m. UTC | #4

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