[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 13/15] cryptodisk: Properly handle non-512 byte sized sect
|
From: |
Patrick Steinhardt |
|
Subject: |
Re: [PATCH v4 13/15] cryptodisk: Properly handle non-512 byte sized sectors. |
|
Date: |
Sun, 22 Nov 2020 13:30:46 +0100 |
On Fri, Nov 20, 2020 at 02:42:35AM -0600, Glenn Washburn wrote:
> On Sun, 15 Nov 2020 11:07:27 +0100
> Patrick Steinhardt <ps@pks.im> wrote:
>
> > On Fri, Nov 06, 2020 at 10:44:33PM -0600, 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 | 55
> > > ++++++++++++++++++++++--------------- grub-core/disk/luks.c |
> > > 5 ++-- grub-core/disk/luks2.c | 7 ++++-
> > > include/grub/cryptodisk.h | 8 +++++-
> > > 4 files changed, 49 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/grub-core/disk/cryptodisk.c
> > > b/grub-core/disk/cryptodisk.c index 31b73c535..61f8e57f4 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,12 +238,12 @@ 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)
> > > / sizeof (grub_uint32_t));
> > > - grub_uint32_t iv[(GRUB_CRYPTO_MAX_CIPHER_BLOCKSIZE + 3) / 4];
> > > + grub_uint32_t iv[(GRUB_CRYPTO_MAX_CIPHER_BLOCKSIZE + 3) / 4]
> > > __attribute__((aligned (sizeof (grub_uint64_t))));
> > > if (dev->rekey)
> > > {
> > > @@ -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,15 +282,25 @@ 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 & GRUB_TYPE_U_MAX
> > > (iv[0]));
> > > + /*
> > > + * 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:
> > > + /* The IV is the 64 bit byte offset of the sector. */
> > > iv[1] = grub_cpu_to_le32 (sector >> (GRUB_TYPE_BITS
> > > (iv[1])
> > > - -
> > > dev->log_sector_size));
> > > - iv[0] = grub_cpu_to_le32 ((sector <<
> > > dev->log_sector_size)
> > > + - log_sector_size));
> > > + iv[0] = grub_cpu_to_le32 ((sector << log_sector_size)
> > > & GRUB_TYPE_U_MAX (iv[0]));
> > > break;
> > > case GRUB_CRYPTODISK_MODE_IV_BENBI:
> > > @@ -312,10 +323,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;
> > > @@ -323,10 +334,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;
> > > @@ -338,7 +349,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,
> > > @@ -369,11 +380,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);
> > > @@ -382,10 +393,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;
> > > @@ -400,9 +411,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
> > > @@ -767,7 +778,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);
> > > }
> > >
> > > @@ -808,7 +819,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 aa9877b68..84c3fa73a 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_sectors = grub_be_to_cpu32 (header.payloadOffset);
> > > newdev->source_disk = NULL;
> > > - newdev->log_sector_size = 9;
> > > + newdev->log_sector_size = GRUB_LUKS1_LOG_SECTOR_SIZE;
> > > newdev->total_sectors = grub_disk_get_size (disk) -
> > > newdev->offset_sectors; 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,
> > > +
> > > GRUB_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 355bb4aec..4a4a0dec4 100644
> > > --- a/grub-core/disk/luks2.c
> > > +++ b/grub-core/disk/luks2.c
> > > @@ -504,7 +504,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,
> > > + GRUB_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 258b777bf..ee30e4537 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 GRUB_LUKS1_LOG_SECTOR_SIZE 9
> >
> > Sorry to be nitpicky, but this constant is used for both LUKS1 and
> > LUKS2. Shouldn't it just be called `GRUB_LUKS_LOG_SECTOR_SIZE`?
> >
> > Patrick
>
> Its named LUKS1 because that macro is meant to represent the log of the
> sector size for the encrypted data segment, which is fixed at
> 512-bytes (as you know LUKS2 is variable). I think a name suggesting
> its only for the encrypted key data would be even longer and more
> unwieldy. And I don't think just removing the '1' would be an accurate
> name. Do you have other suggestions?
>
> Glenn
I mean we could split it up into two constants,
`GRUB_LUKS1_LOG_SECTOR_SIZE` and `GRUB_LUKS2_KEYSLOT_LOG_SECTOR_SIZE`.
Not sure if it's worth it, though.
Patrick
> >
> > > +/* By default dm-crypt increments the IV every 512 bytes. */
> > > +#define GRUB_CRYPTODISK_IV_LOG_SIZE 9
> > > +
> > > #define GRUB_CRYPTODISK_GF_LOG_SIZE 7
> > > #define GRUB_CRYPTODISK_GF_SIZE (1U << GRUB_CRYPTODISK_GF_LOG_SIZE)
> > > #define GRUB_CRYPTODISK_GF_LOG_BYTES (GRUB_CRYPTODISK_GF_LOG_SIZE
> > > - 3) @@ -145,7 +151,7 @@ grub_cryptodisk_setkey (grub_cryptodisk_t
> > > 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); grub_err_t
> > > grub_cryptodisk_insert (grub_cryptodisk_t newdev, const char *name,
> > > grub_disk_t source);
> > > --
> > > 2.27.0
> > >
signature.asc
Description: PGP signature
[PATCH v4 15/15] luks2: Error check segment.sector_size., Glenn Washburn, 2020/11/06