QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Maxim Levitsky <mlevitsk@redhat.com>
To: "Daniel P. Berrangé" <berrange@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>, Fam Zheng <fam@euphon.net>,
	qemu-block@nongnu.org, Markus Armbruster <armbru@redhat.com>,
	qemu-devel@nongnu.org, Max Reitz <mreitz@redhat.com>,
	Stefan Hajnoczi <stefanha@redhat.com>
Subject: Re: [Qemu-devel] [PATCH 04/13] qcrypto-luks: refactoring: simplify the math used for keyslot locations
Date: Sun, 25 Aug 2019 17:30:20 +0300
Message-ID: <7cf4bde6ca7bb74adf1dfc40e2d1ad61b83c6b28.camel@redhat.com> (raw)
In-Reply-To: <20190822104704.GI3267@redhat.com>

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



  reply index

Thread overview: 87+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-14 20:22 [Qemu-devel] [PATCH 00/13] RFC: luks/encrypted qcow2 key management Maxim Levitsky
2019-08-14 20:22 ` [Qemu-devel] [PATCH 01/13] block-crypto: misc refactoring Maxim Levitsky
2019-08-20 16:38   ` Max Reitz
2019-08-22  0:05     ` Maxim Levitsky
2019-08-22 14:34       ` Max Reitz
2019-08-22 15:04         ` Maxim Levitsky
2019-08-21 15:39   ` Daniel P. Berrangé
2019-08-22  0:08     ` Maxim Levitsky
2019-08-14 20:22 ` [Qemu-devel] [PATCH 02/13] qcrypto-luks: " Maxim Levitsky
2019-08-15 21:40   ` [Qemu-devel] [Qemu-block] " John Snow
2019-08-19 14:21     ` Maxim Levitsky
2019-08-22 10:29     ` Daniel P. Berrangé
2019-08-22 11:04       ` Maxim Levitsky
2019-08-22 11:10         ` Daniel P. Berrangé
2019-08-22 11:13           ` Maxim Levitsky
2019-08-20 17:36   ` [Qemu-devel] " Max Reitz
2019-08-21 23:59     ` Maxim Levitsky
2019-08-22 14:32       ` Max Reitz
2019-08-25 10:46         ` Maxim Levitsky
2019-08-14 20:22 ` [Qemu-devel] [PATCH 03/13] qcrypto-luks: refactoring: extract load/store/check/parse header functions Maxim Levitsky
2019-08-20 18:01   ` Max Reitz
2019-08-21 22:43     ` Maxim Levitsky
2019-08-22 10:32       ` Daniel P. Berrangé
2019-08-22 10:57         ` Maxim Levitsky
2019-08-22 10:34       ` Daniel P. Berrangé
2019-08-25 14:11         ` Maxim Levitsky
2019-08-22 10:38   ` Daniel P. Berrangé
2019-08-25 14:09     ` Maxim Levitsky
2019-08-14 20:22 ` [Qemu-devel] [PATCH 04/13] qcrypto-luks: refactoring: simplify the math used for keyslot locations Maxim Levitsky
2019-08-22 10:47   ` Daniel P. Berrangé
2019-08-25 14:30     ` Maxim Levitsky [this message]
2019-08-14 20:22 ` [Qemu-devel] [PATCH 05/13] qcrypto-luks: clear the masterkey and password before freeing them always Maxim Levitsky
2019-08-20 18:12   ` Max Reitz
2019-08-21 22:40     ` Maxim Levitsky
2019-08-22 10:49     ` Daniel P. Berrangé
2019-08-22 10:56       ` Maxim Levitsky
2019-08-25 15:31         ` Maxim Levitsky
2019-08-25 17:15           ` Maxim Levitsky
2019-08-27  8:55           ` Daniel P. Berrangé
2019-08-21 23:01   ` [Qemu-devel] [Qemu-block] " Nir Soffer
2019-08-21 23:11     ` Maxim Levitsky
2019-08-14 20:22 ` [Qemu-devel] [PATCH 06/13] qcrypto-luks: implement more rigorous header checking Maxim Levitsky
2019-08-22 11:04   ` Daniel P. Berrangé
2019-08-25 15:40     ` Maxim Levitsky
2019-08-25 16:08       ` Maxim Levitsky
2019-08-26 13:31         ` Eric Blake
2019-08-26 13:39           ` Maxim Levitsky
2019-08-27  8:56         ` Daniel P. Berrangé
2019-08-14 20:22 ` [Qemu-devel] [PATCH 07/13] block: add manage-encryption command (qmp and blockdev) Maxim Levitsky
2019-08-20 18:27   ` Max Reitz
2019-08-21 22:32     ` Maxim Levitsky
2019-08-22 11:14     ` Daniel P. Berrangé
2019-08-21 11:47   ` Markus Armbruster
2019-08-21 22:24     ` Maxim Levitsky
2019-08-22 14:07       ` Markus Armbruster
2019-08-25 16:42         ` Maxim Levitsky
2019-08-14 20:22 ` [Qemu-devel] [PATCH 08/13] qcrypto: add the plumbing for encryption management Maxim Levitsky
2019-08-22 11:16   ` Daniel P. Berrangé
2019-08-22 11:47     ` Maxim Levitsky
2019-08-22 11:49       ` Daniel P. Berrangé
2019-08-14 20:22 ` [Qemu-devel] [PATCH 09/13] qcrypto-luks: implement the encryption key management Maxim Levitsky
2019-08-22 11:27   ` Daniel P. Berrangé
2019-08-25 17:01     ` Maxim Levitsky
2019-08-14 20:22 ` [Qemu-devel] [PATCH 10/13] block/crypto: " Maxim Levitsky
2019-08-22 11:29   ` Daniel P. Berrangé
2019-08-22 11:36     ` Maxim Levitsky
2019-08-14 20:22 ` [Qemu-devel] [PATCH 11/13] block/qcow2: implement the encryption key managment Maxim Levitsky
2019-08-14 20:22 ` [Qemu-devel] [PATCH 12/13] qemu-img: implement key management Maxim Levitsky
2019-08-20 18:29   ` Max Reitz
2019-08-21 22:33     ` Maxim Levitsky
2019-08-22 11:32     ` Daniel P. Berrangé
2019-08-22 14:42       ` Max Reitz
2019-08-25 17:04         ` Maxim Levitsky
2019-08-14 20:22 ` [Qemu-devel] [PATCH 13/13] iotests : add tests for encryption " Maxim Levitsky
2019-08-14 21:08 ` [Qemu-devel] [PATCH 00/13] RFC: luks/encrypted qcow2 " Eric Blake
2019-08-15  8:49   ` Maxim Levitsky
2019-08-15  9:10   ` Kevin Wolf
2019-08-15 14:18     ` Markus Armbruster
2019-08-15 14:44       ` Maxim Levitsky
2019-08-15 15:00         ` Eric Blake
2019-08-19 12:35           ` Maxim Levitsky
2019-08-21 11:31             ` Markus Armbruster
2019-08-21 13:22               ` Maxim Levitsky
2019-08-20 17:59 ` Max Reitz
2019-08-21 22:00   ` Maxim Levitsky
2019-08-22 11:35 ` Daniel P. Berrangé
2019-08-25 17:10   ` Maxim Levitsky

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7cf4bde6ca7bb74adf1dfc40e2d1ad61b83c6b28.camel@redhat.com \
    --to=mlevitsk@redhat.com \
    --cc=armbru@redhat.com \
    --cc=berrange@redhat.com \
    --cc=fam@euphon.net \
    --cc=kwolf@redhat.com \
    --cc=mreitz@redhat.com \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=stefanha@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git