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

[Glenn Washburn]

On Fri, 30 Oct 2020 21:47:14 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Mon, Oct 19, 2020 at 06:09:54PM -0500, Glenn Washburn wrote:
> > 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 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.
> >
> > 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>
> > ---
> >  grub-core/disk/cryptodisk.c | 52
> > ++++++++++++++++++++++--------------- grub-core/disk/luks.c       |
> >  5 ++-- grub-core/disk/luks2.c      |  7 ++++-
> >  include/grub/cryptodisk.h   |  8 +++++-
> >  4 files changed, 47 insertions(+), 25 deletions(-)
> >
> > diff --git a/grub-core/disk/cryptodisk.c
> > b/grub-core/disk/cryptodisk.c index a3d672f68..623f0f396 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,7 +238,7 @@ 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)
> > @@ -270,7 +271,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 +282,23 @@ grub_cryptodisk_endecrypt (struct
> > grub_cryptodisk *dev, }
> >       break;
> >     case GRUB_CRYPTODISK_MODE_IV_PLAIN64:
> > -     iv[1] = grub_cpu_to_le32 (sector >> 32);
> > -     /* FALLTHROUGH */
> >     case GRUB_CRYPTODISK_MODE_IV_PLAIN:
> > -     iv[0] = grub_cpu_to_le32 (sector & 0xFFFFFFFF);
> > +     /*
> > +      * The IV is a 32 or 64 bit value of the dm-crypt native
> > sector
> > +      * number. If using 32 bit IV mode, zero out the most
> > significant
> > +      * 32 bits.
> > +      */
> > +     {
> > +       grub_uint64_t *iv64 = (grub_uint64_t *)iv;
> 
> ./configure --target=arm-linux-gnueabihf --with-platform=coreboot ...
> make
> 
> ...and you get this:
> 
>   disk/cryptodisk.c: In function ‘grub_cryptodisk_endecrypt’:
>   disk/cryptodisk.c:292:28: error: cast increases required alignment
> of target type [-Werror=cast-align] grub_uint64_t *iv64 =
> (grub_uint64_t *)iv; ^
>   cc1: all warnings being treated as errors
> 
> I think every 32-bit build will/may fail. It seems to me that
>   (grub_uint64_t *)(void *) iv;
> or
>   (grub_uint64_t *)(grub_addr_t) iv;
> should help.

I'm having issues building for arm-linux-gnueabihf, so I can't
effectively test this.  This code does compile for
--target=i386 --with-platform=pc, which I believe is 32-bit.

Since I can't test your suggestion, I can't verify that it works.
However, I suspect it may get rid of the compiler problem, but not
solve the underlying issue, which is that iv may not be 8-byte aligned.
My guess is that if the compiler message goes away you could get a
crash at the next line if iv is not 8-byte aligned.  Can you see if the
warning disappears if you define iv like this:

  grub_uint32_t iv[(GRUB_CRYPTO_MAX_CIPHER_BLOCKSIZE + 3) / 4]
  __attribute__((aligned(sizeof(grub_uint64_t))));

I'm not sure the best way to handle this the "grub way". Do you see a
better way to do this?

Glenn