Re: [Qemu-devel] [PATCH 03/13] qcrypto-luks: refactoring: extract load/store/check/parse header functions
On Thu, 2019-08-22 at 11:34 +0100, Daniel P. Berrangé wrote: > On Thu, Aug 22, 2019 at 01:43:05AM +0300, Maxim Levitsky wrote: > > On Tue, 2019-08-20 at 20:01 +0200, Max Reitz wrote: > > > 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) > > > > Could you elaborate on that? I thought that code should not > > exceed 80 character limit. > > > > > > > > > Signed-off-by: Maxim Levitsky > > > > --- > > > > crypto/block-luks.c | 756 ++-- > > > > 1 file changed, 440 insertions(+), 316 deletions(-) > > > > > > Also, this commit is just too big. > > > > Yea, but it has no functional changes. > > I can split it further, but that won't help much IMHO. > > I'd find it easier to review if each newly introduced method was a > separate patch. It makes it easier to see which bit of removed > code was added to which method. > > Regards, > Daniel Done, patch is now split in several patches. Best regards, Maxim Levitsky
Re: [Qemu-devel] [PATCH 03/13] qcrypto-luks: refactoring: extract load/store/check/parse header functions
On Thu, 2019-08-22 at 11:38 +0100, Daniel P. Berrangé wrote:
> 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
Fixed,
Best regards,
Maxim Levitsky
Re: [Qemu-devel] [PATCH 03/13] qcrypto-luks: refactoring: extract load/store/check/parse header functions
On Thu, 2019-08-22 at 11:32 +0100, Daniel P. Berrangé wrote: > On Thu, Aug 22, 2019 at 01:43:05AM +0300, Maxim Levitsky wrote: > > On Tue, 2019-08-20 at 20:01 +0200, Max Reitz wrote: > > > 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) > > > > Could you elaborate on that? I thought that code should not > > exceed 80 character limit. > > Code itself should be wrapped at 80 chars, but it is common for > commit messages to use 72 chars wrapping. > > > Regards, > Daniel All right, will do this. Best regards, Maxim Levitsky
Re: [Qemu-devel] [PATCH 03/13] qcrypto-luks: refactoring: extract load/store/check/parse header functions
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-devel] [PATCH 03/13] qcrypto-luks: refactoring: extract load/store/check/parse header functions
On Thu, Aug 22, 2019 at 01:43:05AM +0300, Maxim Levitsky wrote: > On Tue, 2019-08-20 at 20:01 +0200, Max Reitz wrote: > > 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) > Could you elaborate on that? I thought that code should not > exceed 80 character limit. > > > > > > Signed-off-by: Maxim Levitsky > > > --- > > > crypto/block-luks.c | 756 ++-- > > > 1 file changed, 440 insertions(+), 316 deletions(-) > > > > Also, this commit is just too big. > > Yea, but it has no functional changes. > I can split it further, but that won't help much IMHO. I'd find it easier to review if each newly introduced method was a separate patch. It makes it easier to see which bit of removed code was added to which method. 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-devel] [PATCH 03/13] qcrypto-luks: refactoring: extract load/store/check/parse header functions
On Thu, Aug 22, 2019 at 01:43:05AM +0300, Maxim Levitsky wrote: > On Tue, 2019-08-20 at 20:01 +0200, Max Reitz wrote: > > 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) > Could you elaborate on that? I thought that code should not > exceed 80 character limit. Code itself should be wrapped at 80 chars, but it is common for commit messages to use 72 chars wrapping. 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-devel] [PATCH 03/13] qcrypto-luks: refactoring: extract load/store/check/parse header functions
On Tue, 2019-08-20 at 20:01 +0200, Max Reitz wrote: > 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) Could you elaborate on that? I thought that code should not exceed 80 character limit. > > > Signed-off-by: Maxim Levitsky > > --- > > crypto/block-luks.c | 756 ++-- > > 1 file changed, 440 insertions(+), 316 deletions(-) > > Also, this commit is just too big. Yea, but it has no functional changes. I can split it further, but that won't help much IMHO. Best regards, Maxim Levitsky
Re: [Qemu-devel] [PATCH 03/13] qcrypto-luks: refactoring: extract load/store/check/parse header functions
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-devel] [PATCH 03/13] qcrypto-luks: refactoring: extract load/store/check/parse header functions
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;
+
+/*
+
