Re: [Qemu-devel] [PATCH 1/2] qcow2: include LUKS payload overhead in qemu-img measure

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Mon, Jan 21, 2019 at 01:45:44PM +0000, Daniel P. Berrangé wrote:
> On Mon, Jan 21, 2019 at 02:15:03PM +0100, Max Reitz wrote:
> > On 21.01.19 14:08, Max Reitz wrote:
> > > On 15.01.19 12:10, Stefan Hajnoczi wrote:
> > >> LUKS encryption reserves clusters for its own payload data.  The size of
> > >> this area must be included in the qemu-img measure calculation so that
> > >> we arrive at the correct minimum required image size.
> > >>
> > >> (Ab)use the qcrypto_block_create() API to determine the payload
> > >> overhead.  We discard the payload data that qcrypto thinks will be
> > >> written to the image.
> > >>
> > >> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > >> ---
> > >>  block/qcow2.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> > >>  1 file changed, 50 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/block/qcow2.c b/block/qcow2.c
> > >> index 4897abae5e..7ab93a5d2f 100644
> > >> --- a/block/qcow2.c
> > >> +++ b/block/qcow2.c
> > > 
> > > [...]
> > > 
> > >> @@ -4274,6 +4294,35 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
> > >>      has_backing_file = !!optstr;
> > >>      g_free(optstr);
> > >>  
> > >> +    optstr = qemu_opt_get_del(opts, BLOCK_OPT_ENCRYPT_FORMAT);
> > >> +    has_luks = optstr && strcmp(optstr, "luks") == 0;
> > >> +    g_free(optstr);
> > >> +
> > >> +    if (has_luks) {
> > >> +        QCryptoBlockCreateOptions cryptoopts = {
> > >> +            .format = Q_CRYPTO_BLOCK_FORMAT_LUKS,
> > >> +        };
> > >> +        QCryptoBlock *crypto;
> > >> +        size_t headerlen;
> > >> +
> > >> +        optstr = qemu_opt_get_del(opts, "encrypt.key-secret");
> > >> +        cryptoopts.u.luks.has_key_secret = !!optstr;
> > >> +        cryptoopts.u.luks.key_secret = optstr;
> > > 
> > > I wonder if you couldn't just make some secret up here (if the user
> > > doesn't specify anything).  Its content shouldn't matter, right?
> > 
> > And now I wonder whether other options may not have actual influence on
> > the crypto header size.  I suppose in theory the cipher algorithm may
> > cause a difference, although it's probably not enough to warrant another
> > cluster...
> 
> The LUKS header comprises three parts
> 
>  - A field size initial header with general config properties
>  - A series of fixed size key slot headers, one per slot, fixed
>    at 8 slots in the code
>  - A series of regions of key material, one per slot.
> 
> The last one is a problem - the size of the key slot regions is
> the number of anti-forensic stripes (4096) multipled by the size
> of the master key. The latter depends on the size of the cipher
> key. This is referred to as "splitkeylen" in the code creating
> this. The overall space reserved for headers is then determined
> by:
> 
>     luks->header.payload_offset =
>         (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
>          QCRYPTO_BLOCK_LUKS_SECTOR_SIZE) +
>         (ROUND_UP(DIV_ROUND_UP(splitkeylen, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE),
>                   (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
>                    QCRYPTO_BLOCK_LUKS_SECTOR_SIZE)) *
>          QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS);
> 
> So for AES-128  key materials takes 0.5 MB, while for AES-256 key material
> takes 1 MB, so adding together you get 1052672 byte for header in AES-128
> and 2068480 bytes for AES-256, rounded up to qcow2 cluster size (whatever
> that is configured to be)

Thanks.  I think qemu-img measure needs to take all of
QCryptoBlockCreateOptions and pass them in to qcrypto_block_create().
That way the result is accurate (although specific to the options that
were provided).

The current patch constructs a dummy QCryptoBlockCreateOptions instead
of populating it with options from the command-line.  I'll try to fix
this in v2.

Stefan

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