Re: [PATCH v2 06/10] cryptodisk: Properly handle non-512 byte sized sectors.

[Glenn Washburn]

On Fri, 9 Oct 2020 11:50:29 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> On Sat, Oct 03, 2020 at 05:55:30PM -0500, 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 | 52
> > ++++++++++++++++++++++--------------- grub-core/disk/luks.c       |
> >  5 ++-- grub-core/disk/luks2.c      |  7 ++++-
> >  include/grub/cryptodisk.h   |  8 +++++-
> >  4 files changed, 47 insertions(+), 25 deletions(-)
> > 
> > diff --git a/grub-core/disk/cryptodisk.c
> > b/grub-core/disk/cryptodisk.c index a3d672f68..623f0f396 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,7 +238,7 @@ 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)
> > @@ -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,14 +282,23 @@ 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 & 0xFFFFFFFF);
> > +     /*
> > +      * 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;
> 
> I may be misreading this, but aren't we zeroing out 64 bits here
> instead of 32 bits as the comment states?
> 
> Patrick

We are only zeroing out 32bits because iv is declared as an array of
grub_uint32_t.  iv[1] will be the 32 right-most bits of iv64 (and
iv[0] the left-most 32 bits). Since iv64 is little-endian the right-most
32 bits will correspond to the most significant 32 bits of iv64.

> > +     }
> >       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 -
> > log_sector_size));
> > +     iv[0] = grub_cpu_to_le32 ((sector << log_sector_size)
> >                                 & 0xFFFFFFFF);
> >       break;
> >     case GRUB_CRYPTODISK_MODE_IV_BENBI:
> > @@ -311,10 +321,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;
> > @@ -322,10 +332,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;
> > @@ -337,7 +347,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,
> > @@ -368,11 +378,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);
> > @@ -381,10 +391,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;
> > @@ -399,9 +409,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
> > @@ -766,7 +776,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 +817,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..20cc20b9b 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 = LUKS1_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,
> > +                                     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 9f7d6e12b..b7d3b425a 100644
> > --- a/grub-core/disk/luks2.c
> > +++ b/grub-core/disk/luks2.c
> > @@ -499,7 +499,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,
> > +                                 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 e1b21e785..006f3ec49 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 LUKS1_LOG_SECTOR_SIZE 9
> > +
> > +/* 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) @@ -139,7 +145,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
> >