[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v4 13/18] qcow2: add support for LUKS encryption
From: |
Daniel P. Berrange |
Subject: |
Re: [Qemu-block] [PATCH v4 13/18] qcow2: add support for LUKS encryption format |
Date: |
Mon, 20 Feb 2017 18:18:13 +0000 |
User-agent: |
Mutt/1.7.1 (2016-10-04) |
On Thu, Feb 16, 2017 at 02:42:04PM +0100, Alberto Garcia wrote:
> On Fri 10 Feb 2017 06:09:05 PM CET, Daniel P. Berrange wrote:
> > @@ -990,12 +1123,6 @@ static int qcow2_open(BlockDriverState *bs, QDict
> > *options, int flags,
> > s->refcount_max = UINT64_C(1) << (s->refcount_bits - 1);
> > s->refcount_max += s->refcount_max - 1;
> >
> > - if (header.crypt_method > QCOW_CRYPT_AES) {
> > - error_setg(errp, "Unsupported encryption method: %" PRIu32,
> > - header.crypt_method);
> > - ret = -EINVAL;
> > - goto fail;
> > - }
>
> Here you remove the check for a valid encryption method...
>
> > s->crypt_method_header = header.crypt_method;
> > if (s->crypt_method_header) {
> > if (bdrv_uses_whitelist() &&
> > @@ -1012,6 +1139,12 @@ static int qcow2_open(BlockDriverState *bs, QDict
> > *options, int flags,
> > goto fail;
> > }
> >
> > + if (s->crypt_method_header == QCOW_CRYPT_AES) {
> > + s->crypt_physical_offset = false;
> > + } else {
> > + s->crypt_physical_offset = true;
> > + }
> > +
>
> ...and here you assume that if it's not AES then it must be LUKS.
> However at this point crypt_method_header hasn't been verified yet and
> can have any value.
>
> The image will fail to open anyway because of the qcow2_update_options()
> call later in this function, but I think it wouldn't hurt to have an
> explicit check here, or at least an explanatory comment.
Yes, we're assuming LUKS and any future scheme we implement will
use the else{} code path, the first path is insecure.
>
> > +static int qcow2_crypt_method_from_format(const char *format)
> > +{
> > + if (g_str_equal(format, "luks")) {
> > + return QCOW_CRYPT_LUKS;
> > + } else if (g_str_equal(format, "aes")) {
> > + return QCOW_CRYPT_AES;
> > + } else {
> > + return -EINVAL;
> > + }
> > +}
> >
> > static int qcow2_set_up_encryption(BlockDriverState *bs, QemuOpts *opts,
> > - Error **errp)
> > + const char *format, Error **errp)
> > {
> > BDRVQcow2State *s = bs->opaque;
> > QCryptoBlockCreateOptions *cryptoopts = NULL;
> > QCryptoBlock *crypto = NULL;
> > int ret = -EINVAL;
> >
> > - cryptoopts = block_crypto_create_opts_init(
> > - Q_CRYPTO_BLOCK_FORMAT_QCOW, opts, "aes-", errp);
> > + if (g_str_equal(format, "luks")) {
> > + cryptoopts = block_crypto_create_opts_init(
> > + Q_CRYPTO_BLOCK_FORMAT_LUKS, opts, "luks-", errp);
> > + s->crypt_method_header = QCOW_CRYPT_LUKS;
> > + } else if (g_str_equal(format, "aes")) {
> > + cryptoopts = block_crypto_create_opts_init(
> > + Q_CRYPTO_BLOCK_FORMAT_QCOW, opts, "aes-", errp);
> > + s->crypt_method_header = QCOW_CRYPT_AES;
> > + } else {
>
> You just added a nice qcow2_crypt_method_from_format() function
> immediately before this one, I think this one would be more readable if
> you use it here.
Yes, it lets us turn it into a nicer switch()
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
- Re: [Qemu-block] [PATCH v4 12/18] qcow2: extend specification to cover LUKS encryption, (continued)
- [Qemu-block] [PATCH v4 11/18] qcow2: convert QCow2 to use QCryptoBlock for encryption, Daniel P. Berrange, 2017/02/10
- [Qemu-block] [PATCH v4 15/18] iotests: enable tests 134 and 158 to work with qcow (v1), Daniel P. Berrange, 2017/02/10
- [Qemu-block] [PATCH v4 14/18] qcow2: add iotests to cover LUKS encryption support, Daniel P. Berrange, 2017/02/10
- [Qemu-block] [PATCH v4 13/18] qcow2: add support for LUKS encryption format, Daniel P. Berrange, 2017/02/10
- [Qemu-block] [PATCH v4 16/18] block: rip out all traces of password prompting, Daniel P. Berrange, 2017/02/10
- [Qemu-block] [PATCH v4 17/18] block: remove all encryption handling APIs, Daniel P. Berrange, 2017/02/10
- [Qemu-block] [PATCH v4 18/18] block: pass option prefix down to crypto layer, Daniel P. Berrange, 2017/02/10