| Message ID | 20190115111007.27159-2-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qcow2: include LUKS payload overhead in qemu-img measure | expand |
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? Max > + > + crypto = qcrypto_block_create(&cryptoopts, "encrypt.", > + qcow2_measure_crypto_hdr_init_func, > + qcow2_measure_crypto_hdr_write_func, > + &headerlen, &local_err); > + > + g_free(optstr); > + if (!crypto) { > + goto err; > + } > + qcrypto_block_free(crypto); > + > + luks_payload_size = ROUND_UP(headerlen, cluster_size); > + } > + > virtual_size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0); > virtual_size = ROUND_UP(virtual_size, cluster_size);
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... Max
On 21.01.19 14:15, 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... I stand corrected: With aes-128: $ qemu-img create -f qcow2 --object secret,id=sec0,data=foo \ -o encrypt.format=luks,encrypt.key-secret=sec0,encrypt.cipher-alg=aes-128 \ foo.qcow2 64M Formatting 'foo.qcow2', fmt=qcow2 size=67108864 encrypt.format=luks encrypt.key-secret=sec0 encrypt.cipher-alg=aes-128 cluster_size=65536 lazy_refcounts=off refcount_bits=16 $ qemu-img info foo.qcow2 [...] disk size: 448K [...] With aes-256: $ qemu-img create -f qcow2 --object secret,id=sec0,data=foo \ -o encrypt.format=luks,encrypt.key-secret=sec0,encrypt.cipher-alg=aes-256 \ foo.qcow2 64MFormatting 'foo.qcow2', fmt=qcow2 size=67108864 Formatting 'foo.qcow2', fmt=qcow2 size=67108864 encrypt.format=luks encrypt.key-secret=sec0 encrypt.cipher-alg=aes-256 cluster_size=65536 lazy_refcounts=off refcount_bits=16 $ qemu-img info foo.qcow2 [...] disk size: 540K [...] So it does make a difference and those options should be parsed as well. Max
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) Regards, Daniel
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
diff --git a/block/qcow2.c b/block/qcow2.c index 4897abae5e..7ab93a5d2f 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -4231,6 +4231,24 @@ static coroutine_fn int qcow2_co_flush_to_os(BlockDriverState *bs) return ret; } +static ssize_t qcow2_measure_crypto_hdr_init_func(QCryptoBlock *block, + size_t headerlen, void *opaque, Error **errp) +{ + size_t *headerlenp = opaque; + + /* Stash away the payload size */ + *headerlenp = headerlen; + return 0; +} + +static ssize_t qcow2_measure_crypto_hdr_write_func(QCryptoBlock *block, + size_t offset, const uint8_t *buf, size_t buflen, + void *opaque, Error **errp) +{ + /* Discard the bytes, we're not actually writing to an image */ + return buflen; +} + static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs, Error **errp) { @@ -4240,11 +4258,13 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs, uint64_t virtual_size; /* disk size as seen by guest */ uint64_t refcount_bits; uint64_t l2_tables; + uint64_t luks_payload_size = 0; size_t cluster_size; int version; char *optstr; PreallocMode prealloc; bool has_backing_file; + bool has_luks; /* Parse image creation options */ cluster_size = qcow2_opt_get_cluster_size_del(opts, &local_err); @@ -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; + + crypto = qcrypto_block_create(&cryptoopts, "encrypt.", + qcow2_measure_crypto_hdr_init_func, + qcow2_measure_crypto_hdr_write_func, + &headerlen, &local_err); + + g_free(optstr); + if (!crypto) { + goto err; + } + qcrypto_block_free(crypto); + + luks_payload_size = ROUND_UP(headerlen, cluster_size); + } + virtual_size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0); virtual_size = ROUND_UP(virtual_size, cluster_size); @@ -4344,7 +4393,7 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs, info = g_new(BlockMeasureInfo, 1); info->fully_allocated = qcow2_calc_prealloc_size(virtual_size, cluster_size, - ctz32(refcount_bits)); + ctz32(refcount_bits)) + luks_payload_size; /* Remove data clusters that are not required. This overestimates the * required size because metadata needed for the fully allocated file is
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(-)