[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 9/9] cryptodisk: Properly handle non-512 byte sized sector
|
From: |
Glenn Washburn |
|
Subject: |
Re: [PATCH v2 9/9] cryptodisk: Properly handle non-512 byte sized sectors |
|
Date: |
Mon, 31 Aug 2020 13:43:36 -0500 |
I just noticed that I have a couple minor non-functional changes that I
think will make this patch a little better. I had been planning on
updating my original patch, but since you're picking this up, this is a
better place to update. Let me know if you'd like the updated version
of the patch instead of my inline comments below.
On Wed, 26 Aug 2020 10:14:02 +0200
Patrick Steinhardt <ps@pks.im> wrote:
> From: Glenn Washburn <development@efficientek.com>
>
> 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 cipher block in the fifth 4K sector will be
> encrypted with an IV equal to 32, as opposed to 32-39 for each
> sequential 512 byte block or an IV of 4 for each cipher block 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>
> Reviewed-by: Patrick Steinhardt <ps@pks.im>
> ---
> grub-core/disk/cryptodisk.c | 47
> +++++++++++++++++++++---------------- grub-core/disk/luks.c |
> 5 ++-- grub-core/disk/luks2.c | 6 ++++-
> include/grub/cryptodisk.h | 2 +-
> 4 files changed, 36 insertions(+), 24 deletions(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 0b63b7d96..0b8ac5b30 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -33,6 +33,9 @@
>
> GRUB_MOD_LICENSE ("GPLv3+");
>
> +/* Internally encrypted sectors are 512 bytes regardless of what the
> cryptodisk is */ +#define CRYPT_LOG_SECTOR_SIZE 9
> +
> grub_cryptodisk_dev_t grub_cryptodisk_list;
>
> static const struct grub_arg_option options[] =
> @@ -224,7 +227,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
> sector_size,
> + int do_encrypt)
I've changed the sector_size parameter to log_sector_size, which is
what it really is. I think this is really important to change because
there's already enough confusingly named identifiers.
> {
> grub_size_t i;
> gcry_err_code_t err;
> @@ -237,12 +241,13 @@ 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 << 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_uint64_t iv_calc;
>
> if (dev->rekey)
> {
> @@ -270,7 +275,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 << 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 +286,16 @@ grub_cryptodisk_endecrypt (struct
> grub_cryptodisk *dev, }
> break;
> case GRUB_CRYPTODISK_MODE_IV_PLAIN64:
> - iv[1] = grub_cpu_to_le32 (sector >> 32);
> + iv_calc = sector << (sector_size - CRYPT_LOG_SECTOR_SIZE);
> + iv[1] = grub_cpu_to_le32 (iv_calc >> 32);
> /* FALLTHROUGH */
> case GRUB_CRYPTODISK_MODE_IV_PLAIN:
> - iv[0] = grub_cpu_to_le32 (sector & 0xFFFFFFFF);
> + iv_calc = sector << (sector_size - CRYPT_LOG_SECTOR_SIZE);
> + iv[0] = grub_cpu_to_le32 (iv_calc & 0xFFFFFFFF);
> 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 - sector_size));
> + iv[0] = grub_cpu_to_le32 ((sector << sector_size)
> & 0xFFFFFFFF);
> break;
> case GRUB_CRYPTODISK_MODE_IV_BENBI:
> @@ -311,10 +318,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 << sector_size), iv);
> else
> err = grub_crypto_cbc_decrypt (dev->cipher, data + i,
> data + i,
> - (1U <<
> dev->log_sector_size), iv);
> + (1U << sector_size), iv);
> if (err)
> return err;
> break;
> @@ -322,10 +329,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 << sector_size), iv);
> else
> err = grub_crypto_pcbc_decrypt (dev->cipher, data + i,
> data + i,
> - (1U <<
> dev->log_sector_size), iv);
> + (1U << sector_size), iv);
> if (err)
> return err;
> break;
> @@ -337,7 +344,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 << sector_size);
> j += dev->cipher->cipher->blocksize)
> {
> grub_crypto_xor (data + i + j, data + i + j, iv,
> @@ -368,11 +375,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 << sector_size));
> else
> err = grub_crypto_ecb_decrypt (dev->cipher, data + i,
> data + i,
> - (1U <<
> dev->log_sector_size));
> + (1U << sector_size));
> if (err)
> return err;
> lrw_xor (&sec, dev, data + i);
> @@ -381,10 +388,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 << sector_size));
> else
> err = grub_crypto_ecb_decrypt (dev->cipher, data + i,
> data + i,
> - (1U <<
> dev->log_sector_size));
> + (1U << sector_size));
> if (err)
> return err;
> break;
> @@ -399,9 +406,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
> sector_size) {
> - return grub_cryptodisk_endecrypt (dev, data, len, sector, 0);
> + return grub_cryptodisk_endecrypt (dev, data, len, sector,
> sector_size, 0); }
>
> grub_err_t
> @@ -766,7 +773,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 +814,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..b30c4551e 100644
> --- a/grub-core/disk/luks.c
> +++ b/grub-core/disk/luks.c
> @@ -30,6 +30,7 @@
> GRUB_MOD_LICENSE ("GPLv3+");
>
> #define MAX_PASSPHRASE 256
> +#define LOG_SECTOR_SIZE 9
>
> #define LUKS_KEY_ENABLED 0x00AC71F3
>
> @@ -124,7 +125,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 = 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 +248,7 @@ 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,
> 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 26e1126b1..1f5f9766a 100644
> --- a/grub-core/disk/luks2.c
> +++ b/grub-core/disk/luks2.c
> @@ -492,7 +492,11 @@ luks2_decrypt_key (grub_uint8_t *out_key,
> goto err;
> }
>
> - gcry_ret = grub_cryptodisk_decrypt (crypt, split_key,
> k->area.size, 0);
> + /*
> + * The encrypted key slots are always with 512byte sectors,
> + * regardless of encrypted data sector size
> + */
> + gcry_ret = grub_cryptodisk_decrypt (crypt, split_key,
> k->area.size, 0, 9); if (gcry_ret)
I've reworded the comment to be slightly more readable:
The key slots area is always encrypted in 512-byte sectors,
regardless of encrypted data sector size.
> {
> ret = grub_crypto_gcry_error (gcry_ret);
> diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> index e1b21e785..06653a622 100644
> --- a/include/grub/cryptodisk.h
> +++ b/include/grub/cryptodisk.h
> @@ -139,7 +139,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
> sector_size); grub_err_t
> grub_cryptodisk_insert (grub_cryptodisk_t newdev, const char *name,
> grub_disk_t source);
- [PATCH v2 3/9] luks2: Fix use of incorrect index and some error messages, (continued)
[PATCH v2 7/9] cryptodisk: Fix incorrect calculation of start sector, Patrick Steinhardt, 2020/08/26
[PATCH v2 8/9] cryptodisk: Fix cipher IV mode 'plain64' always being set as 'plain', Patrick Steinhardt, 2020/08/26
[PATCH v2 9/9] cryptodisk: Properly handle non-512 byte sized sectors, Patrick Steinhardt, 2020/08/26
- Re: [PATCH v2 9/9] cryptodisk: Properly handle non-512 byte sized sectors,
Glenn Washburn <=
Re: [PATCH v2 0/9] Cryptodisk fixes for v2.06, Glenn Washburn, 2020/08/26