[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 04/13] qcrypto-luks: refactoring: simplify the m
From: |
Daniel P . Berrangé |
Subject: |
Re: [Qemu-devel] [PATCH 04/13] qcrypto-luks: refactoring: simplify the math used for keyslot locations |
Date: |
Thu, 22 Aug 2019 11:47:04 +0100 |
User-agent: |
Mutt/1.12.0 (2019-05-25) |
On Wed, Aug 14, 2019 at 11:22:10PM +0300, Maxim Levitsky wrote:
> Signed-off-by: Maxim Levitsky <address@hidden>
> ---
> 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 :|
- Re: [Qemu-devel] [Qemu-block] [PATCH 02/13] qcrypto-luks: misc refactoring, (continued)
- Re: [Qemu-devel] [Qemu-block] [PATCH 02/13] qcrypto-luks: misc refactoring, Maxim Levitsky, 2019/08/19
- Re: [Qemu-devel] [Qemu-block] [PATCH 02/13] qcrypto-luks: misc refactoring, Daniel P . Berrangé, 2019/08/22
- Re: [Qemu-devel] [Qemu-block] [PATCH 02/13] qcrypto-luks: misc refactoring, Maxim Levitsky, 2019/08/22
- Re: [Qemu-devel] [Qemu-block] [PATCH 02/13] qcrypto-luks: misc refactoring, Daniel P . Berrangé, 2019/08/22
- Re: [Qemu-devel] [Qemu-block] [PATCH 02/13] qcrypto-luks: misc refactoring, Maxim Levitsky, 2019/08/22
Re: [Qemu-devel] [PATCH 02/13] qcrypto-luks: misc refactoring, Max Reitz, 2019/08/20
[Qemu-devel] [PATCH 04/13] qcrypto-luks: refactoring: simplify the math used for keyslot locations, Maxim Levitsky, 2019/08/14
- Re: [Qemu-devel] [PATCH 04/13] qcrypto-luks: refactoring: simplify the math used for keyslot locations,
Daniel P . Berrangé <=
[Qemu-devel] [PATCH 05/13] qcrypto-luks: clear the masterkey and password before freeing them always, Maxim Levitsky, 2019/08/14
- Re: [Qemu-devel] [PATCH 05/13] qcrypto-luks: clear the masterkey and password before freeing them always, Max Reitz, 2019/08/20
- Re: [Qemu-devel] [PATCH 05/13] qcrypto-luks: clear the masterkey and password before freeing them always, Maxim Levitsky, 2019/08/21
- Re: [Qemu-devel] [PATCH 05/13] qcrypto-luks: clear the masterkey and password before freeing them always, Daniel P . Berrangé, 2019/08/22
- Re: [Qemu-devel] [PATCH 05/13] qcrypto-luks: clear the masterkey and password before freeing them always, Maxim Levitsky, 2019/08/22
- Re: [Qemu-devel] [PATCH 05/13] qcrypto-luks: clear the masterkey and password before freeing them always, Maxim Levitsky, 2019/08/25
- Re: [Qemu-devel] [PATCH 05/13] qcrypto-luks: clear the masterkey and password before freeing them always, Maxim Levitsky, 2019/08/25
- Re: [Qemu-devel] [PATCH 05/13] qcrypto-luks: clear the masterkey and password before freeing them always, Daniel P . Berrangé, 2019/08/27
Re: [Qemu-devel] [Qemu-block] [PATCH 05/13] qcrypto-luks: clear the masterkey and password before freeing them always, Nir Soffer, 2019/08/21