On Fri, Oct 23, 2020 at 08:09:10PM +0200, Patrick Steinhardt 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;
> > + *iv64 = grub_cpu_to_le64 (sector << (log_sector_size
> > + -
> > GRUB_CRYPTODISK_IV_LOG_SIZE));
> > + if (dev->mode_iv == GRUB_CRYPTODISK_MODE_IV_PLAIN)
> > + iv[1] = 0;
> > + }
> > break;
> > case GRUB_CRYPTODISK_MODE_IV_BYTECOUNT64:
> > - iv[1] = grub_cpu_to_le32 (sector >> (32 - dev->log_sector_size));
> > - iv[0] = grub_cpu_to_le32 ((sector << dev->log_sector_size)
> > + iv[1] = grub_cpu_to_le32 (sector >> (32 - log_sector_size));
> > + iv[0] = grub_cpu_to_le32 ((sector << log_sector_size)
> > & 0xFFFFFFFF);
> > break;
> > case GRUB_CRYPTODISK_MODE_IV_BENBI:
> > @@ -311,10 +321,10 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk
> > *dev,
> > case GRUB_CRYPTODISK_MODE_CBC:
> > if (do_encrypt)
> > err = grub_crypto_cbc_encrypt (dev->cipher, data + i, data + i,
> > - (1U << dev->log_sector_size), iv);
> > + (1U << log_sector_size), iv);
> > else
> > err = grub_crypto_cbc_decrypt (dev->cipher, data + i, data + i,
> > - (1U << dev->log_sector_size), iv);
> > + (1U << log_sector_size), iv);
> > if (err)
> > return err;
> > break;
> > @@ -322,10 +332,10 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk
> > *dev,
> > case GRUB_CRYPTODISK_MODE_PCBC:
> > if (do_encrypt)
> > err = grub_crypto_pcbc_encrypt (dev->cipher, data + i, data + i,
> > - (1U << dev->log_sector_size), iv);
> > + (1U << log_sector_size), iv);
> > else
> > err = grub_crypto_pcbc_decrypt (dev->cipher, data + i, data + i,
> > - (1U << dev->log_sector_size), iv);
> > + (1U << log_sector_size), iv);
> > if (err)
> > return err;
> > break;
> > @@ -337,7 +347,7 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk *dev,
> > if (err)
> > return err;
> >
> > - for (j = 0; j < (1U << dev->log_sector_size);
> > + for (j = 0; j < (1U << log_sector_size);
> > j += dev->cipher->cipher->blocksize)
> > {
> > grub_crypto_xor (data + i + j, data + i + j, iv,
> > @@ -368,11 +378,11 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk
> > *dev,
> > if (do_encrypt)
> > err = grub_crypto_ecb_encrypt (dev->cipher, data + i,
> > data + i,
> > - (1U << dev->log_sector_size));
> > + (1U << log_sector_size));
> > else
> > err = grub_crypto_ecb_decrypt (dev->cipher, data + i,
> > data + i,
> > - (1U << dev->log_sector_size));
> > + (1U << log_sector_size));
> > if (err)
> > return err;
> > lrw_xor (&sec, dev, data + i);
> > @@ -381,10 +391,10 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk
> > *dev,
> > case GRUB_CRYPTODISK_MODE_ECB:
> > if (do_encrypt)
> > err = grub_crypto_ecb_encrypt (dev->cipher, data + i, data + i,
> > - (1U << dev->log_sector_size));
> > + (1U << log_sector_size));
> > else
> > err = grub_crypto_ecb_decrypt (dev->cipher, data + i, data + i,
> > - (1U << dev->log_sector_size));
> > + (1U << log_sector_size));
> > if (err)
> > return err;
> > break;
> > @@ -399,9 +409,9 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk *dev,
> > gcry_err_code_t
> > grub_cryptodisk_decrypt (struct grub_cryptodisk *dev,
> > grub_uint8_t * data, grub_size_t len,
> > - grub_disk_addr_t sector)
> > + grub_disk_addr_t sector, grub_size_t log_sector_size)
> > {
> > - return grub_cryptodisk_endecrypt (dev, data, len, sector, 0);
> > + return grub_cryptodisk_endecrypt (dev, data, len, sector,
> > log_sector_size, 0);
> > }
> >
> > grub_err_t
> > @@ -766,7 +776,7 @@ grub_cryptodisk_read (grub_disk_t disk,
> > grub_disk_addr_t sector,
> > }
> > gcry_err = grub_cryptodisk_endecrypt (dev, (grub_uint8_t *) buf,
> > size << disk->log_sector_size,
> > - sector, 0);
> > + sector, dev->log_sector_size, 0);
> > return grub_crypto_gcry_error (gcry_err);
> > }
> >
> > @@ -807,7 +817,7 @@ grub_cryptodisk_write (grub_disk_t disk,
> > grub_disk_addr_t sector,
> >
> > gcry_err = grub_cryptodisk_endecrypt (dev, (grub_uint8_t *) tmp,
> > size << disk->log_sector_size,
> > - sector, 1);
> > + sector, disk->log_sector_size, 1);
> > if (gcry_err)
> > {
> > grub_free (tmp);
> > diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> > index 59702067a..20cc20b9b 100644
> > --- a/grub-core/disk/luks.c
> > +++ b/grub-core/disk/luks.c
> > @@ -124,7 +124,7 @@ configure_ciphers (grub_disk_t disk, const char
> > *check_uuid,
> > return NULL;
> > newdev->offset = grub_be_to_cpu32 (header.payloadOffset);
> > newdev->source_disk = NULL;
> > - newdev->log_sector_size = 9;
> > + newdev->log_sector_size = LUKS1_LOG_SECTOR_SIZE;
> > newdev->total_length = grub_disk_get_size (disk) - newdev->offset;
> > grub_memcpy (newdev->uuid, uuid, sizeof (uuid));
> > newdev->modname = "luks";
> > @@ -247,7 +247,8 @@ luks_recover_key (grub_disk_t source,
> > return err;
> > }
> >
> > - gcry_err = grub_cryptodisk_decrypt (dev, split_key, length, 0);
> > + gcry_err = grub_cryptodisk_decrypt (dev, split_key, length, 0,
> > + LUKS1_LOG_SECTOR_SIZE);
> > if (gcry_err)
> > {
> > grub_free (split_key);
> > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> > index 311d18684..b9e0e98e1 100644
> > --- a/grub-core/disk/luks2.c
> > +++ b/grub-core/disk/luks2.c
> > @@ -500,7 +500,12 @@ luks2_decrypt_key (grub_uint8_t *out_key,
> > goto err;
> > }
> >
> > - gcry_ret = grub_cryptodisk_decrypt (crypt, split_key, k->area.size, 0);
> > + /*
> > + * The key slots area is always encrypted in 512-byte sectors,
> > + * regardless of encrypted data sector size.
> > + */
> > + gcry_ret = grub_cryptodisk_decrypt (crypt, split_key, k->area.size, 0,
> > + LUKS1_LOG_SECTOR_SIZE);
> > if (gcry_ret)
> > {
> > ret = grub_crypto_gcry_error (gcry_ret);
> > diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> > index e1b21e785..006f3ec49 100644
> > --- a/include/grub/cryptodisk.h
> > +++ b/include/grub/cryptodisk.h
> > @@ -48,6 +48,12 @@ typedef enum
> >
> > #define GRUB_CRYPTODISK_MAX_UUID_LENGTH 71
> >
> > +/* LUKS1 specification defines the block size to always be 512 bytes. */
> > +#define LUKS1_LOG_SECTOR_SIZE 9
>
> Nit: This should probably be called `GRUB_LUKS1_LOG_SECTOR_SIZE`.
Yep.
> Otherwise the patch looks good to me.
Same for me, Reviewed-by: Daniel Kiper <daniel.ki...@oracle.com>
I can fix this minor issue before committing the patch.
Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
