Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 crypto/block-luks.c | 64 +++++++++++++++++++++++++++------------------
 1 file changed, 38 insertions(+), 26 deletions(-)

diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 6bb369f3b4..e1a4df94b7 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -417,6 +417,33 @@ static int masterkeylen(QCryptoBlockLUKS *luks)
 }
 
 
+/*
+ * Returns number of sectors needed to store the key material
+ * given number of anti forensic stripes
+ */
+static int splitkeylen_sectors(QCryptoBlockLUKS *luks, int stripes)
+
+{
+    /*
+     * This calculation doesn't match that shown in the spec,
+     * but instead follows the cryptsetup implementation.
+     */
+
+    size_t header_sectors = QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
+                     QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
+
+    size_t splitkeylen = masterkeylen(luks) * stripes;
+
+    /* First align the key material size to block size*/
+    size_t splitkeylen_sectors =
+            DIV_ROUND_UP(splitkeylen, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE);
+
+    /* Then also align the key material size to the size of the header */
+    return ROUND_UP(splitkeylen_sectors, header_sectors);
+}
+
+
+
 /*
  * Stores the main LUKS header, taking care of endianess
  */
@@ -1169,7 +1196,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
     QCryptoBlockCreateOptionsLUKS luks_opts;
     Error *local_err = NULL;
     uint8_t *masterkey = NULL;
-    size_t splitkeylen = 0;
+    size_t next_sector;
     size_t i;
     char *password;
     const char *cipher_alg;
@@ -1388,23 +1415,16 @@ qcrypto_block_luks_create(QCryptoBlock *block,
         goto error;
     }
 
+    /* start with the sector that follows the header*/
+    next_sector = QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
+                  QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
 
-    /* Although LUKS has multiple key slots, we're just going
-     * to use the first key slot */
-    splitkeylen = luks->header.key_bytes * QCRYPTO_BLOCK_LUKS_STRIPES;
     for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
-        luks->header.key_slots[i].active = QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED;
-        luks->header.key_slots[i].stripes = QCRYPTO_BLOCK_LUKS_STRIPES;
-
-        /* This calculation doesn't match that shown in the spec,
-         * but instead follows the cryptsetup implementation.
-         */
-        luks->header.key_slots[i].key_offset =
-            (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
-             QCRYPTO_BLOCK_LUKS_SECTOR_SIZE) +
-            (ROUND_UP(DIV_ROUND_UP(splitkeylen, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE),
-                      (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
-                       QCRYPTO_BLOCK_LUKS_SECTOR_SIZE)) * i);
+        QCryptoBlockLUKSKeySlot *slot = &luks->header.key_slots[i];
+        slot->active = QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED;
+        slot->key_offset = next_sector;
+        slot->stripes = QCRYPTO_BLOCK_LUKS_STRIPES;
+        next_sector += splitkeylen_sectors(luks, QCRYPTO_BLOCK_LUKS_STRIPES);
     }
 
 
@@ -1412,17 +1432,9 @@ qcrypto_block_luks_create(QCryptoBlock *block,
      * slot headers, rounded up to the nearest sector, combined with
      * the size of each master key material region, also rounded up
      * to the nearest sector */
-    luks->header.payload_offset =
-        (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
-         QCRYPTO_BLOCK_LUKS_SECTOR_SIZE) +
-        (ROUND_UP(DIV_ROUND_UP(splitkeylen, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE),
-                  (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
-                   QCRYPTO_BLOCK_LUKS_SECTOR_SIZE)) *
-         QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS);
-
+    luks->header.payload_offset = next_sector;
     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;
 
     /* Reserve header space to match payload offset */
     initfunc(block, block->payload_offset, opaque, &local_err);
-- 
2.17.2

On Wed, Aug 14, 2019 at 11:22:10PM +0300, Maxim Levitsky wrote:
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  crypto/block-luks.c | 64 +++++++++++++++++++++++++++------------------
>  1 file changed, 38 insertions(+), 26 deletions(-)
> 
> diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> index 6bb369f3b4..e1a4df94b7 100644
> --- a/crypto/block-luks.c
> +++ b/crypto/block-luks.c
> @@ -417,6 +417,33 @@ static int masterkeylen(QCryptoBlockLUKS *luks)
>  }
>  
>  
> +/*
> + * Returns number of sectors needed to store the key material
> + * given number of anti forensic stripes
> + */
> +static int splitkeylen_sectors(QCryptoBlockLUKS *luks, int stripes)

Needs a qcrypto_block_luks_ prefix on method name.

I'd also put 'static int' on a separate line from method name
to reduce too long lines.

> +
> +{
> +    /*
> +     * This calculation doesn't match that shown in the spec,
> +     * but instead follows the cryptsetup implementation.
> +     */
> +
> +    size_t header_sectors = QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
> +                     QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;

Following line indent should only be 4 spaces

> +
> +    size_t splitkeylen = masterkeylen(luks) * stripes;
> +
> +    /* First align the key material size to block size*/
> +    size_t splitkeylen_sectors =
> +            DIV_ROUND_UP(splitkeylen, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE);

Again 4 space indent.

> +
> +    /* Then also align the key material size to the size of the header */
> +    return ROUND_UP(splitkeylen_sectors, header_sectors);
> +}
> +
> +
> +
>  /*
>   * Stores the main LUKS header, taking care of endianess
>   */
> @@ -1169,7 +1196,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
>      QCryptoBlockCreateOptionsLUKS luks_opts;
>      Error *local_err = NULL;
>      uint8_t *masterkey = NULL;
> -    size_t splitkeylen = 0;
> +    size_t next_sector;
>      size_t i;
>      char *password;
>      const char *cipher_alg;
> @@ -1388,23 +1415,16 @@ qcrypto_block_luks_create(QCryptoBlock *block,
>          goto error;
>      }
>  
> +    /* start with the sector that follows the header*/
> +    next_sector = QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
> +                  QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;

I'd suggest 'post_header_sector'

>  
> -    /* Although LUKS has multiple key slots, we're just going
> -     * to use the first key slot */
> -    splitkeylen = luks->header.key_bytes * QCRYPTO_BLOCK_LUKS_STRIPES;
>      for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
> -        luks->header.key_slots[i].active = QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED;
> -        luks->header.key_slots[i].stripes = QCRYPTO_BLOCK_LUKS_STRIPES;
> -
> -        /* This calculation doesn't match that shown in the spec,
> -         * but instead follows the cryptsetup implementation.
> -         */
> -        luks->header.key_slots[i].key_offset =
> -            (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
> -             QCRYPTO_BLOCK_LUKS_SECTOR_SIZE) +
> -            (ROUND_UP(DIV_ROUND_UP(splitkeylen, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE),
> -                      (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
> -                       QCRYPTO_BLOCK_LUKS_SECTOR_SIZE)) * i);
> +        QCryptoBlockLUKSKeySlot *slot = &luks->header.key_slots[i];
> +        slot->active = QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED;
> +        slot->key_offset = next_sector;
> +        slot->stripes = QCRYPTO_BLOCK_LUKS_STRIPES;
> +        next_sector += splitkeylen_sectors(luks, QCRYPTO_BLOCK_LUKS_STRIPES);

I'm not a fan of the next_sector accumulator .

I'd prefer to see the '* i' part done in splitkeylen_sectors, so that
we have

  slot->key_offset = post_header_sector +
        splitkeylen_sectors(luks, QCRYPTO_BLOCK_LUKS_STRIPES, i);

> @@ -1412,17 +1432,9 @@ qcrypto_block_luks_create(QCryptoBlock *block,
>       * slot headers, rounded up to the nearest sector, combined with
>       * the size of each master key material region, also rounded up
>       * to the nearest sector */
> -    luks->header.payload_offset =
> -        (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
> -         QCRYPTO_BLOCK_LUKS_SECTOR_SIZE) +
> -        (ROUND_UP(DIV_ROUND_UP(splitkeylen, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE),
> -                  (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
> -                   QCRYPTO_BLOCK_LUKS_SECTOR_SIZE)) *
> -         QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS);
> -
> +    luks->header.payload_offset = next_sector;

  luks->header.payload_offset = post_header_sector +
        splitkeylen_sectors(luks, QCRYPTO_BLOCK_LUKS_STRIPES,
	                    QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS);


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


This is reverting a whitspace change done in previous 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 :|

On Thu, 2019-08-22 at 11:47 +0100, Daniel P. Berrangé wrote:
> On Wed, Aug 14, 2019 at 11:22:10PM +0300, Maxim Levitsky wrote:
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  crypto/block-luks.c | 64 +++++++++++++++++++++++++++------------------
> >  1 file changed, 38 insertions(+), 26 deletions(-)
> > 
> > diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> > index 6bb369f3b4..e1a4df94b7 100644
> > --- a/crypto/block-luks.c
> > +++ b/crypto/block-luks.c
> > @@ -417,6 +417,33 @@ static int masterkeylen(QCryptoBlockLUKS *luks)
> >  }
> >  
> >  
> > +/*
> > + * Returns number of sectors needed to store the key material
> > + * given number of anti forensic stripes
> > + */
> > +static int splitkeylen_sectors(QCryptoBlockLUKS *luks, int stripes)
> 
> Needs a qcrypto_block_luks_ prefix on method name.
Done.

> 
> I'd also put 'static int' on a separate line from method name
> to reduce too long lines.
Done.
> 
> > +
> > +{
> > +    /*
> > +     * This calculation doesn't match that shown in the spec,
> > +     * but instead follows the cryptsetup implementation.
> > +     */
> > +
> > +    size_t header_sectors = QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
> > +                     QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
> 
> Following line indent should only be 4 spaces
I didn't knew that. Fixed.
> 
> > +
> > +    size_t splitkeylen = masterkeylen(luks) * stripes;
> > +
> > +    /* First align the key material size to block size*/
> > +    size_t splitkeylen_sectors =
> > +            DIV_ROUND_UP(splitkeylen, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE);
> 
> Again 4 space indent.
> 
> > +
> > +    /* Then also align the key material size to the size of the header */
> > +    return ROUND_UP(splitkeylen_sectors, header_sectors);
> > +}
> > +
> > +
> > +
> >  /*
> >   * Stores the main LUKS header, taking care of endianess
> >   */
> > @@ -1169,7 +1196,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
> >      QCryptoBlockCreateOptionsLUKS luks_opts;
> >      Error *local_err = NULL;
> >      uint8_t *masterkey = NULL;
> > -    size_t splitkeylen = 0;
> > +    size_t next_sector;
> >      size_t i;
> >      char *password;
> >      const char *cipher_alg;
> > @@ -1388,23 +1415,16 @@ qcrypto_block_luks_create(QCryptoBlock *block,
> >          goto error;
> >      }
> >  
> > +    /* start with the sector that follows the header*/
> > +    next_sector = QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
> > +                  QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
> 
> I'd suggest 'post_header_sector'
I called it now header_sectors, and each split key, split_key_sectors.
I hope that this is good enough.
> 
> >  
> > -    /* Although LUKS has multiple key slots, we're just going
> > -     * to use the first key slot */
> > -    splitkeylen = luks->header.key_bytes * QCRYPTO_BLOCK_LUKS_STRIPES;
> >      for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
> > -        luks->header.key_slots[i].active = QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED;
> > -        luks->header.key_slots[i].stripes = QCRYPTO_BLOCK_LUKS_STRIPES;
> > -
> > -        /* This calculation doesn't match that shown in the spec,
> > -         * but instead follows the cryptsetup implementation.
> > -         */
> > -        luks->header.key_slots[i].key_offset =
> > -            (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
> > -             QCRYPTO_BLOCK_LUKS_SECTOR_SIZE) +
> > -            (ROUND_UP(DIV_ROUND_UP(splitkeylen, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE),
> > -                      (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
> > -                       QCRYPTO_BLOCK_LUKS_SECTOR_SIZE)) * i);
> > +        QCryptoBlockLUKSKeySlot *slot = &luks->header.key_slots[i];
> > +        slot->active = QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED;
> > +        slot->key_offset = next_sector;
> > +        slot->stripes = QCRYPTO_BLOCK_LUKS_STRIPES;
> > +        next_sector += splitkeylen_sectors(luks, QCRYPTO_BLOCK_LUKS_STRIPES);
> 
> I'm not a fan of the next_sector accumulator .
I actually think that accumulator is cleaner here,
but I won't argue about this. Fixed.


> 
> I'd prefer to see the '* i' part done in splitkeylen_sectors, so that
> we have
> 
>   slot->key_offset = post_header_sector +
>         splitkeylen_sectors(luks, QCRYPTO_BLOCK_LUKS_STRIPES, i);
> 
> > @@ -1412,17 +1432,9 @@ qcrypto_block_luks_create(QCryptoBlock *block,
> >       * slot headers, rounded up to the nearest sector, combined with
> >       * the size of each master key material region, also rounded up
> >       * to the nearest sector */
> > -    luks->header.payload_offset =
> > -        (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
> > -         QCRYPTO_BLOCK_LUKS_SECTOR_SIZE) +
> > -        (ROUND_UP(DIV_ROUND_UP(splitkeylen, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE),
> > -                  (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
> > -                   QCRYPTO_BLOCK_LUKS_SECTOR_SIZE)) *
> > -         QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS);
> > -
> > +    luks->header.payload_offset = next_sector;
> 
>   luks->header.payload_offset = post_header_sector +
>         splitkeylen_sectors(luks, QCRYPTO_BLOCK_LUKS_STRIPES,
> 	                    QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS);
> 
> 
> >      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;
> 
> 
> This is reverting a whitspace change done in previous method
True. Fixed.

> 
> Regards,
> Daniel


Best regards,
	Maxim Levitsky