Re: [Qemu-devel] [PATCH v8 13/20] qcow2: add support for LUKS encryption format

[Daniel P. Berrange]

On Wed, Jun 07, 2017 at 07:19:28PM +0200, Max Reitz wrote:
> On 2017-06-01 19:27, Daniel P. Berrange wrote:
> > This adds support for using LUKS as an encryption format
> > with the qcow2 file, using the new encrypt.format parameter
> > to request "luks" format. e.g.
> > 
> >   # qemu-img create --object secret,data=123456,id=sec0 \
> >        -f qcow2 -o encrypt.format=luks,encrypt.key-secret=sec0 \
> >        test.qcow2 10G
> > 
> > The legacy "encryption=on" parameter still results in
> > creation of the old qcow2 AES format (and is equivalent
> > to the new 'encryption-format=aes'). e.g. the following are
> > equivalent:
> > 
> >   # qemu-img create --object secret,data=123456,id=sec0 \
> >        -f qcow2 -o encryption=on,encrypt.key-secret=sec0 \
> >        test.qcow2 10G
> > 
> >  # qemu-img create --object secret,data=123456,id=sec0 \
> >        -f qcow2 -o encryption-format=aes,encrypt.key-secret=sec0 \
> >        test.qcow2 10G
> > 
> > With the LUKS format it is necessary to store the LUKS
> > partition header and key material in the QCow2 file. This
> > data can be many MB in size, so cannot go into the QCow2
> > header region directly. Thus the spec defines a FDE
> > (Full Disk Encryption) header extension that specifies
> > the offset of a set of clusters to hold the FDE headers,
> > as well as the length of that region. The LUKS header is
> > thus stored in these extra allocated clusters before the
> > main image payload.
> > 
> > Aside from all the cryptographic differences implied by
> > use of the LUKS format, there is one further key difference
> > between the use of legacy AES and LUKS encryption in qcow2.
> > For LUKS, the initialiazation vectors are generated using
> > the host physical sector as the input, rather than the
> > guest virtual sector. This guarantees unique initialization
> > vectors for all sectors when qcow2 internal snapshots are
> > used, thus giving stronger protection against watermarking
> > attacks.
> > 
> > Reviewed-by: Eric Blake <address@hidden>
> > Reviewed-by: Alberto Garcia <address@hidden>
> > Signed-off-by: Daniel P. Berrange <address@hidden>
> > ---
> >  block/qcow2-cluster.c      |   4 +-
> >  block/qcow2-refcount.c     |  10 ++
> >  block/qcow2.c              | 267 
> > ++++++++++++++++++++++++++++++++++++++------
> >  block/qcow2.h              |   9 ++
> >  qapi/block-core.json       |   5 +-
> >  tests/qemu-iotests/082.out | 270 
> > ++++++++++++++++++++++++++++++++++++---------
> >  6 files changed, 477 insertions(+), 88 deletions(-)
> 
> [...]
> 
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index 38c0420..30d0343 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> 
> [...]
> 
> > @@ -2072,22 +2253,30 @@ static int qcow2_set_up_encryption(BlockDriverState 
> > *bs, const char *encryptfmt,
> >      qdict_extract_subqdict(options, &encryptopts, "encrypt.");
> >      QDECREF(options);
> >  
> > -    if (!g_str_equal(encryptfmt, "aes")) {
> > -        error_setg(errp, "Unknown encryption format '%s', expected 'aes'",
> > -                   encryptfmt);
> > -        ret = -EINVAL;
> > -        goto out;
> > +    int fmt = qcow2_crypt_method_from_format(encryptfmt);
> 
> Because I love nit picking: Variable declarations should be at the start
> of the surrounding block.

Opps, dunno what I was thinking there.

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 :|