Re: [Qemu-block] [PATCH 03/13] qcrypto-luks: refactoring: extract load/store/check/parse header functions

2019-08-22 Thread Daniel P . Berrangé
On Wed, Aug 14, 2019 at 11:22:09PM +0300, Maxim Levitsky wrote:
> With upcoming key management, the header will
> need to be stored after the image is created.
> 
> Extracting load header isn't strictly needed, but
> do this anyway for the symmetry.
> 
> Also I extracted a function that does basic sanity
> checks on the just read header, and a function
> which parses all the crypto format to make the
> code a bit more readable, plus now the code
> doesn't destruct the in-header cipher-mode string,
> so that the header now can be stored many times,
> which is needed for the key management.
> 
> Also this allows to contain the endianess conversions
> in these functions alone
> 
> The header is no longer endian swapped in place,
> to prevent (mostly theoretical races I think)
> races where someone could see the header in the
> process of beeing byteswapped.
> 
> Signed-off-by: Maxim Levitsky 
> ---
>  crypto/block-luks.c | 756 ++--
>  1 file changed, 440 insertions(+), 316 deletions(-)

>  if (!(flags & QCRYPTO_BLOCK_OPEN_NO_IO)) {
>  /* Try to find which key slot our password is valid for
>   * and unlock the master key from that slot.
>   */
> -
>  masterkey = g_new0(uint8_t, masterkeylen(luks));
>  
>  if (qcrypto_block_luks_find_key(block,
> @@ -845,12 +1132,10 @@ qcrypto_block_luks_open(QCryptoBlock *block,
>  }
>  
>  block->sector_size = QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
> -block->payload_offset = luks->header.payload_offset *
> -block->sector_size;
> +block->payload_offset = luks->header.payload_offset * block->sector_size;
>  
>  g_free(masterkey);
>  g_free(password);
> -
>  return 0;

Smoe unrelated whitespace changes here.


> +/* populate the slot 0 with the password encrypted master key*/
> +/* This will also store the header */
> +if (qcrypto_block_luks_store_key(block,
> + 0,
> + password,
> + masterkey,
> + luks_opts.iter_time,
> + writefunc,
> + opaque,
> + errp)) {
>  goto error;
> -}
> + }

Indent is off by 1


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



Re: [Qemu-block] [PATCH 03/13] qcrypto-luks: refactoring: extract load/store/check/parse header functions

2019-08-20 Thread Max Reitz
On 14.08.19 22:22, Maxim Levitsky wrote:
> With upcoming key management, the header will
> need to be stored after the image is created.
> 
> Extracting load header isn't strictly needed, but
> do this anyway for the symmetry.
> 
> Also I extracted a function that does basic sanity
> checks on the just read header, and a function
> which parses all the crypto format to make the
> code a bit more readable, plus now the code
> doesn't destruct the in-header cipher-mode string,
> so that the header now can be stored many times,
> which is needed for the key management.
> 
> Also this allows to contain the endianess conversions
> in these functions alone
> 
> The header is no longer endian swapped in place,
> to prevent (mostly theoretical races I think)
> races where someone could see the header in the
> process of beeing byteswapped.

The formatting looks weird, it doesn’t look quite 72 characters wide...
 (what commit messages normally use)

> Signed-off-by: Maxim Levitsky 
> ---
>  crypto/block-luks.c | 756 ++--
>  1 file changed, 440 insertions(+), 316 deletions(-)

Also, this commit is just too big.

Max



signature.asc
Description: OpenPGP digital signature


[Qemu-block] [PATCH 03/13] qcrypto-luks: refactoring: extract load/store/check/parse header functions

2019-08-14 Thread Maxim Levitsky
With upcoming key management, the header will
need to be stored after the image is created.

Extracting load header isn't strictly needed, but
do this anyway for the symmetry.

Also I extracted a function that does basic sanity
checks on the just read header, and a function
which parses all the crypto format to make the
code a bit more readable, plus now the code
doesn't destruct the in-header cipher-mode string,
so that the header now can be stored many times,
which is needed for the key management.

Also this allows to contain the endianess conversions
in these functions alone

The header is no longer endian swapped in place,
to prevent (mostly theoretical races I think)
races where someone could see the header in the
process of beeing byteswapped.

Signed-off-by: Maxim Levitsky 
---
 crypto/block-luks.c | 756 ++--
 1 file changed, 440 insertions(+), 316 deletions(-)

diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 48213abde7..6bb369f3b4 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -417,6 +417,427 @@ static int masterkeylen(QCryptoBlockLUKS *luks)
 }
 
 
+/*
+ * Stores the main LUKS header, taking care of endianess
+ */
+static int
+qcrypto_block_luks_store_header(QCryptoBlock *block,
+QCryptoBlockWriteFunc writefunc,
+void *opaque,
+Error **errp)
+{
+QCryptoBlockLUKS *luks = block->opaque;
+Error *local_err = NULL;
+size_t i;
+QCryptoBlockLUKSHeader *hdr_copy;
+
+/* Create a copy of the header */
+hdr_copy = g_new0(QCryptoBlockLUKSHeader, 1);
+memcpy(hdr_copy, >header, sizeof(QCryptoBlockLUKSHeader));
+
+/*
+ * Everything on disk uses Big Endian (tm), so flip header fields
+ * before writing them
+ */
+cpu_to_be16s(_copy->version);
+cpu_to_be32s(_copy->payload_offset);
+cpu_to_be32s(_copy->key_bytes);
+cpu_to_be32s(_copy->master_key_iterations);
+
+for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
+cpu_to_be32s(_copy->key_slots[i].active);
+cpu_to_be32s(_copy->key_slots[i].iterations);
+cpu_to_be32s(_copy->key_slots[i].key_offset);
+cpu_to_be32s(_copy->key_slots[i].stripes);
+}
+
+/* Write out the partition header and key slot headers */
+writefunc(block, 0, (const uint8_t *)hdr_copy, sizeof(*hdr_copy),
+  opaque, _err);
+
+g_free(hdr_copy);
+
+if (local_err) {
+error_propagate(errp, local_err);
+return -1;
+}
+return 0;
+}
+
+/*
+ * Loads the main LUKS header,and byteswaps it to native endianess
+ * And run basic sanity checks on it
+ */
+static int
+qcrypto_block_luks_load_header(QCryptoBlock *block,
+QCryptoBlockReadFunc readfunc,
+void *opaque,
+Error **errp)
+{
+ssize_t rv;
+size_t i;
+int ret = 0;
+QCryptoBlockLUKS *luks = block->opaque;
+
+/*
+ * Read the entire LUKS header, minus the key material from
+ * the underlying device
+ */
+
+rv = readfunc(block, 0,
+  (uint8_t *)>header,
+  sizeof(luks->header),
+  opaque,
+  errp);
+if (rv < 0) {
+ret = rv;
+goto fail;
+}
+
+/*
+ * The header is always stored in big-endian format, so
+ * convert everything to native
+ */
+be16_to_cpus(>header.version);
+be32_to_cpus(>header.payload_offset);
+be32_to_cpus(>header.key_bytes);
+be32_to_cpus(>header.master_key_iterations);
+
+for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
+be32_to_cpus(>header.key_slots[i].active);
+be32_to_cpus(>header.key_slots[i].iterations);
+be32_to_cpus(>header.key_slots[i].key_offset);
+be32_to_cpus(>header.key_slots[i].stripes);
+}
+
+
+return 0;
+fail:
+return ret;
+}
+
+
+/*
+ * Does basic sanity checks on the LUKS header
+ */
+static int
+qcrypto_block_luks_check_header(QCryptoBlockLUKS *luks, Error **errp)
+{
+int ret;
+
+if (memcmp(luks->header.magic, qcrypto_block_luks_magic,
+   QCRYPTO_BLOCK_LUKS_MAGIC_LEN) != 0) {
+error_setg(errp, "Volume is not in LUKS format");
+ret = -EINVAL;
+goto fail;
+}
+
+if (luks->header.version != QCRYPTO_BLOCK_LUKS_VERSION) {
+error_setg(errp, "LUKS version %" PRIu32 " is not supported",
+   luks->header.version);
+ret = -ENOTSUP;
+goto fail;
+}
+
+return 0;
+fail:
+return ret;
+}
+
+
+/*
+ * Parses the crypto parameters that are stored in the LUKS header
+ * to string
+ */
+
+static int
+qcrypto_block_luks_parse_header(QCryptoBlockLUKS *luks, Error **errp)
+{
+char *cipher_mode = g_strdup(luks->header.cipher_mode);
+char *ivgen_name, *ivhash_name;
+int ret = -1;
+Error *local_err = NULL;
+
+/*
+