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

[Glenn Washburn]

On Fri, 20 Nov 2020 15:17:25 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> 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

Ok, will do.  I'm unsure how to commit the patch though.  Since you are
the author and I am the committer.  I'm thinking I should have my name
as the commit author, but yours should be in a Signed-off-by sig and I
don't need to add a Signed-off-by sig.  How should I proceed?  I'll
follow the commit message template from 9dab2f51e.  Also, how should I
attribute your contribution in cryptodisk.c?

Glenn