grub-devel
[Top][All Lists]
Advanced

[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, 15 Nov 2020 11:07:27 +0100

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

> +/* 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
> 

Attachment: signature.asc
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]