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

[Daniel Kiper]

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.

Daniel