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

[Daniel Kiper]

On Fri, Nov 06, 2020 at 01:08:28PM -0600, Glenn Washburn wrote:
> 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?

Below you can find all fixes which are needed to properly build
the GRUB with your patches on all platforms. If you have any
questions drop me a line.

Daniel

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 61f8e57f4..b62835acc 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -243,7 +243,7 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk *dev,
       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] 
__attribute__((aligned (sizeof (grub_uint64_t))));
+      grub_uint32_t iv[(GRUB_CRYPTO_MAX_CIPHER_BLOCKSIZE + 3) / 4];

       if (dev->rekey)
        {
@@ -289,9 +289,11 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk *dev,
           * 32 bits.
           */
          {
-           grub_uint64_t *iv64 = (grub_uint64_t *) iv;
-           *iv64 = grub_cpu_to_le64 (sector << (log_sector_size
+           grub_uint64_t iv64;
+
+           iv64 = grub_cpu_to_le64 (sector << (log_sector_size
                                                 - 
GRUB_CRYPTODISK_IV_LOG_SIZE));
+           grub_set_unaligned64 (iv, iv64);
            if (dev->mode_iv == GRUB_CRYPTODISK_MODE_IV_PLAIN)
              iv[1] = 0;
          }
diff --git a/grub-core/kern/compiler-rt.c b/grub-core/kern/compiler-rt.c
index a464200c6..2057c2e0c 100644
--- a/grub-core/kern/compiler-rt.c
+++ b/grub-core/kern/compiler-rt.c
@@ -448,7 +448,7 @@ __clzsi2 (grub_uint32_t val)
 }
 #endif

-#if defined(__riscv) || defined(__sparc__)
+#if defined(__mips__) || defined(__riscv) || defined(__sparc__)
 int
 __clzdi2 (grub_uint64_t val)
 {
diff --git a/include/grub/compiler-rt.h b/include/grub/compiler-rt.h
index 7591980b4..17828b322 100644
--- a/include/grub/compiler-rt.h
+++ b/include/grub/compiler-rt.h
@@ -115,7 +115,7 @@ int
 EXPORT_FUNC (__clzsi2) (grub_uint32_t val);
 #endif

-#if defined(__riscv) || defined(__sparc__)
+#if defined(__mips__) || defined(__riscv) || defined(__sparc__)
 int
 EXPORT_FUNC (__clzdi2) (grub_uint64_t val);
 #endif