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

[Daniel Kiper]

On Thu, Nov 19, 2020 at 11:18:34PM -0600, Glenn Washburn wrote:
> On Thu, 19 Nov 2020 15:25:33 +0100
> Daniel Kiper <dkiper@net-space.pl> wrote:
>
> > 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
>
> Great, thanks for the help.  I'm confused as to why the changes to
> compiler-rt* are needed now.  I assume that they are needed because of
> the use of __builtin_clzll.  My v4 changes include a patch that moves
> the use of __builtin_clzll in luks2.c into a macro in misc.h, but I
> don't think that would have necessitated this change because the only
> code that uses the macro is in luks2.c.  So the compiler should see no
> difference from current master.  Could it be that mips targets are not
> properly building with current master due to the use of __builtin_clzll?
> Or am I totally off?

I am not sure why this pop up after your patch series. If I have some
time I will dig into it.

> I'll add these in the forth coming patch series.  For now, I'll assume
> that I should put the compiler-rt* changes in a separate patch and
> merge in the cryptodisk.c changes.

Please provide compiler-rt* changes in a separate patch. Good example is
in commit 9dab2f51e (sparc: Enable __clzsi2() and __clzdi2()). Please
include this fix in new patch series.

Daniel