Re: [Qemu-devel] [PATCH v5 11/18] qcow2: convert QCow2 to use QCryptoBlock for encryption

[Daniel P. Berrange]

On Tue, Feb 21, 2017 at 02:30:10PM +0100, Alberto Garcia wrote:
> On Tue 21 Feb 2017 12:55:05 PM CET, Daniel P. Berrange wrote:
> > +    switch (s->crypt_method_header) {
> > +    case QCOW_CRYPT_NONE:
> > +        break;
> > +
> > +    case QCOW_CRYPT_AES:
> > +        r->crypto_opts = block_crypto_open_opts_init(
> > +            Q_CRYPTO_BLOCK_FORMAT_QCOW, opts, "aes-", errp);
> > +        break;
> > +
> > +    default:
> > +        error_setg(errp, "Unsupported encryption method %d",
> > +                   s->crypt_method_header);
> > +        break;
> > +    }
> > +    if (s->crypt_method_header && !r->crypto_opts) {
> > +        ret = -EINVAL;
> > +        goto fail;
> > +    }
> 
> This last condition relies on the assumption that QCOW_CRYPT_NONE == 0.
> 
> I think it's safe to assume that its value is never going to change and
> therefore this isn't too important, but I'm just pointing it out in case
> you want to make it explicit.

Yeah, I'll make it explicit to be kinder to future reviewers :-)

> 
> > @@ -1122,6 +1145,24 @@ static int qcow2_open(BlockDriverState *bs, QDict 
> > *options, int flags,
> >          goto fail;
> >      }
> >  
> > +    if (s->crypt_method_header == QCOW_CRYPT_AES) {
> > +        unsigned int cflags = 0;
> > +        if (flags & BDRV_O_NO_IO) {
> > +            cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
> > +        }
> > +        /* TODO how do we pass the same crypto opts down to the
> > +         * backing file by default, so we don't have to manually
> > +         * provide the same key-secret property against the full
> > +         * backing chain
> > +         */
> > +        s->crypto = qcrypto_block_open(s->crypto_opts, NULL, NULL,
> > +                                       cflags, errp);
> > +        if (!s->crypto) {
> > +            ret = -EINVAL;
> > +            goto fail;
> > +        }
> > +    }
> 
> Actually this has the same problem that I mentioned for patch 9: if
> qcow2_open() fails then s->crypto is leaked.

Yep, and the crypto_opts actually


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