From: Glenn Washburn <development@efficientek.com>
To: Patrick Steinhardt <ps@pks.im>
Cc: grub-devel@gnu.org,
Denis GNUtoo Carikli <GNUtoo@cyberdimension.org>,
Daniel Kiper <daniel.kiper@oracle.com>
Subject: Re: [PATCH v2 9/9] cryptodisk: Properly handle non-512 byte sized sectors
Date: Mon, 31 Aug 2020 13:43:36 -0500 [thread overview]
Message-ID: <20200831134336.2dfaa2fc@crass-HP-ZBook-15-G2> (raw)
In-Reply-To: <f520aecfa28d2a91be7ee42ef24da6ff5ec38914.1598429170.git.ps@pks.im>
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);
next prev parent reply other threads:[~2020-08-31 18:43 UTC|newest]
Thread overview: 68+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-23 10:59 [PATCH 0/9] Cryptodisk fixes for v2.06 Patrick Steinhardt
2020-08-23 10:59 ` [PATCH 1/9] json: Remove invalid typedef redefinition Patrick Steinhardt
2020-08-23 10:59 ` [PATCH 2/9] luks: Fix out-of-bounds copy of UUID Patrick Steinhardt
2020-08-23 21:34 ` Denis 'GNUtoo' Carikli
2020-08-26 7:18 ` Patrick Steinhardt
2020-08-23 11:03 ` [PATCH 3/9] luks2: Fix use of incorrect index and some error messages Patrick Steinhardt
2020-08-24 6:30 ` Glenn Washburn
2020-08-24 6:33 ` Patrick Steinhardt
2020-08-23 11:03 ` [PATCH 4/9] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors Patrick Steinhardt
2020-08-23 11:03 ` [PATCH 5/9] luks2: Improve error reporting when decrypting/verifying key Patrick Steinhardt
2020-08-23 11:03 ` [PATCH 6/9] cryptodisk: Unregister cryptomount command when removing module Patrick Steinhardt
2020-08-23 11:04 ` [PATCH 7/9] cryptodisk: Incorrect calculation of start sector for grub_disk_read in grub_cryptodisk_read Patrick Steinhardt
2020-08-23 11:04 ` [PATCH 8/9] cryptodisk: Fix cipher IV mode 'plain64' always being set as 'plain' Patrick Steinhardt
2020-08-23 11:04 ` [PATCH 9/9] cryptodisk: Properly handle non-512 byte sized sectors Patrick Steinhardt
2020-08-24 6:22 ` [PATCH 0/9] Cryptodisk fixes for v2.06 Glenn Washburn
2020-08-24 6:31 ` Patrick Steinhardt
2020-08-26 8:13 ` [PATCH v2 " Patrick Steinhardt
2020-08-26 8:13 ` [PATCH v2 1/9] json: Remove invalid typedef redefinition Patrick Steinhardt
2020-08-26 8:13 ` [PATCH v2 2/9] luks: Fix out-of-bounds copy of UUID Patrick Steinhardt
2020-08-26 8:13 ` [PATCH v2 3/9] luks2: Fix use of incorrect index and some error messages Patrick Steinhardt
2020-08-26 8:13 ` [PATCH v2 4/9] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors Patrick Steinhardt
2020-08-26 8:13 ` [PATCH v2 5/9] luks2: Improve error reporting when decrypting/verifying key Patrick Steinhardt
2020-08-26 8:13 ` [PATCH v2 6/9] cryptodisk: Unregister cryptomount command when removing module Patrick Steinhardt
2020-08-26 23:44 ` [PATCH] cryptodisk: Incorrect calculation of sector in grub_cryptodisk_read/write Glenn Washburn
2020-08-26 23:50 ` Glenn Washburn
2020-08-28 7:12 ` Patrick Steinhardt
2020-08-26 8:13 ` [PATCH v2 7/9] cryptodisk: Fix incorrect calculation of start sector Patrick Steinhardt
2020-08-26 8:13 ` [PATCH v2 8/9] cryptodisk: Fix cipher IV mode 'plain64' always being set as 'plain' Patrick Steinhardt
2020-08-26 8:14 ` [PATCH v2 9/9] cryptodisk: Properly handle non-512 byte sized sectors Patrick Steinhardt
2020-08-31 18:43 ` Glenn Washburn [this message]
2020-09-01 15:28 ` Patrick Steinhardt
2020-09-01 23:21 ` [PATCH] " Glenn Washburn
2020-09-02 0:01 ` Glenn Washburn
2020-09-07 15:28 ` Patrick Steinhardt
2020-08-26 22:16 ` [PATCH v2 0/9] Cryptodisk fixes for v2.06 Glenn Washburn
2020-08-28 7:17 ` Patrick Steinhardt
2020-09-07 15:27 ` [PATCH v3 " Patrick Steinhardt
2020-09-07 15:27 ` [PATCH v3 1/9] json: Remove invalid typedef redefinition Patrick Steinhardt
2020-09-07 15:27 ` [PATCH v3 2/9] luks: Fix out-of-bounds copy of UUID Patrick Steinhardt
2020-09-07 15:27 ` [PATCH v3 3/9] luks2: Fix use of incorrect index and some error messages Patrick Steinhardt
2020-09-08 12:58 ` Daniel Kiper
2020-09-21 6:45 ` Glenn Washburn
2020-09-21 11:24 ` Daniel Kiper
2020-09-07 15:27 ` [PATCH v3 4/9] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors Patrick Steinhardt
2020-09-08 13:21 ` Daniel Kiper
2020-09-21 6:28 ` Glenn Washburn
2020-09-21 11:23 ` Daniel Kiper
2020-10-03 5:42 ` Glenn Washburn
2020-10-27 19:11 ` Daniel Kiper
2020-10-29 19:53 ` Glenn Washburn
2020-10-30 12:49 ` Daniel Kiper
2020-11-03 20:21 ` Glenn Washburn
2020-11-04 13:15 ` Daniel Kiper
2020-11-06 6:41 ` Glenn Washburn
2020-09-07 15:27 ` [PATCH v3 5/9] luks2: Improve error reporting when decrypting/verifying key Patrick Steinhardt
2020-09-07 15:27 ` [PATCH v3 6/9] cryptodisk: Unregister cryptomount command when removing module Patrick Steinhardt
2020-09-08 13:28 ` Daniel Kiper
2020-09-21 6:45 ` Glenn Washburn
2020-09-21 11:25 ` Daniel Kiper
2020-09-07 15:27 ` [PATCH v3 7/9] cryptodisk: Fix incorrect calculation of start sector Patrick Steinhardt
2020-09-07 15:28 ` [PATCH v3 8/9] cryptodisk: Fix cipher IV mode 'plain64' always being set as 'plain' Patrick Steinhardt
2020-09-08 13:42 ` Daniel Kiper
2020-09-07 15:28 ` [PATCH v3 9/9] cryptodisk: Properly handle non-512 byte sized sectors Patrick Steinhardt
2020-09-09 11:21 ` Daniel Kiper
2020-09-21 5:58 ` Glenn Washburn
2020-09-21 11:16 ` Daniel Kiper
2020-09-09 11:28 ` [PATCH v3 0/9] Cryptodisk fixes for v2.06 Daniel Kiper
2020-09-17 14:14 ` Patrick Steinhardt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200831134336.2dfaa2fc@crass-HP-ZBook-15-G2 \
--to=development@efficientek.com \
--cc=GNUtoo@cyberdimension.org \
--cc=daniel.kiper@oracle.com \
--cc=grub-devel@gnu.org \
--cc=ps@pks.im \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.