Re: [PATCH v3 0/9] Cryptodisk fixes for v2.06

Re: [PATCH v3 0/9] Cryptodisk fixes for v2.06

[Glenn Washburn]

Sep 17, 2020 8:14:40 AM Patrick Steinhardt <ps@pks.im>:

> On Mon, Sep 07, 2020 at 05:27:27PM +0200, Patrick Steinhardt wrote:
>> this is the third version of this patchset, collecting various fixes for
>> LUKS2/cryptodisk for the upcoming release of GRUB v2.06.
>>
>> Besides my Reviewed-by tag, the only thing that changed is the final
>> patch by Glenn. Quoting him:
>>
>>> The main difference with this patch is that sector_size is renamed to
>>> log_sector_size, grub has enough inaccurate or misleading names.
>>> Additionally, rename LOG_SECTOR_SIZE to LUKS_LOG_SECTOR_SIZE and
>>> CRYPT_LOG_SECTOR_SIZE to GRUB_CRYPTODISK_IV_LOG_SIZE and moved to
>>> cryptodisk.h.  Also a comment was reworded for clarity.
>
> A subset of these patches has been applied by Daniel, leaving us at
> (rearranged for better readability):
>
>> Glenn Washburn (6):
>> cryptodisk: Fix incorrect calculation of start sector
>> cryptodisk: Unregister cryptomount command when removing module
>
> Both were picked.
>
>> luks2: Fix use of incorrect index and some error messages
>> luks2: grub_cryptodisk_t->total_length is the max number of device
>> native sectors
>> cryptodisk: Fix cipher IV mode 'plain64' always being set as 'plain'
>> cryptodisk: Properly handle non-512 byte sized sectors
>
> These weren't yet and got some feedback.
>
>> Patrick Steinhardt (3):
>> json: Remove invalid typedef redefinition
>> luks: Fix out-of-bounds copy of UUID
>> luks2: Improve error reporting when decrypting/verifying key
>
> All three of these have been applied.
>
> @Glenn: seeing that all of my patches have been applied, do you want to
> take over your remaining four patches again? That'd probably make the
> process easier for both of us.

Sure, I can do that. I've been traveling for the last several weeks with little connectivity and limited time, which is why I haven't been active in addressing the responses on this thread. Thanks for helping move this forward

[PATCH 0/9] Cryptodisk fixes for v2.06

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2701 bytes --]

Hi,

I've sifted through the mailing list contents of the last few months to
cherry-pick cryptodisk bugfixes which I think should be included in the
v2.06 release. I've found the following 9 patches from Glenn and me
which should probably be included, separated them out from their
respective patch series and made them play nice with each other.

This patch series shouldn't be applied as-is, but my intention is
instead to bundle all fixes which apply to v2.06 in a single thread to
make discussion easier and help us keep track of what needs to be done.
I've got some comments which I've sent to the original threads already
and added notes below.

- luks2: grub_cryptodisk_t->total_length is the max number of device
  native sectors

    I'm not sure if this fix is correct, mostly because I think that
    `grub_disk_get_size` is buggy already: it returns sectors for
    partitions and the total size for disks. So I do think we need
    another patch to fix that function, too.


- cryptodisk: Incorrect calculation of start sector for grub_disk_read
  in grub_cryptodisk_read

    The patch looks correct to me and matches what both LUKS and LUKS2
    on-disk format say. But I'm surprised our code ever worked correctly
    without this fix, which does make me feel uncomfortable.

- cryptodisk: Properly handle non-512 byte sized sectors

    Should we pick this for v2.06? It definitely fixes things, but also
    feels a bit like feature-enablement.

I've added my Reviewed-by to those patches which look obviously correct
to me.

Glenn, please let me know if this somehow interferes with your work or
if you'd like to handle upstreaming of those fixes yourself.

Patrick


Glenn Washburn (6):
  luks2: Fix use of incorrect index and some error messages
  luks2: grub_cryptodisk_t->total_length is the max number of device
    native sectors
  cryptodisk: Unregister cryptomount command when removing module
  cryptodisk: Incorrect calculation of start sector for grub_disk_read
    in grub_cryptodisk_read
  cryptodisk: Fix cipher IV mode 'plain64' always being set as 'plain'
  cryptodisk: Properly handle non-512 byte sized sectors

Patrick Steinhardt (3):
  json: Remove invalid typedef redefinition
  luks: Fix out-of-bounds copy of UUID
  luks2: Improve error reporting when decrypting/verifying key

 grub-core/disk/cryptodisk.c | 56 +++++++++++++++++++++----------------
 grub-core/disk/luks.c       |  7 +++--
 grub-core/disk/luks2.c      | 33 +++++++++++++---------
 grub-core/lib/json/json.h   |  9 +++---
 include/grub/cryptodisk.h   |  2 +-
 5 files changed, 62 insertions(+), 45 deletions(-)

-- 
2.28.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 0/9] Cryptodisk fixes for v2.06

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 12933 bytes --]

Hi,

this is the third version of this patchset, collecting various fixes for
LUKS2/cryptodisk for the upcoming release of GRUB v2.06.

Besides my Reviewed-by tag, the only thing that changed is the final
patch by Glenn. Quoting him:

    > The main difference with this patch is that sector_size is renamed to
    > log_sector_size, grub has enough inaccurate or misleading names.
    > Additionally, rename LOG_SECTOR_SIZE to LUKS_LOG_SECTOR_SIZE and
    > CRYPT_LOG_SECTOR_SIZE to GRUB_CRYPTODISK_IV_LOG_SIZE and moved to
    > cryptodisk.h.  Also a comment was reworded for clarity.

The range-diff against v2 is attached below.

Patrick

Glenn Washburn (6):
  luks2: Fix use of incorrect index and some error messages
  luks2: grub_cryptodisk_t->total_length is the max number of device
    native sectors
  cryptodisk: Unregister cryptomount command when removing module
  cryptodisk: Fix incorrect calculation of start sector
  cryptodisk: Fix cipher IV mode 'plain64' always being set as 'plain'
  cryptodisk: Properly handle non-512 byte sized sectors

Patrick Steinhardt (3):
  json: Remove invalid typedef redefinition
  luks: Fix out-of-bounds copy of UUID
  luks2: Improve error reporting when decrypting/verifying key

 grub-core/disk/cryptodisk.c | 60 +++++++++++++++++++------------------
 grub-core/disk/luks.c       |  8 +++--
 grub-core/disk/luks2.c      | 55 ++++++++++++++++++++--------------
 grub-core/lib/json/json.h   |  9 +++---
 include/grub/cryptodisk.h   |  9 +++++-
 include/grub/disk.h         |  7 +++++
 6 files changed, 88 insertions(+), 60 deletions(-)

Range-diff against v2:
 1:  ee402de4d =  1:  ee402de4d json: Remove invalid typedef redefinition
 2:  5ecb9a4eb =  2:  5ecb9a4eb luks: Fix out-of-bounds copy of UUID
 3:  f8da5b4b4 =  3:  f8da5b4b4 luks2: Fix use of incorrect index and some error messages
 4:  150491a07 !  4:  efbf789e2 luks2: grub_cryptodisk_t->total_length is the max number of device native sectors
    @@ Commit message
         segment.size is in bytes which need to be converted to cryptodisk sectors.
     
         Signed-off-by: Glenn Washburn <development@efficientek.com>
    +    Reviewed-by: Patrick Steinhardt <ps@pks.im>
     
      ## grub-core/disk/luks2.c ##
     @@ grub-core/disk/luks2.c: luks2_decrypt_key (grub_uint8_t *out_key,
 5:  7dbfad568 =  5:  eb4198a01 luks2: Improve error reporting when decrypting/verifying key
 6:  dbf25a0ae =  6:  7ef38470b cryptodisk: Unregister cryptomount command when removing module
 7:  4ee7f8774 =  7:  a9765c0f4 cryptodisk: Fix incorrect calculation of start sector
 8:  4aecb174b =  8:  5497b02cc cryptodisk: Fix cipher IV mode 'plain64' always being set as 'plain'
 9:  f520aecfa !  9:  81b8a7f91 cryptodisk: Properly handle non-512 byte sized sectors
    @@ Commit message
         Reviewed-by: Patrick Steinhardt <ps@pks.im>
     
      ## grub-core/disk/cryptodisk.c ##
    -@@
    - 
    - 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[] =
     @@ grub-core/disk/cryptodisk.c: 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,
    ++			   grub_disk_addr_t sector, grub_size_t log_sector_size,
     +			   int do_encrypt)
      {
        grub_size_t i;
    @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_endecrypt (struct grub_cryptodisk *
      	    : 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))
    ++  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;
    ++      grub_uint64_t iv_calc = 0;
      
            if (dev->rekey)
      	{
    @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_endecrypt (struct grub_cryptodisk *
      	      return GPG_ERR_OUT_OF_MEMORY;
      
     -	    tmp = grub_cpu_to_le64 (sector << dev->log_sector_size);
    -+	    tmp = grub_cpu_to_le64 (sector << 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));
    @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_endecrypt (struct grub_cryptodisk *
      	  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_calc = sector << (log_sector_size - GRUB_CRYPTODISK_IV_LOG_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_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));
     -	  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)
    ++	  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:
    @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_endecrypt (struct grub_cryptodisk *
      	  if (do_encrypt)
      	    err = grub_crypto_cbc_encrypt (dev->cipher, data + i, data + i,
     -					   (1U << dev->log_sector_size), iv);
    -+					   (1U << 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 << sector_size), iv);
    ++					   (1U << log_sector_size), iv);
      	  if (err)
      	    return err;
      	  break;
    @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_endecrypt (struct grub_cryptodisk *
      	  if (do_encrypt)
      	    err = grub_crypto_pcbc_encrypt (dev->cipher, data + i, data + i,
     -					    (1U << dev->log_sector_size), iv);
    -+					    (1U << 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 << sector_size), iv);
    ++					    (1U << log_sector_size), iv);
      	  if (err)
      	    return err;
      	  break;
    @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_endecrypt (struct grub_cryptodisk *
      	      return err;
      	    
     -	    for (j = 0; j < (1U << dev->log_sector_size);
    -+	    for (j = 0; j < (1U << 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,
    @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_endecrypt (struct grub_cryptodisk *
      	      err = grub_crypto_ecb_encrypt (dev->cipher, data + i, 
      					     data + i,
     -					     (1U << dev->log_sector_size));
    -+					     (1U << sector_size));
    ++					     (1U << log_sector_size));
      	    else
      	      err = grub_crypto_ecb_decrypt (dev->cipher, data + i, 
      					     data + i,
     -					     (1U << dev->log_sector_size));
    -+					     (1U << sector_size));
    ++					     (1U << log_sector_size));
      	    if (err)
      	      return err;
      	    lrw_xor (&sec, dev, data + i);
    @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_endecrypt (struct grub_cryptodisk *
      	  if (do_encrypt)
      	    err = grub_crypto_ecb_encrypt (dev->cipher, data + i, data + i,
     -					   (1U << dev->log_sector_size));
    -+					   (1U << sector_size));
    ++					   (1U << log_sector_size));
      	  else
      	    err = grub_crypto_ecb_decrypt (dev->cipher, data + i, data + i,
     -					   (1U << dev->log_sector_size));
    -+					   (1U << sector_size));
    ++					   (1U << log_sector_size));
      	  if (err)
      	    return err;
      	  break;
    @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_endecrypt (struct grub_cryptodisk *
      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_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, sector_size, 0);
    ++  return grub_cryptodisk_endecrypt (dev, data, len, sector, log_sector_size, 0);
      }
      
      grub_err_t
    @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_write (grub_disk_t disk, grub_disk_
            grub_free (tmp);
     
      ## grub-core/disk/luks.c ##
    -@@
    - GRUB_MOD_LICENSE ("GPLv3+");
    - 
    - #define MAX_PASSPHRASE 256
    -+#define LOG_SECTOR_SIZE 9
    - 
    - #define LUKS_KEY_ENABLED  0x00AC71F3
    - 
     @@ grub-core/disk/luks.c: 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->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";
    @@ grub-core/disk/luks.c: luks_recover_key (grub_disk_t source,
      	}
      
     -      gcry_err = grub_cryptodisk_decrypt (dev, split_key, length, 0);
    -+      gcry_err = grub_cryptodisk_decrypt (dev, split_key, length, 0, LOG_SECTOR_SIZE);
    ++      gcry_err = grub_cryptodisk_decrypt (dev, split_key, length, 0,
    ++					  LUKS_LOG_SECTOR_SIZE);
            if (gcry_err)
      	{
      	  grub_free (split_key);
    @@ grub-core/disk/luks2.c: luks2_decrypt_key (grub_uint8_t *out_key,
      
     -  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
    ++   * 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, 9);
    ++  gcry_ret = grub_cryptodisk_decrypt (crypt, split_key, k->area.size, 0,
    ++				      LUKS_LOG_SECTOR_SIZE);
        if (gcry_ret)
          {
            ret = grub_crypto_gcry_error (gcry_ret);
     
      ## include/grub/cryptodisk.h ##
    +@@ include/grub/cryptodisk.h: 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)
     @@ include/grub/cryptodisk.h: 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_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.28.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 0/9] Cryptodisk fixes for v2.06

[Daniel Kiper]

Hey Patrick,

On Mon, Sep 07, 2020 at 05:27:27PM +0200, Patrick Steinhardt wrote:
> Hi,
>
> this is the third version of this patchset, collecting various fixes for
> LUKS2/cryptodisk for the upcoming release of GRUB v2.06.
>
> Besides my Reviewed-by tag, the only thing that changed is the final
> patch by Glenn. Quoting him:
>
>     > The main difference with this patch is that sector_size is renamed to
>     > log_sector_size, grub has enough inaccurate or misleading names.
>     > Additionally, rename LOG_SECTOR_SIZE to LUKS_LOG_SECTOR_SIZE and
>     > CRYPT_LOG_SECTOR_SIZE to GRUB_CRYPTODISK_IV_LOG_SIZE and moved to
>     > cryptodisk.h.  Also a comment was reworded for clarity.

A few patches require some polishing. However, feel free to add
  Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
to the patches 1, 2, 5, 6 and 7.

Daniel

Re: [PATCH v3 0/9] Cryptodisk fixes for v2.06

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1718 bytes --]

On Mon, Sep 07, 2020 at 05:27:27PM +0200, Patrick Steinhardt wrote:
> this is the third version of this patchset, collecting various fixes for
> LUKS2/cryptodisk for the upcoming release of GRUB v2.06.
> 
> Besides my Reviewed-by tag, the only thing that changed is the final
> patch by Glenn. Quoting him:
> 
>     > The main difference with this patch is that sector_size is renamed to
>     > log_sector_size, grub has enough inaccurate or misleading names.
>     > Additionally, rename LOG_SECTOR_SIZE to LUKS_LOG_SECTOR_SIZE and
>     > CRYPT_LOG_SECTOR_SIZE to GRUB_CRYPTODISK_IV_LOG_SIZE and moved to
>     > cryptodisk.h.  Also a comment was reworded for clarity.

A subset of these patches has been applied by Daniel, leaving us at
(rearranged for better readability):

> Glenn Washburn (6):
>   cryptodisk: Fix incorrect calculation of start sector
>   cryptodisk: Unregister cryptomount command when removing module

Both were picked.

>   luks2: Fix use of incorrect index and some error messages
>   luks2: grub_cryptodisk_t->total_length is the max number of device
>     native sectors
>   cryptodisk: Fix cipher IV mode 'plain64' always being set as 'plain'
>   cryptodisk: Properly handle non-512 byte sized sectors

These weren't yet and got some feedback.

> Patrick Steinhardt (3):
>   json: Remove invalid typedef redefinition
>   luks: Fix out-of-bounds copy of UUID
>   luks2: Improve error reporting when decrypting/verifying key

All three of these have been applied.

@Glenn: seeing that all of my patches have been applied, do you want to
take over your remaining four patches again? That'd probably make the
process easier for both of us.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]