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