On Fri, 30 Oct 2020 21:47:14 +0100
Daniel Kiper <dki...@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 <developm...@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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
