Re: [PATCH v3 9/9] cryptodisk: Properly handle non-512 byte sized sectors

[Daniel Kiper]

On Mon, Sep 21, 2020 at 05:58:06AM +0000, Glenn Washburn wrote:
> Sep 9, 2020 5:22:11 AM Daniel Kiper <daniel.kiper@oracle.com>:
>
> > On Mon, Sep 07, 2020 at 05:28:08PM +0200, Patrick Steinhardt wrote:
> >> From: Glenn Washburn <development@efficientek.com>
> >>
> >> By default, dm-crypt internally uses an IV that corresponds to 512-byte
> >> sectors, even when a larger sector size is specified. What this means is
> >> that when using a larger sector size, the IV is incremented every sector.
> >> However, the amount the IV is incremented is the number of 512 byte blocks
> >> in a sector (ie 8 for 4K sectors). Confusingly the IV does not corespond to
> >> the number of, for example, 4K sectors. So each cipher block in the fifth
> >> 4K sector will be encrypted with an IV equal to 32, as opposed to 32-39 for
> >
> > s/32-39/32-9/?
>
> I'm reading this and realizing it's worded badly and confusing. Perhaps this 
> sentence is better?
>
> "So each 512 byte cipher block in a sector will be encrypted with the
> same IV and the IV will be incremented afterwards by the number of 512
> byte cipher blocks in the sector."

Yeah, LGTM.

> >> each sequential 512 byte block or an IV of 4 for each cipher block in the
> >> sector.
> >>
> >> There are some encryption utilities which do it the intuitive way and have
> >> the IV equal to the sector number regardless of sector size (ie. the fifth
> >> sector would have an IV of 4 for each cipher block). And this is supported
> >> by dm-crypt with the iv_large_sectors option and also cryptsetup as of 
> >> 2.3.3
> >> with the --iv-large-sectors, though not with LUKS headers (only with --type
> >> plain). However, support for this has not been included as grub does not
> >> support plain devices right now.
> >>
> >> One gotcha here is that the encrypted split keys are encrypted with a hard-
> >> coded 512-byte sector size. So even if your data is encrypted with 4K 
> >> sector
> >> sizes, the split key encrypted area must be decrypted with a block size of
> >> 512 (ie the IV increments every 512 bytes). This made these changes less
> >> aestetically pleasing than desired.
> >>
> >> Signed-off-by: Glenn Washburn <development@efficientek.com>
> >> Reviewed-by: Patrick Steinhardt <ps@pks.im>
> >> ---
> >> grub-core/disk/cryptodisk.c | 44 ++++++++++++++++++++-----------------
> >> grub-core/disk/luks.c       |  5 +++--
> >> grub-core/disk/luks2.c      |  7 +++++-
> >> include/grub/cryptodisk.h   |  9 +++++++-
> >> 4 files changed, 41 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> >> index 0b63b7d96..6319f3164 100644
> >> --- a/grub-core/disk/cryptodisk.c
> >> +++ b/grub-core/disk/cryptodisk.c
> >> @@ -224,7 +224,8 @@ lrw_xor (const struct lrw_sector *sec,
> >> static gcry_err_code_t
> >> grub_cryptodisk_endecrypt (struct grub_cryptodisk *dev,
> >> grub_uint8_t * data, grub_size_t len,
> >> -        grub_disk_addr_t sector, int do_encrypt)
> >> +        grub_disk_addr_t sector, grub_size_t log_sector_size,
> >> +        int do_encrypt)
> >> {
> >> grub_size_t i;
> >> gcry_err_code_t err;
> >> @@ -237,12 +238,13 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk 
> >> *dev,
> >> return (do_encrypt ? grub_crypto_ecb_encrypt (dev->cipher, data, data, len)
> >> : grub_crypto_ecb_decrypt (dev->cipher, data, data, len));
> >>
> >> -  for (i = 0; i < len; i += (1U << dev->log_sector_size))
> >> +  for (i = 0; i < len; i += (1U << log_sector_size))
> >> {
> >> grub_size_t sz = ((dev->cipher->cipher->blocksize
> >> + sizeof (grub_uint32_t) - 1)
> >> / sizeof (grub_uint32_t));
> >> grub_uint32_t iv[(GRUB_CRYPTO_MAX_CIPHER_BLOCKSIZE + 3) / 4];
> >> +      grub_uint64_t iv_calc = 0;
> >>
> >> if (dev->rekey)
> >> {
> >> @@ -270,7 +272,7 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk *dev,
> >> if (!ctx)
> >> return GPG_ERR_OUT_OF_MEMORY;
> >>
> >> -     tmp = grub_cpu_to_le64 (sector << dev->log_sector_size);
> >> +     tmp = grub_cpu_to_le64 (sector << log_sector_size);
> >> dev->iv_hash->init (ctx);
> >> dev->iv_hash->write (ctx, dev->iv_prefix, dev->iv_prefix_len);
> >> dev->iv_hash->write (ctx, &tmp, sizeof (tmp));
> >> @@ -281,14 +283,16 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk 
> >> *dev,
> >> }
> >> break;
> >> case GRUB_CRYPTODISK_MODE_IV_PLAIN64:
> >> -   iv[1] = grub_cpu_to_le32 (sector >> 32);
> >> +   iv_calc = sector << (log_sector_size - GRUB_CRYPTODISK_IV_LOG_SIZE);
> >> +   iv[1] = grub_cpu_to_le32 (iv_calc >> 32);
> >
> > Why 32? Could you use a constant or add a comment here?
>
> Plain mode uses a 32 bit IV and plain64 uses a 64 bit IV. So in the
> plain64 case only deal with the non-plain (ie not lower 32 bits) IV
> bits and we deal with the plain case in the fall through.
>
> I don't think a constant is warranted and I can add in a comment in
> this patch, but I'd like to point out that the "32" literal you're
> commenting on is not code I've introduced. As such, the comment would
> not be relevant to this patch. Given that, do you still want a comment
> in this patch?

If you touch this code I want it fixed this way or another, If you think
comment should be added in separate patch and/or in different place I am
OK with it.

> >> /* FALLTHROUGH */
> >> case GRUB_CRYPTODISK_MODE_IV_PLAIN:
> >> -   iv[0] = grub_cpu_to_le32 (sector & 0xFFFFFFFF);
> >> +   iv_calc = sector << (log_sector_size - GRUB_CRYPTODISK_IV_LOG_SIZE);
> >> +   iv[0] = grub_cpu_to_le32 (iv_calc & 0xFFFFFFFF);
> >> break;
> >> case GRUB_CRYPTODISK_MODE_IV_BYTECOUNT64:
> >> -   iv[1] = grub_cpu_to_le32 (sector >> (32 - dev->log_sector_size));
> >
> > Ditto?
>
> For modes other than plain and plain64, all that is being done is
> substituting "dev->log_sector_size" for "log_sector_size". Again,
> those constant "32" integers are not code that I've added and I don't
> think comments are relevant to this patch.
>
> Here the "32" is used to create the IV in 2 32bit chunks because the
> variable "iv" is an array of 32bit uints.

As above, if you think separate patch is better go for it...

> >> -   iv[0] = grub_cpu_to_le32 ((sector << dev->log_sector_size)
> >> +   iv[1] = grub_cpu_to_le32 (sector >> (32 - log_sector_size));
> >
> > Ditto?
> >
> > [...]
> >
> >> diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> >> index 59702067a..2e1347b13 100644
> >> --- a/grub-core/disk/luks.c
> >> +++ b/grub-core/disk/luks.c
> >> @@ -124,7 +124,7 @@ configure_ciphers (grub_disk_t disk, const char 
> >> *check_uuid,
> >> return NULL;
> >> newdev->offset = grub_be_to_cpu32 (header.payloadOffset);
> >> newdev->source_disk = NULL;
> >> -  newdev->log_sector_size = 9;
> >> +  newdev->log_sector_size = LUKS_LOG_SECTOR_SIZE;
> >> newdev->total_length = grub_disk_get_size (disk) - newdev->offset;
> >> grub_memcpy (newdev->uuid, uuid, sizeof (uuid));
> >> newdev->modname = "luks";
> >> @@ -247,7 +247,8 @@ luks_recover_key (grub_disk_t source,
> >> return err;
> >> }
> >>
> >> -      gcry_err = grub_cryptodisk_decrypt (dev, split_key, length, 0);
> >> +      gcry_err = grub_cryptodisk_decrypt (dev, split_key, length, 0,
> >> +           LUKS_LOG_SECTOR_SIZE);
> >> if (gcry_err)
> >> {
> >> grub_free (split_key);
> >> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> >> index 26e1126b1..eb64a0596 100644
> >> --- a/grub-core/disk/luks2.c
> >> +++ b/grub-core/disk/luks2.c
> >> @@ -492,7 +492,12 @@ luks2_decrypt_key (grub_uint8_t *out_key,
> >> goto err;
> >> }
> >>
> >> -  gcry_ret = grub_cryptodisk_decrypt (crypt, split_key, k->area.size, 0);
> >> +  /*
> >> +   * The key slots area is always encrypted in 512-byte sectors,
> >> +   * regardless of encrypted data sector size.
> >> +   */
> >> +  gcry_ret = grub_cryptodisk_decrypt (crypt, split_key, k->area.size, 0,
> >> +             LUKS_LOG_SECTOR_SIZE);
> >
> > s/LUKS_LOG_SECTOR_SIZE/GRUB_CRYPTODISK_IV_LOG_SIZE/?
>
> LUKS_LOG_SECTOR_SIZE and GRUB_CRYPTODISK_IV_LOG_SIZE are the same
> number, but they represent different things. According to the luks2
> spec the key slot area is decrypted as specified in the luks1 spec. So
> perhaps the constant should be renamed to LUKS1_LOG_SECTOR_SIZE,
> because luks2 does not have a constant sector size.
>
> The constant GRUB_CRYPTODISK_IV_LOG_SIZE is meant to represent the log
> of the sector size that dm-crypt uses natively for incrementing the
> IV.

Please add similar comments to places where these constants are defined.

Daniel