From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35084) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aRokK-0000mu-V4 for qemu-devel@nongnu.org; Fri, 05 Feb 2016 17:20:58 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aRokJ-0007zJ-BA for qemu-devel@nongnu.org; Fri, 05 Feb 2016 17:20:56 -0500 References: <1453311539-1193-1-git-send-email-berrange@redhat.com> <1453311539-1193-11-git-send-email-berrange@redhat.com> From: Eric Blake Message-ID: <56B5203B.1010209@redhat.com> Date: Fri, 5 Feb 2016 15:20:43 -0700 MIME-Version: 1.0 In-Reply-To: <1453311539-1193-11-git-send-email-berrange@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="GOPF5pmJJtND82exC1XlFrtESBFisdImb" Subject: Re: [Qemu-devel] [PATCH v2 10/17] block: add generic full disk encryption driver List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" , qemu-devel@nongnu.org Cc: Kevin Wolf , Fam Zheng , qemu-block@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --GOPF5pmJJtND82exC1XlFrtESBFisdImb Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 01/20/2016 10:38 AM, Daniel P. Berrange wrote: > Add a block driver that is capable of supporting any full disk > encryption format. This utilizes the previously added block > encryption code, and at this time supports the LUKS format. >=20 > The driver code is capable of supporting any format supported > by the QCryptoBlock module, so it registers one block driver > for each format. >=20 > At this time, the "luks" driver is registered. New LUKS > compatible volume can be formatted using qemu-img >=20 > $ qemu-img create --object secret,data=3D123456,id=3Dsec0 \ > -f luks -o key-secret=3Dsec0,cipher-alg=3Daes-256,\ > cipher-mode=3Dcbc,ivgen-alg=3Dplain64,hash-alg=3Dsha25= 6 \ > demo.luks 10G Are you still adding -o options like this, or is it better to use the new --image-opts flag from your other series for this purpose? >=20 > And query its size >=20 > $ qemu-img info --object secret,data=3D123456,id=3Dsec0 --source demo.= luks,driver=3Dluks,key-secret=3Dsec0 And this definitely looks stale. > image: json:{"key-secret": "sec0", "driver": "luks", "file": {"driver":= "file", "filename": "demo.luks"}} > file format: luks > virtual size: 10.0G (10737416192 bytes) That size is 2048 bytes less than 10G; what happened? With default striping, you have 4000 stripes * 32 byte aes-256 key * 8 key material + header (where I pointed out that you are using a default offset of 4096, even though 1024 would be sufficient on 512-byte sectors), or roughly 1M, occupied by the LUKS header (in fact, having the payload aligned to 1M is probably wise, even if it can pack in tighter, just because a 1M-aligned disk with a 1M header is a GOOD thing). Therefore, you should either be counting the _entire_ LUKS header as part of the image (and report a full 10G), or you should _only_ be counting the payload size (and report 10G-1M, not 10G-2k). My vote: do the same as we do for qcow2 or any other format. Make the size requested by the user as the size visible to the guest, and a fully-allocated image will take more space on the host than what the guest is using (that is, a fully-allocated 10G LUKS disk would present 10G payload to the guest but occupy 10G+1M on the host). > disk size: 132K This, however, looks reasonable - with the defaults, key material 0 will occupy just under 128k, and nothing is written in key material 1-7. >=20 > All volumes created by this new 'luks' driver should be > capable of being opened by the kernel dm-crypt driver. > With this initial impl, not all volumes created with > dm-crypt can be opened by the QEMU 'luks' driver. This > is due to lack of support for certain algorithms, in > particular the 'xts' cipher mode. These limitations will > be addressed in a later series of patches, with the > intent that QEMU should be able to open anything that > dm-crypt LUKS supports. Cool! Would it be worth augmenting the commit message to show a sequence of commands where you loopback mount a LUKS image file created by qemu, then wire that up with cryptsetup, as a proof that the two are compatible implementations? >=20 > Signed-off-by: Daniel P. Berrange > --- > block/Makefile.objs | 2 + > block/crypto.c | 541 +++++++++++++++++++++++++++++++++++++++++++= ++++++++ > qapi/block-core.json | 18 +- > 3 files changed, 560 insertions(+), 1 deletion(-) > create mode 100644 block/crypto.c >=20 > +++ b/block/crypto.c > + > +#define BLOCK_CRYPTO_OPT_LUKS_KEY_SECRET "key-secret" > +#define BLOCK_CRYPTO_OPT_LUKS_CIPHER_ALG "cipher-alg" > +#define BLOCK_CRYPTO_OPT_LUKS_CIPHER_MODE "cipher-mode" > +#define BLOCK_CRYPTO_OPT_LUKS_IVGEN_ALG "ivgen-alg" > +#define BLOCK_CRYPTO_OPT_LUKS_IVGEN_HASH_ALG "ivgen-hash-alg" > +#define BLOCK_CRYPTO_OPT_LUKS_HASH_ALG "hash-alg" I suggested in 7/17 that user-specified striping might be a parameter worth adding; I guess this would be impacted as well. > + > +static int block_crypto_probe_generic(QCryptoBlockFormat format, > + const uint8_t *buf, > + int buf_size, > + const char *filename) > +{ > + if (qcrypto_block_has_format(format, > + buf, buf_size)) { > + return 100; I don't know why we have particular values for any of the probes, but at least this matches the arbitrary value reported by qcow2. > + } else { > + return 0; > + } > +} > + > + > +static ssize_t block_crypto_read_func(QCryptoBlock *block, > + size_t offset, > + uint8_t *buf, > + size_t buflen, > + Error **errp, > + void *opaque) > +{ > + BlockDriverState *bs =3D opaque; > + ssize_t ret; > + > + ret =3D bdrv_pread(bs->file->bs, offset, buf, buflen); > + if (ret < 0) { > + error_setg_errno(errp, -ret, "Could not read encryption header= "); Are we guaranteed that the offset being read here will always lie in the encryption header portion of the file? > + return ret; > + } > + return ret; > +} > + > + > +static ssize_t block_crypto_write_func(QCryptoBlock *block, > + size_t offset, > + const uint8_t *buf, > + size_t buflen, > + Error **errp, > + void *opaque) > +{ > + BlockDriverState *bs =3D opaque; > + ssize_t ret; > + > + ret =3D bdrv_pwrite(bs, offset, buf, buflen); > + if (ret < 0) { > + error_setg_errno(errp, -ret, "Could not write encryption heade= r"); Likewise. > + > +#define BLOCK_CRYPTO_MAX_SECTORS 32 16k is not enough space to represent the LUKS header and the first key material. Does this number need to be larger to allow reading up to 1M of the image to find all 8 key materials? Or is this just being used to limit the maximum amount we'll encrypt/decrypt in one pass of the loop? > + > +static coroutine_fn int > +block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num, > + int remaining_sectors, QEMUIOVector *qiov) > +{ > + BlockCrypto *crypto =3D bs->opaque; > + int cur_nr_sectors; /* number of sectors in current iteration */ > + uint64_t bytes_done =3D 0; > + uint8_t *cipher_data =3D NULL; > + QEMUIOVector hd_qiov; > + int ret =3D 0; > + size_t payload_offset =3D qcrypto_block_get_payload_offset(crypto-= >block); > + > + qemu_iovec_init(&hd_qiov, qiov->niov); > + > + qemu_co_mutex_lock(&crypto->lock); > + > + while (remaining_sectors) { > + cur_nr_sectors =3D remaining_sectors; > + > + if (cur_nr_sectors > BLOCK_CRYPTO_MAX_SECTORS) { > + cur_nr_sectors =3D BLOCK_CRYPTO_MAX_SECTORS; > + } > + cipher_data =3D > + qemu_try_blockalign(bs->file->bs, cur_nr_sectors * 512); Why are you allocating the block in the loop? Can you hoist the allocation outside, and reuse the memory across iterations? > + > +static int64_t block_crypto_getlength(BlockDriverState *bs) > +{ > + BlockCrypto *crypto =3D bs->opaque; > + int64_t len =3D bdrv_getlength(bs->file->bs); > + > + ssize_t offset =3D qcrypto_block_get_payload_offset(crypto->block)= ; > + > + len -=3D (offset * 512); Will need a tweak if you fix qcrypto_block_get_payload_offset to be in bytes. > +++ b/qapi/block-core.json > @@ -1546,7 +1546,7 @@ > { 'enum': 'BlockdevDriver', > 'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop', > 'dmg', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device',= > - 'http', 'https', 'null-aio', 'null-co', 'parallels', > + 'http', 'https', 'luks', 'null-aio', 'null-co', 'parallels= ', Missing documentation; BlockDeviceInfo has been where we've documented which release introduced which drivers. > ## > +# @BlockdevOptionsLUKS > +# > +# Driver specific block device options for LUKS. > +# > +# @key-secret: #optional the ID of a QCryptoSecret object providing > +# the decryption key (since 2.6) Worth mentioning that it is mandatory except when doing a metadata-only probe. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --GOPF5pmJJtND82exC1XlFrtESBFisdImb Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJWtSA7AAoJEKeha0olJ0NqH9YH/0229CqP6hjjfJuL0WS8Zc0h DCSnDBcesvSeAEqLg04sPfQPZFI+V1Di65/bpb+evLcWE20QpdCXggXyYIPMXgbl xmI2rkLaAXhU6vC1Mh9U57qummjsQ4695xD6FgTy6OiXmNOEPbiEbM4QhO8FgYib 0HmAHLivQu11KEuKyqHwpomR6RHezN8AFOtZ9AUbgFRy2upkhZWvVLLxWwDO2lZS VHjV7aoRZxuZHeXe1nskGFPzSnI0De6Tckp2Fj51O/tQhz5aDz18QDwKvM4NVcQb HaJaqQ0KoLJ32MdAQS9IB8stySEID8r+l8v20IRVQwkIhhN668YZQrB/NZcTmGs= =/Gem -----END PGP SIGNATURE----- --GOPF5pmJJtND82exC1XlFrtESBFisdImb--