Re: [PATCH v3 9/9] cryptodisk: Properly handle non-512 byte sized sectors
On Mon, Sep 21, 2020 at 05:58:06AM +, Glenn Washburn wrote:
> Sep 9, 2020 5:22:11 AM Daniel Kiper :
>
> > On Mon, Sep 07, 2020 at 05:28:08PM +0200, Patrick Steinhardt wrote:
> >> From: Glenn Washburn
> >>
> >> 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."
Yeah, LGTM.
> >> 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
> >> Reviewed-by: Patrick Steinhardt
> >> ---
> >> 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, , 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?
If you touch
Re: [PATCH v3 9/9] cryptodisk: Properly handle non-512 byte sized sectors
Sep 9, 2020 5:22:11 AM Daniel Kiper :
> On Mon, Sep 07, 2020 at 05:28:08PM +0200, Patrick Steinhardt wrote:
>> From: Glenn Washburn
>>
>> 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
>> Reviewed-by: Patrick Steinhardt
>> ---
>> 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, , 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 & 0x);
>> + iv_calc = sector << (log_sector_size - GRUB_CRYPTODISK_IV_LOG_SIZE);
>> + iv[0] = grub_cpu_to_le32 (iv_calc & 0x);
>> break;
>> case GRUB_CRYPTODISK_MODE_IV_BYTECOUNT64:
>> -
Re: [PATCH v3 9/9] cryptodisk: Properly handle non-512 byte sized sectors
On Mon, Sep 07, 2020 at 05:28:08PM +0200, Patrick Steinhardt wrote:
> From: Glenn Washburn
>
> 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/?
> 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
> Reviewed-by: Patrick Steinhardt
> ---
> 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, , 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?
> /* FALLTHROUGH */
> case GRUB_CRYPTODISK_MODE_IV_PLAIN:
> - iv[0] = grub_cpu_to_le32 (sector & 0x);
> + iv_calc = sector << (log_sector_size - GRUB_CRYPTODISK_IV_LOG_SIZE);
> + iv[0] = grub_cpu_to_le32 (iv_calc & 0x);
> break;
> case GRUB_CRYPTODISK_MODE_IV_BYTECOUNT64:
> - iv[1] = grub_cpu_to_le32 (sector >> (32 - dev->log_sector_size));
Ditto?
> - 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 =
