Re: [Qemu-devel] [PATCH v2 11/13] qcrypto-luks: refactoring: simplify the math used for keyslot locations

2019-09-12 Thread Maxim Levitsky 
On Fri, 2019-09-06 at 14:17 +0100, Daniel P. Berrangé wrote:
> On Mon, Aug 26, 2019 at 04:51:01PM +0300, Maxim Levitsky wrote:
> > Signed-off-by: Maxim Levitsky 
> > ---
> >  crypto/block-luks.c | 64 +
> >  1 file changed, 41 insertions(+), 23 deletions(-)
> > 
> > diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> > index d713125925..6a43d97ce5 100644
> > --- a/crypto/block-luks.c
> > +++ b/crypto/block-luks.c
> > @@ -409,6 +409,32 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm 
> > cipher,
> >  }
> >  }
> >  
> > +/*
> > + * Returns number of sectors needed to store the key material
> > + * given number of anti forensic stripes
> > + */
> > +static int
> > +qcrypto_block_luks_splitkeylen_sectors(const QCryptoBlockLUKS *luks,
> > +   unsigned 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;
> 
> The caller already calculated that so just pass it in

All right, no problem.

> 
> > +
> > +size_t splitkeylen = luks->header.master_key_len * 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
> >   */
> > @@ -1151,7 +1177,8 @@ qcrypto_block_luks_create(QCryptoBlock *block,
> >  QCryptoBlockCreateOptionsLUKS luks_opts;
> >  Error *local_err = NULL;
> >  g_autofree uint8_t *masterkey = NULL;
> > -size_t splitkeylen = 0;
> > +size_t header_sectors;
> > +size_t split_key_sectors;
> >  size_t i;
> >  g_autofree char *password;
> >  const char *cipher_alg;
> > @@ -1370,37 +1397,28 @@ qcrypto_block_luks_create(QCryptoBlock *block,
> >  goto error;
> >  }
> >  
> > +/* start with the sector that follows the header*/
> > +header_sectors = QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
> > +QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
> > +
> > +split_key_sectors =
> > +qcrypto_block_luks_splitkeylen_sectors(luks,
> > +   QCRYPTO_BLOCK_LUKS_STRIPES);
> >  
> > -/* Although LUKS has multiple key slots, we're just going
> > - * to use the first key slot */
> > -splitkeylen = luks->header.master_key_len * 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;
> > +QCryptoBlockLUKSKeySlot *slot = &luks->header.key_slots[i];
> > +slot->active = QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED;
> >  
> > -/* This calculation doesn't match that shown in the spec,
> > - * but instead follows the cryptsetup implementation.
> > - */
> > -luks->header.key_slots[i].key_offset_sector =
> > -(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);
> > +slot->key_offset_sector = header_sectors + i * split_key_sectors;
> > +slot->stripes = QCRYPTO_BLOCK_LUKS_STRIPES;
> >  }
> >  
> > -
> >  /* The total size of the LUKS headers is the partition header + key
> >   * 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_sector =
> > -(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_sector = header_sectors +
> > +QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS * split_key_sectors;
> >  
> >  block->sector_size = QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
> >  block->payload_offset = luks->header.payload_offset_sector *
> 
> Reviewed-by: Daniel P. Berrangé 
> 
> Regards,
> Daniel

Best regards,
Maxim Levitsky

Re: [Qemu-devel] [PATCH v2 11/13] qcrypto-luks: refactoring: simplify the math used for keyslot locations

2019-09-06 Thread Daniel P . Berrangé 
On Mon, Aug 26, 2019 at 04:51:01PM +0300, Maxim Levitsky wrote:
> Signed-off-by: Maxim Levitsky 
> ---
>  crypto/block-luks.c | 64 +
>  1 file changed, 41 insertions(+), 23 deletions(-)
> 
> diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> index d713125925..6a43d97ce5 100644
> --- a/crypto/block-luks.c
> +++ b/crypto/block-luks.c
> @@ -409,6 +409,32 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm 
> cipher,
>  }
>  }
>  
> +/*
> + * Returns number of sectors needed to store the key material
> + * given number of anti forensic stripes
> + */
> +static int
> +qcrypto_block_luks_splitkeylen_sectors(const QCryptoBlockLUKS *luks,
> +   unsigned 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;

The caller already calculated that so just pass it in

> +
> +size_t splitkeylen = luks->header.master_key_len * 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
>   */
> @@ -1151,7 +1177,8 @@ qcrypto_block_luks_create(QCryptoBlock *block,
>  QCryptoBlockCreateOptionsLUKS luks_opts;
>  Error *local_err = NULL;
>  g_autofree uint8_t *masterkey = NULL;
> -size_t splitkeylen = 0;
> +size_t header_sectors;
> +size_t split_key_sectors;
>  size_t i;
>  g_autofree char *password;
>  const char *cipher_alg;
> @@ -1370,37 +1397,28 @@ qcrypto_block_luks_create(QCryptoBlock *block,
>  goto error;
>  }
>  
> +/* start with the sector that follows the header*/
> +header_sectors = QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
> +QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
> +
> +split_key_sectors =
> +qcrypto_block_luks_splitkeylen_sectors(luks,
> +   QCRYPTO_BLOCK_LUKS_STRIPES);
>  
> -/* Although LUKS has multiple key slots, we're just going
> - * to use the first key slot */
> -splitkeylen = luks->header.master_key_len * 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;
> +QCryptoBlockLUKSKeySlot *slot = &luks->header.key_slots[i];
> +slot->active = QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED;
>  
> -/* This calculation doesn't match that shown in the spec,
> - * but instead follows the cryptsetup implementation.
> - */
> -luks->header.key_slots[i].key_offset_sector =
> -(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);
> +slot->key_offset_sector = header_sectors + i * split_key_sectors;
> +slot->stripes = QCRYPTO_BLOCK_LUKS_STRIPES;
>  }
>  
> -
>  /* The total size of the LUKS headers is the partition header + key
>   * 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_sector =
> -(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_sector = header_sectors +
> +QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS * split_key_sectors;
>  
>  block->sector_size = QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
>  block->payload_offset = luks->header.payload_offset_sector *

Reviewed-by: Daniel P. Berrangé 

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

[Qemu-devel] [PATCH v2 11/13] qcrypto-luks: refactoring: simplify the math used for keyslot locations

2019-08-26 Thread Maxim Levitsky 
Signed-off-by: Maxim Levitsky 
---
 crypto/block-luks.c | 64 +
 1 file changed, 41 insertions(+), 23 deletions(-)

diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index d713125925..6a43d97ce5 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -409,6 +409,32 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm 
cipher,
 }
 }
 
+/*
+ * Returns number of sectors needed to store the key material
+ * given number of anti forensic stripes
+ */
+static int
+qcrypto_block_luks_splitkeylen_sectors(const QCryptoBlockLUKS *luks,
+   unsigned 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 = luks->header.master_key_len * 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
  */
@@ -1151,7 +1177,8 @@ qcrypto_block_luks_create(QCryptoBlock *block,
 QCryptoBlockCreateOptionsLUKS luks_opts;
 Error *local_err = NULL;
 g_autofree uint8_t *masterkey = NULL;
-size_t splitkeylen = 0;
+size_t header_sectors;
+size_t split_key_sectors;
 size_t i;
 g_autofree char *password;
 const char *cipher_alg;
@@ -1370,37 +1397,28 @@ qcrypto_block_luks_create(QCryptoBlock *block,
 goto error;
 }
 
+/* start with the sector that follows the header*/
+header_sectors = QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET /
+QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
+
+split_key_sectors =
+qcrypto_block_luks_splitkeylen_sectors(luks,
+   QCRYPTO_BLOCK_LUKS_STRIPES);
 
-/* Although LUKS has multiple key slots, we're just going
- * to use the first key slot */
-splitkeylen = luks->header.master_key_len * 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;
+QCryptoBlockLUKSKeySlot *slot = &luks->header.key_slots[i];
+slot->active = QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED;
 
-/* This calculation doesn't match that shown in the spec,
- * but instead follows the cryptsetup implementation.
- */
-luks->header.key_slots[i].key_offset_sector =
-(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);
+slot->key_offset_sector = header_sectors + i * split_key_sectors;
+slot->stripes = QCRYPTO_BLOCK_LUKS_STRIPES;
 }
 
-
 /* The total size of the LUKS headers is the partition header + key
  * 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_sector =
-(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_sector = header_sectors +
+QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS * split_key_sectors;
 
 block->sector_size = QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
 block->payload_offset = luks->header.payload_offset_sector *
-- 
2.17.2