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

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Aug 26, 2019 at 04:51:01PM +0300, Maxim Levitsky wrote:
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  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é <berrange@redhat.com>

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