Re: [PATCH v3 9/9] cryptodisk: Properly handle non-512 byte sized sectors

[Glenn Washburn]

Sep 9, 2020 5:22:11 AM Daniel Kiper <daniel.kiper@oracle.com>:

> On Mon, Sep 07, 2020 at 05:28:08PM +0200, Patrick Steinhardt 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
>
> s/32-39/32-9/?

I'm reading this and realizing it's worded badly and confusing. Perhaps this 
sentence is better?

"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."

>> 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 | 44 ++++++++++++++++++++-----------------
>> grub-core/disk/luks.c       |  5 +++--
>> grub-core/disk/luks2.c      |  7 +++++-
>> include/grub/cryptodisk.h   |  9 +++++++-
>> 4 files changed, 41 insertions(+), 24 deletions(-)
>>
>> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
>> index 0b63b7d96..6319f3164 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,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 << 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_uint64_t iv_calc = 0;
>>
>> if (dev->rekey)
>> {
>> @@ -270,7 +272,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 +283,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 << (log_sector_size - GRUB_CRYPTODISK_IV_LOG_SIZE);
>> +   iv[1] = grub_cpu_to_le32 (iv_calc >> 32);
>
> Why 32? Could you use a constant or add a comment here?

Plain mode uses a 32 bit IV and plain64 uses a 64 bit IV. So in the plain64 
case only deal with the non-plain (ie not lower 32 bits) IV bits and we deal 
with the plain case in the fall through.

I don't think a constant is warranted and I can add in a comment in this patch, 
but I'd like to point out that the "32" literal you're commenting on is not 
code I've introduced. As such, the comment would not be relevant to this patch. 
Given that, do you still want a comment in this patch?

>> /* FALLTHROUGH */
>> case GRUB_CRYPTODISK_MODE_IV_PLAIN:
>> -   iv[0] = grub_cpu_to_le32 (sector & 0xFFFFFFFF);
>> +   iv_calc = sector << (log_sector_size - GRUB_CRYPTODISK_IV_LOG_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));
>
> Ditto?

For modes other than plain and plain64, all that is being done is substituting 
"dev->log_sector_size" for "log_sector_size". Again, those constant "32" 
integers are not code that I've added and I don't think comments are relevant 
to this patch.

Here the "32" is used to create the IV in 2 32bit chunks because the variable 
"iv" is an array of 32bit uints.

>> -   iv[0] = grub_cpu_to_le32 ((sector << dev->log_sector_size)
>> +   iv[1] = grub_cpu_to_le32 (sector >> (32 - log_sector_size));
>
> Ditto?
>
> [...]
>
>> diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
>> index 59702067a..2e1347b13 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 = LUKS_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,
>> +           LUKS_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..eb64a0596 100644
>> --- a/grub-core/disk/luks2.c
>> +++ b/grub-core/disk/luks2.c
>> @@ -492,7 +492,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,
>> +             LUKS_LOG_SECTOR_SIZE);
>
> s/LUKS_LOG_SECTOR_SIZE/GRUB_CRYPTODISK_IV_LOG_SIZE/?

LUKS_LOG_SECTOR_SIZE and GRUB_CRYPTODISK_IV_LOG_SIZE
are the same number, but they represent different things. According to the 
luks2 spec the key slot area is decrypted as specified in the luks1 spec. So 
perhaps the constant should be renamed to LUKS1_LOG_SECTOR_SIZE, because luks2 
does not have a constant sector size.

The constant GRUB_CRYPTODISK_IV_LOG_SIZE
is meant to represent the log of the sector size that dm-crypt uses natively 
for incrementing the IV.

>> if (gcry_ret)
>> {
>> ret = grub_crypto_gcry_error (gcry_ret);
>> diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
>> index e1b21e785..ecb37ba43 100644
>> --- a/include/grub/cryptodisk.h
>> +++ b/include/grub/cryptodisk.h
>> @@ -48,6 +48,13 @@ typedef enum
>>
>> #define GRUB_CRYPTODISK_MAX_UUID_LENGTH 71
>>
>> +#define LUKS_LOG_SECTOR_SIZE 9
>> +
>> +/* For the purposes of IV incrementing the sector size is 512 bytes, unless
>> + * otherwise specified.
>> + */
>> +#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)
>> @@ -139,7 +146,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);
>
> Daniel
>

Glenn