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

[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 1/9] json: Remove invalid typedef redefinition

[Patrick Steinhardt]

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

The C standard does not allow for typedef redefinitions, even if they
map to the same underlying type. In order to avoid including the
"jsmn.h" in "json.h" and thus exposing jsmn's internals, we have exactly
such a forward-declaring typedef in "json.h". If enforcing the GNU99 C
standard, clang may generate a warning about this non-standard
construct.

Fix the issue by using a simple `struct jsmntok` forward declaration
instead of using a typedef.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Tested-by: Chuck Tuffli <chuck@freebsd.org>
---
 grub-core/lib/json/json.h | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/grub-core/lib/json/json.h b/grub-core/lib/json/json.h
index 01614f6df..657902cfc 100644
--- a/grub-core/lib/json/json.h
+++ b/grub-core/lib/json/json.h
@@ -36,13 +36,14 @@ enum grub_json_type
 };
 typedef enum grub_json_type grub_json_type_t;
 
-typedef struct jsmntok jsmntok_t;
+/* Forward-declaration to avoid including "jsmn.h" */
+struct jsmntok;
 
 struct grub_json
 {
-  jsmntok_t   *tokens;
-  char	      *string;
-  grub_size_t idx;
+  struct jsmntok *tokens;
+  char		 *string;
+  grub_size_t	 idx;
 };
 typedef struct grub_json grub_json_t;
 
-- 
2.28.0


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

[PATCH 2/9] luks: Fix out-of-bounds copy of UUID

[Patrick Steinhardt]

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

When configuring a LUKS disk, we copy over the UUID from the LUKS header
into the new `grub_cryptodisk_t` structure via `grub_memcpy ()`. As size
we mistakenly use the size of the `grub_cryptodisk_t` UUID field, which
is guaranteed to be strictly bigger than the LUKS UUID field we're
copying. As a result, the copy always goes out-of-bounds and copies some
garbage from other surrounding fields. During runtime, this isn't
noticed due to the fact that we always NUL-terminate the UUID and thus
never hit the trailing garbage.

Fix the issue by using the size of the local stripped UUID field.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 grub-core/disk/luks.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index 6ae162601..76f89dd29 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -125,7 +125,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
   newdev->source_disk = NULL;
   newdev->log_sector_size = 9;
   newdev->total_length = grub_disk_get_size (disk) - newdev->offset;
-  grub_memcpy (newdev->uuid, uuid, sizeof (newdev->uuid));
+  grub_memcpy (newdev->uuid, uuid, sizeof (uuid));
   newdev->modname = "luks";
 
   /* Configure the hash used for the AF splitter and HMAC.  */
-- 
2.28.0


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

[PATCH 3/9] luks2: Fix use of incorrect index and some error messages

[Patrick Steinhardt]

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

From: Glenn Washburn <development@efficientek.com>

Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Patrick Steinhardt <ps@pks.im>
---
 grub-core/disk/luks2.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index e3ff7c83d..200f81d3a 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -275,34 +275,34 @@ luks2_get_keyslot (grub_luks2_keyslot_t *k, grub_luks2_digest_t *d, grub_luks2_s
     return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not get digests");
   for (j = 0; j < size; j++)
     {
-      if (grub_json_getchild (&digest, &digests, i) ||
+      if (grub_json_getchild (&digest, &digests, j) ||
           grub_json_getchild (&digest, &digest, 0) ||
           luks2_parse_digest (d, &digest))
-	return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse digest %"PRIuGRUB_SIZE, i);
+	return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse digest %"PRIuGRUB_SIZE, j);
 
       if ((d->keyslots & (1 << idx)))
 	break;
     }
   if (j == size)
-      return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for keyslot %"PRIuGRUB_SIZE);
+      return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for keyslot %"PRIuGRUB_SIZE, i);
 
   /* Get segment that matches the digest. */
   if (grub_json_getvalue (&segments, root, "segments") ||
       grub_json_getsize (&size, &segments))
     return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not get segments");
-  for (j = 0; j < size; j++)
+  for (i = j, j = 0; j < size; j++)
     {
-      if (grub_json_getchild (&segment, &segments, i) ||
+      if (grub_json_getchild (&segment, &segments, j) ||
 	  grub_json_getuint64 (&idx, &segment, NULL) ||
 	  grub_json_getchild (&segment, &segment, 0) ||
           luks2_parse_segment (s, &segment))
-	return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse segment %"PRIuGRUB_SIZE, i);
+	return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse segment %"PRIuGRUB_SIZE, j);
 
       if ((d->segments & (1 << idx)))
 	break;
     }
   if (j == size)
-    return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No segment for digest %"PRIuGRUB_SIZE);
+    return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No segment for digest %"PRIuGRUB_SIZE, i);
 
   return GRUB_ERR_NONE;
 }
-- 
2.28.0


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

[PATCH 4/9] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors

[Patrick Steinhardt]

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

From: Glenn Washburn <development@efficientek.com>

The total_length field is named confusingly because length usually refers to
bytes, whereas in this case its really the total number of sectors on the
device. Also counter-intuitively, grub_disk_get_size returns the total
number of device native sectors sectors. We need to convert the sectors from
the size of the underlying device to the cryptodisk sector size. And
segment.size is in bytes which need to be converted to cryptodisk sectors.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/luks2.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 200f81d3a..fa4b62825 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -416,7 +416,7 @@ luks2_decrypt_key (grub_uint8_t *out_key,
   grub_uint8_t salt[GRUB_CRYPTODISK_MAX_KEYLEN];
   grub_uint8_t *split_key = NULL;
   grub_size_t saltlen = sizeof (salt);
-  char cipher[32], *p;;
+  char cipher[32], *p;
   const gcry_md_spec_t *hash;
   gcry_err_code_t gcry_ret;
   grub_err_t ret;
@@ -602,9 +602,10 @@ luks2_recover_key (grub_disk_t disk,
       crypt->log_sector_size = sizeof (unsigned int) * 8
 		- __builtin_clz ((unsigned int) segment.sector_size) - 1;
       if (grub_strcmp (segment.size, "dynamic") == 0)
-	crypt->total_length = grub_disk_get_size (disk) - crypt->offset;
+	crypt->total_length = (grub_disk_get_size (disk) >> (crypt->log_sector_size - disk->log_sector_size))
+			       - crypt->offset;
       else
-	crypt->total_length = grub_strtoull (segment.size, NULL, 10);
+	crypt->total_length = grub_strtoull (segment.size, NULL, 10) >> crypt->log_sector_size;
 
       ret = luks2_decrypt_key (candidate_key, disk, crypt, &keyslot,
 			       (const grub_uint8_t *) passphrase, grub_strlen (passphrase));
-- 
2.28.0


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

[PATCH 5/9] luks2: Improve error reporting when decrypting/verifying key

[Patrick Steinhardt]

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

While we already set up error messages in both `luks2_verify_key()` and
`luks2_decrypt_key()`, we do not ever print them. This makes it really
hard to discover why a given key actually failed to decrypt a disk.

Improve this by including the error message in the user-visible output.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 grub-core/disk/luks2.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index fa4b62825..7c530dd1a 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -611,14 +611,16 @@ luks2_recover_key (grub_disk_t disk,
 			       (const grub_uint8_t *) passphrase, grub_strlen (passphrase));
       if (ret)
 	{
-	  grub_dprintf ("luks2", "Decryption with keyslot %"PRIuGRUB_SIZE" failed\n", i);
+	  grub_dprintf ("luks2", "Decryption with keyslot %"PRIuGRUB_SIZE" failed: %s\n",
+			i, grub_errmsg);
 	  continue;
 	}
 
       ret = luks2_verify_key (&digest, candidate_key, keyslot.key_size);
       if (ret)
 	{
-	  grub_dprintf ("luks2", "Could not open keyslot %"PRIuGRUB_SIZE"\n", i);
+	  grub_dprintf ("luks2", "Could not open keyslot %"PRIuGRUB_SIZE": %s\n",
+			i, grub_errmsg);
 	  continue;
 	}
 
-- 
2.28.0


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

[PATCH 6/9] cryptodisk: Unregister cryptomount command when removing module

[Patrick Steinhardt]

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

From: Glenn Washburn <development@efficientek.com>

Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Patrick Steinhardt <ps@pks.im>
---
 grub-core/disk/cryptodisk.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 1897acc4b..b2c6e9a7d 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1311,5 +1311,6 @@ GRUB_MOD_FINI (cryptodisk)
 {
   grub_disk_dev_unregister (&grub_cryptodisk_dev);
   cryptodisk_cleanup ();
+  grub_unregister_extcmd (cmd);
   grub_procfs_unregister (&luks_script);
 }
-- 
2.28.0


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

[PATCH 7/9] cryptodisk: Incorrect calculation of start sector for grub_disk_read in grub_cryptodisk_read

[Patrick Steinhardt]

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

From: Glenn Washburn <development@efficientek.com>

Here dev is a grub_cryptodisk_t and dev->offset is offset in sectors of size
native to the cryptodisk device. The sector is correctly transformed into
native grub sector size, but then added to dev->offset which is not
transformed. It would be nice if the type system would help us with this.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/cryptodisk.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index b2c6e9a7d..76ec2d4b9 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -757,8 +757,8 @@ grub_cryptodisk_read (grub_disk_t disk, grub_disk_addr_t sector,
 		size, sector, dev->offset);
 
   err = grub_disk_read (dev->source_disk,
-			(sector << (disk->log_sector_size
-				   - GRUB_DISK_SECTOR_BITS)) + dev->offset, 0,
+			((sector + dev->offset) << (disk->log_sector_size
+						   - GRUB_DISK_SECTOR_BITS)), 0,
 			size << disk->log_sector_size, buf);
   if (err)
     {
-- 
2.28.0


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

[PATCH 8/9] cryptodisk: Fix cipher IV mode 'plain64' always being set as 'plain'

[Patrick Steinhardt]

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

From: Glenn Washburn <development@efficientek.com>

Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Patrick Steinhardt <ps@pks.im>
---
 grub-core/disk/cryptodisk.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 76ec2d4b9..74ab54eaa 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -501,10 +501,10 @@ grub_cryptodisk_setcipher (grub_cryptodisk_t crypt, const char *ciphername, cons
 
   if (cipheriv == NULL)
       ;
-  else if (grub_memcmp (cipheriv, "plain", sizeof ("plain") - 1) == 0)
-      mode_iv = GRUB_CRYPTODISK_MODE_IV_PLAIN;
   else if (grub_memcmp (cipheriv, "plain64", sizeof ("plain64") - 1) == 0)
       mode_iv = GRUB_CRYPTODISK_MODE_IV_PLAIN64;
+  else if (grub_memcmp (cipheriv, "plain", sizeof ("plain") - 1) == 0)
+      mode_iv = GRUB_CRYPTODISK_MODE_IV_PLAIN;
   else if (grub_memcmp (cipheriv, "benbi", sizeof ("benbi") - 1) == 0)
     {
       if (cipher->cipher->blocksize & (cipher->cipher->blocksize - 1)
-- 
2.28.0


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

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

[Patrick Steinhardt]

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

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
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>
---
 grub-core/disk/cryptodisk.c | 47 +++++++++++++++++++++----------------
 grub-core/disk/luks.c       |  5 ++--
 grub-core/disk/luks2.c      |  6 ++++-
 include/grub/cryptodisk.h   |  2 +-
 4 files changed, 36 insertions(+), 24 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 74ab54eaa..e96d123de 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -33,6 +33,9 @@
 
 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[] =
@@ -224,7 +227,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 sector_size,
+			   int do_encrypt)
 {
   grub_size_t i;
   gcry_err_code_t err;
@@ -237,12 +241,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 << 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;
 
       if (dev->rekey)
 	{
@@ -270,7 +275,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 << 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 +286,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 << (sector_size - CRYPT_LOG_SECTOR_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[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)
 				    & 0xFFFFFFFF);
 	  break;
 	case GRUB_CRYPTODISK_MODE_IV_BENBI:
@@ -311,10 +318,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 << 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);
 	  if (err)
 	    return err;
 	  break;
@@ -322,10 +329,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 << 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);
 	  if (err)
 	    return err;
 	  break;
@@ -337,7 +344,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 << sector_size);
 		 j += dev->cipher->cipher->blocksize)
 	      {
 		grub_crypto_xor (data + i + j, data + i + j, iv,
@@ -368,11 +375,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 << sector_size));
 	    else
 	      err = grub_crypto_ecb_decrypt (dev->cipher, data + i, 
 					     data + i,
-					     (1U << dev->log_sector_size));
+					     (1U << sector_size));
 	    if (err)
 	      return err;
 	    lrw_xor (&sec, dev, data + i);
@@ -381,10 +388,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 << sector_size));
 	  else
 	    err = grub_crypto_ecb_decrypt (dev->cipher, data + i, data + i,
-					   (1U << dev->log_sector_size));
+					   (1U << sector_size));
 	  if (err)
 	    return err;
 	  break;
@@ -399,9 +406,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 sector_size)
 {
-  return grub_cryptodisk_endecrypt (dev, data, len, sector, 0);
+  return grub_cryptodisk_endecrypt (dev, data, len, sector, sector_size, 0);
 }
 
 grub_err_t
@@ -767,7 +774,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 +815,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 76f89dd29..913fb7a9f 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -30,6 +30,7 @@
 GRUB_MOD_LICENSE ("GPLv3+");
 
 #define MAX_PASSPHRASE 256
+#define LOG_SECTOR_SIZE 9
 
 #define LUKS_KEY_ENABLED  0x00AC71F3
 
@@ -123,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 = LOG_SECTOR_SIZE;
   newdev->total_length = grub_disk_get_size (disk) - newdev->offset;
   grub_memcpy (newdev->uuid, uuid, sizeof (uuid));
   newdev->modname = "luks";
@@ -246,7 +247,7 @@ 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, 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 7c530dd1a..b6c0e384d 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -491,7 +491,11 @@ luks2_decrypt_key (grub_uint8_t *out_key,
       goto err;
     }
 
-  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
+   */
+  gcry_ret = grub_cryptodisk_decrypt (crypt, split_key, k->area.size, 0, 9);
   if (gcry_ret)
     {
       ret = grub_crypto_gcry_error (gcry_ret);
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index e1b21e785..06653a622 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -139,7 +139,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 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 2/9] luks: Fix out-of-bounds copy of UUID

[Denis 'GNUtoo' Carikli]

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

On Sun, 23 Aug 2020 12:59:57 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> When configuring a LUKS disk, we copy over the UUID from the LUKS
> header into the new `grub_cryptodisk_t` structure via `grub_memcpy
> ()`. As size we mistakenly use the size of the `grub_cryptodisk_t`
> UUID field, which is guaranteed to be strictly bigger than the LUKS
> UUID field we're copying. As a result, the copy always goes
> out-of-bounds and copies some garbage from other surrounding fields.
> During runtime, this isn't noticed due to the fact that we always
> NUL-terminate the UUID and thus never hit the trailing garbage.
> 
> Fix the issue by using the size of the local stripped UUID field.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  grub-core/disk/luks.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> index 6ae162601..76f89dd29 100644
> --- a/grub-core/disk/luks.c
> +++ b/grub-core/disk/luks.c
> @@ -125,7 +125,7 @@ configure_ciphers (grub_disk_t disk, const char
> *check_uuid, newdev->source_disk = NULL;
>    newdev->log_sector_size = 9;
>    newdev->total_length = grub_disk_get_size (disk) - newdev->offset;
> -  grub_memcpy (newdev->uuid, uuid, sizeof (newdev->uuid));
> +  grub_memcpy (newdev->uuid, uuid, sizeof (uuid));

Is the fact that the real UUID size is 37 (36 + \0) instead of 40 an
issue?

In grub-core/disk/luks.c we have:
> /* On disk LUKS header */
> struct grub_luks_phdr
> {
>   [...]
>   char uuid[40];
>   [...]
> } GRUB_PACKED;
So here we use 40.

It's then used to define the size of the 'uuid' local variable that is
used grub_memcpy:
> static grub_cryptodisk_t
> luks_scan (grub_disk_t disk, const char *check_uuid, int check_boot,
> 	   grub_file_t hdr)
> {
>   [...]
>   char uuid[sizeof (header.uuid) + 1];
>   [...]
>   grub_memcpy (newdev->uuid, uuid, sizeof (newdev->uuid));
>   [...]
> }

However in lib/luks1/luks.h in cryptsetup source code we have:
> /* Actually we need only 37, but we don't want struct autoaligning to kick in */
> #define UUID_STRING_L 40

And still in cryptsetup source code in the LUKS2_luks2_to_luks1 
function in lib/luks2/luks2_luks1_convert.c we have:
> strncpy(hdr1->uuid, hdr2->uuid, UUID_STRING_L); /* max 36 chars */
> hdr1->uuid[UUID_STRING_L-1] = '\0';

Denis.

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

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

[Glenn Washburn]

On Sun, 23 Aug 2020 12:59:47 +0200
Patrick Steinhardt <ps@pks.im> wrote:

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

Its not clear to me what "total size" means here.  However,
`grub_disk_get_size` returns sectors for non-partition disks as well.
Note, that it returns the total number of grub native sector-sized
sectors (ie 512 byte sectors).  I do think that the _size suffix is
misleading and should be named `grub_disk_get_sectors` or something
similar.

Is there something I can do to clear up the uncertainty around this
fix?  I suspect part of the confusion lies in the fact that the
total_length field is actually in cryptodisk sector-sized sectors. We
know this because in `grub_cryptodisk_open` in cryptodisk.c the
total_length field is being set unmodified to the total_sectors field of
a `grub_disk_t` and `grub_disk_get_size` assume that total_sectors
needs to be converted from disk native to grub native sector-sized
sectors.

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

In case its missed, I've explained this in my response to your response
to my original patch.

> - 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 see you fixed the bug in my patch where IV_PLAIN IVs were not being
calculated, thanks.  Considering grubs slow release cycle, I think its
better to include this or at least make a note that non-512-byte
sectors are currently not supported and have cryptomount fail with an
appropriate error message if it detects trying to open a LUKS2 device
with sector size not equal to 512.  As it is, I think LUKS2 support
will have people thinking that non-default sector sizes are supported.

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

I would like to get these patches in a quickly as possible and I
suspect this patchset is probably that route, so I'm pleased to have
these patches in here.  

As a reminder, my CRYPTOMOUNT-TESTS patchset can test the full patch
set (not individual patches because non-512-byte sector support
requires multiple patches in this patchset).

Glenn

Re: [PATCH 3/9] luks2: Fix use of incorrect index and some error messages

[Glenn Washburn]

Was it intentional to include the old/original version of this patch
which you requested to be changed to make it more readable?  There is
an updated patch with subject "[CRYPTO-LUKS v1 18/19] luks2: Fix use of
incorrect index and some error messages.".

Glenn

On Sun, 23 Aug 2020 13:03:07 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> From: Glenn Washburn <development@efficientek.com>
> 
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> Reviewed-by: Patrick Steinhardt <ps@pks.im>
> ---
>  grub-core/disk/luks2.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> index e3ff7c83d..200f81d3a 100644
> --- a/grub-core/disk/luks2.c
> +++ b/grub-core/disk/luks2.c
> @@ -275,34 +275,34 @@ luks2_get_keyslot (grub_luks2_keyslot_t *k,
> grub_luks2_digest_t *d, grub_luks2_s return grub_error
> (GRUB_ERR_BAD_ARGUMENT, "Could not get digests"); for (j = 0; j <
> size; j++) {
> -      if (grub_json_getchild (&digest, &digests, i) ||
> +      if (grub_json_getchild (&digest, &digests, j) ||
>            grub_json_getchild (&digest, &digest, 0) ||
>            luks2_parse_digest (d, &digest))
> -	return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse
> digest %"PRIuGRUB_SIZE, i);
> +	return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse
> digest %"PRIuGRUB_SIZE, j); 
>        if ((d->keyslots & (1 << idx)))
>  	break;
>      }
>    if (j == size)
> -      return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for
> keyslot %"PRIuGRUB_SIZE);
> +      return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for
> keyslot %"PRIuGRUB_SIZE, i); 
>    /* Get segment that matches the digest. */
>    if (grub_json_getvalue (&segments, root, "segments") ||
>        grub_json_getsize (&size, &segments))
>      return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not get
> segments");
> -  for (j = 0; j < size; j++)
> +  for (i = j, j = 0; j < size; j++)
>      {
> -      if (grub_json_getchild (&segment, &segments, i) ||
> +      if (grub_json_getchild (&segment, &segments, j) ||
>  	  grub_json_getuint64 (&idx, &segment, NULL) ||
>  	  grub_json_getchild (&segment, &segment, 0) ||
>            luks2_parse_segment (s, &segment))
> -	return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse
> segment %"PRIuGRUB_SIZE, i);
> +	return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse
> segment %"PRIuGRUB_SIZE, j); 
>        if ((d->segments & (1 << idx)))
>  	break;
>      }
>    if (j == size)
> -    return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No segment for
> digest %"PRIuGRUB_SIZE);
> +    return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No segment for
> digest %"PRIuGRUB_SIZE, i); 
>    return GRUB_ERR_NONE;
>  }

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

[Patrick Steinhardt]

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

On Mon, Aug 24, 2020 at 01:22:20AM -0500, Glenn Washburn wrote:
> On Sun, 23 Aug 2020 12:59:47 +0200
> Patrick Steinhardt <ps@pks.im> wrote:
> 
> > 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.
> 
> Its not clear to me what "total size" means here.  However,
> `grub_disk_get_size` returns sectors for non-partition disks as well.
> Note, that it returns the total number of grub native sector-sized
> sectors (ie 512 byte sectors).  I do think that the _size suffix is
> misleading and should be named `grub_disk_get_sectors` or something
> similar.

Oh, yeah. I've misunderstood the translation from disk sector size to
GRUB-native sector size.

I heavily agree with you that there should be some code cleanup after
v2.06 which does s/grub_disk_get_size/grub_disk_get_sectors/ for this
and other related code. It's just too easy to shoot yourself in the foot
here if you're not knowing a 100% what you're doing.

> Is there something I can do to clear up the uncertainty around this
> fix?  I suspect part of the confusion lies in the fact that the
> total_length field is actually in cryptodisk sector-sized sectors. We
> know this because in `grub_cryptodisk_open` in cryptodisk.c the
> total_length field is being set unmodified to the total_sectors field of
> a `grub_disk_t` and `grub_disk_get_size` assume that total_sectors
> needs to be converted from disk native to grub native sector-sized
> sectors.

No, I've read it again and will add my Reviewed-by for this patch.

> > - 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 see you fixed the bug in my patch where IV_PLAIN IVs were not being
> calculated, thanks.  Considering grubs slow release cycle, I think its
> better to include this or at least make a note that non-512-byte
> sectors are currently not supported and have cryptomount fail with an
> appropriate error message if it detects trying to open a LUKS2 device
> with sector size not equal to 512.  As it is, I think LUKS2 support
> will have people thinking that non-default sector sizes are supported.

I should've added a note in here about my fixup. And let's do
include it then.

> > 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.
> 
> I would like to get these patches in a quickly as possible and I
> suspect this patchset is probably that route, so I'm pleased to have
> these patches in here.  
> 
> As a reminder, my CRYPTOMOUNT-TESTS patchset can test the full patch
> set (not individual patches because non-512-byte sector support
> requires multiple patches in this patchset).

I'll have a look at that patchset soonish.

Patrick

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

Re: [PATCH 3/9] luks2: Fix use of incorrect index and some error messages

[Patrick Steinhardt]

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

On Mon, Aug 24, 2020 at 01:30:05AM -0500, Glenn Washburn wrote:
> Was it intentional to include the old/original version of this patch
> which you requested to be changed to make it more readable?  There is
> an updated patch with subject "[CRYPTO-LUKS v1 18/19] luks2: Fix use of
> incorrect index and some error messages.".

Nope, not at all. I've briefly wondered about the `i = j` thing again
but didn't connect the dots. I'll replace it with the newer version in
v2 of this patch series.

Patrick

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

Re: [PATCH 2/9] luks: Fix out-of-bounds copy of UUID

[Patrick Steinhardt]

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

On Sun, Aug 23, 2020 at 11:34:51PM +0200, Denis 'GNUtoo' Carikli wrote:
> On Sun, 23 Aug 2020 12:59:57 +0200
> Patrick Steinhardt <ps@pks.im> wrote:
> 
> > When configuring a LUKS disk, we copy over the UUID from the LUKS
> > header into the new `grub_cryptodisk_t` structure via `grub_memcpy
> > ()`. As size we mistakenly use the size of the `grub_cryptodisk_t`
> > UUID field, which is guaranteed to be strictly bigger than the LUKS
> > UUID field we're copying. As a result, the copy always goes
> > out-of-bounds and copies some garbage from other surrounding fields.
> > During runtime, this isn't noticed due to the fact that we always
> > NUL-terminate the UUID and thus never hit the trailing garbage.
> > 
> > Fix the issue by using the size of the local stripped UUID field.
> > 
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  grub-core/disk/luks.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> > index 6ae162601..76f89dd29 100644
> > --- a/grub-core/disk/luks.c
> > +++ b/grub-core/disk/luks.c
> > @@ -125,7 +125,7 @@ configure_ciphers (grub_disk_t disk, const char
> > *check_uuid, newdev->source_disk = NULL;
> >    newdev->log_sector_size = 9;
> >    newdev->total_length = grub_disk_get_size (disk) - newdev->offset;
> > -  grub_memcpy (newdev->uuid, uuid, sizeof (newdev->uuid));
> > +  grub_memcpy (newdev->uuid, uuid, sizeof (uuid));
> 
> Is the fact that the real UUID size is 37 (36 + \0) instead of 40 an
> issue?

I think you're right. When copying `header.uuid` into the local
variable, we strip all dashes and will thus only copy 36 bytes plus the
trailing NUL byte. Which effectively means that the last 4 bytes of the
local variable aren't initialized, but we still copy them over into the
new device.

It's probably not going to cause any problems, but we should still do
the right thing and just zero-initialize the UUID variable.

Patrick

> In grub-core/disk/luks.c we have:
> > /* On disk LUKS header */
> > struct grub_luks_phdr
> > {
> >   [...]
> >   char uuid[40];
> >   [...]
> > } GRUB_PACKED;
> So here we use 40.
> 
> It's then used to define the size of the 'uuid' local variable that is
> used grub_memcpy:
> > static grub_cryptodisk_t
> > luks_scan (grub_disk_t disk, const char *check_uuid, int check_boot,
> > 	   grub_file_t hdr)
> > {
> >   [...]
> >   char uuid[sizeof (header.uuid) + 1];
> >   [...]
> >   grub_memcpy (newdev->uuid, uuid, sizeof (newdev->uuid));
> >   [...]
> > }
> 
> However in lib/luks1/luks.h in cryptsetup source code we have:
> > /* Actually we need only 37, but we don't want struct autoaligning to kick in */
> > #define UUID_STRING_L 40
> 
> And still in cryptsetup source code in the LUKS2_luks2_to_luks1 
> function in lib/luks2/luks2_luks1_convert.c we have:
> > strncpy(hdr1->uuid, hdr2->uuid, UUID_STRING_L); /* max 36 chars */
> > hdr1->uuid[UUID_STRING_L-1] = '\0';
> 
> Denis.



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

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

[Patrick Steinhardt]

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

Hi,

this is the second version of cryptodisk fixes which I deem to be
important for the upcoming v2.06 release. Changes:

    - Patch 2: we're now zeroing out the UUID variable to avoid copying
               over uninitialized bytes. Thanks for spotting, Dennis!

    - Patch 3: I've replaced it with the updated version he's posted
               to address my feedback.

    - Patch 7: Updated by Glenn to now also fix writes.

I didn't yet get your test series to work, Glenn. I'll try again on
another day as I'm not on top of things today. Meanwhile, could you
please give it a go with this updated patch series?

Regards
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 | 63 ++++++++++++++++++++-----------------
 grub-core/disk/luks.c       |  8 +++--
 grub-core/disk/luks2.c      | 54 +++++++++++++++++--------------
 grub-core/lib/json/json.h   |  9 +++---
 include/grub/cryptodisk.h   |  2 +-
 include/grub/disk.h         |  7 +++++
 6 files changed, 83 insertions(+), 60 deletions(-)

Range-diff against v1:
 1:  ee402de4d =  1:  ee402de4d json: Remove invalid typedef redefinition
 2:  8668b265f !  2:  5ecb9a4eb luks: Fix out-of-bounds copy of UUID
    @@ Commit message
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## grub-core/disk/luks.c ##
    +@@ grub-core/disk/luks.c: configure_ciphers (grub_disk_t disk, const char *check_uuid,
    +       || grub_be_to_cpu16 (header.version) != 1)
    +     return NULL;
    + 
    ++  grub_memset (uuid, 0, sizeof (uuid));
    +   optr = uuid;
    +   for (iptr = header.uuid; iptr < &header.uuid[ARRAY_SIZE (header.uuid)];
    +        iptr++)
     @@ grub-core/disk/luks.c: configure_ciphers (grub_disk_t disk, const char *check_uuid,
        newdev->source_disk = NULL;
        newdev->log_sector_size = 9;
 3:  7cf9d9526 !  3:  f8da5b4b4 luks2: Fix use of incorrect index and some error messages
    @@ Commit message
         Reviewed-by: Patrick Steinhardt <ps@pks.im>
     
      ## grub-core/disk/luks2.c ##
    -@@ grub-core/disk/luks2.c: luks2_get_keyslot (grub_luks2_keyslot_t *k, grub_luks2_digest_t *d, grub_luks2_s
    +@@ grub-core/disk/luks2.c: luks2_parse_digest (grub_luks2_digest_t *out, const grub_json_t *digest)
    + 
    + static grub_err_t
    + luks2_get_keyslot (grub_luks2_keyslot_t *k, grub_luks2_digest_t *d, grub_luks2_segment_t *s,
    +-		   const grub_json_t *root, grub_size_t i)
    ++		   const grub_json_t *root, grub_size_t keyslot_idx)
    + {
    +   grub_json_t keyslots, keyslot, digests, digest, segments, segment;
    +-  grub_size_t j, size;
    +-  grub_uint64_t idx;
    ++  grub_size_t i, size;
    ++  grub_uint64_t keyslot_key, digest_key, segment_key;
    + 
    +   /* Get nth keyslot */
    +   if (grub_json_getvalue (&keyslots, root, "keyslots") ||
    +-      grub_json_getchild (&keyslot, &keyslots, i) ||
    +-      grub_json_getuint64 (&idx, &keyslot, NULL) ||
    ++      grub_json_getchild (&keyslot, &keyslots, keyslot_idx) ||
    ++      grub_json_getuint64 (&keyslot_key, &keyslot, NULL) ||
    +       grub_json_getchild (&keyslot, &keyslot, 0) ||
    +       luks2_parse_keyslot (k, &keyslot))
    +-    return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse keyslot %"PRIuGRUB_SIZE, i);
    ++    return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse keyslot index %"PRIuGRUB_SIZE, keyslot_idx);
    + 
    +   /* Get digest that matches the keyslot. */
    +   if (grub_json_getvalue (&digests, root, "digests") ||
    +       grub_json_getsize (&size, &digests))
          return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not get digests");
    -   for (j = 0; j < size; j++)
    +-  for (j = 0; j < size; j++)
    ++  for (i = 0; i < size; i++)
          {
    --      if (grub_json_getchild (&digest, &digests, i) ||
    -+      if (grub_json_getchild (&digest, &digests, j) ||
    +       if (grub_json_getchild (&digest, &digests, i) ||
    ++	  grub_json_getuint64 (&digest_key, &digest, NULL) ||
                grub_json_getchild (&digest, &digest, 0) ||
                luks2_parse_digest (d, &digest))
     -	return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse digest %"PRIuGRUB_SIZE, i);
    -+	return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse digest %"PRIuGRUB_SIZE, j);
    ++	return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse digest index %"PRIuGRUB_SIZE, i);
      
    -       if ((d->keyslots & (1 << idx)))
    +-      if ((d->keyslots & (1 << idx)))
    ++      if ((d->keyslots & (1 << keyslot_key)))
      	break;
          }
    -   if (j == size)
    +-  if (j == size)
     -      return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for keyslot %"PRIuGRUB_SIZE);
    -+      return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for keyslot %"PRIuGRUB_SIZE, i);
    ++  if (i == size)
    ++      return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for keyslot \"%"PRIuGRUB_UINT64_T"\"", keyslot_key);
      
        /* Get segment that matches the digest. */
        if (grub_json_getvalue (&segments, root, "segments") ||
            grub_json_getsize (&size, &segments))
          return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not get segments");
     -  for (j = 0; j < size; j++)
    -+  for (i = j, j = 0; j < size; j++)
    ++  for (i = 0; i < size; i++)
          {
    --      if (grub_json_getchild (&segment, &segments, i) ||
    -+      if (grub_json_getchild (&segment, &segments, j) ||
    - 	  grub_json_getuint64 (&idx, &segment, NULL) ||
    +       if (grub_json_getchild (&segment, &segments, i) ||
    +-	  grub_json_getuint64 (&idx, &segment, NULL) ||
    ++	  grub_json_getuint64 (&segment_key, &segment, NULL) ||
      	  grub_json_getchild (&segment, &segment, 0) ||
                luks2_parse_segment (s, &segment))
     -	return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse segment %"PRIuGRUB_SIZE, i);
    -+	return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse segment %"PRIuGRUB_SIZE, j);
    ++	return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse segment index %"PRIuGRUB_SIZE, i);
      
    -       if ((d->segments & (1 << idx)))
    +-      if ((d->segments & (1 << idx)))
    ++      if ((d->segments & (1 << segment_key)))
      	break;
          }
    -   if (j == size)
    +-  if (j == size)
     -    return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No segment for digest %"PRIuGRUB_SIZE);
    -+    return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No segment for digest %"PRIuGRUB_SIZE, i);
    ++  if (i == size)
    ++    return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No segment for digest \"%"PRIuGRUB_UINT64_T"\"", digest_key);
      
        return GRUB_ERR_NONE;
      }
 4:  c3bf40e31 =  4:  150491a07 luks2: grub_cryptodisk_t->total_length is the max number of device native sectors
 5:  4b820da6c =  5:  7dbfad568 luks2: Improve error reporting when decrypting/verifying key
 6:  f7176a87e =  6:  dbf25a0ae cryptodisk: Unregister cryptomount command when removing module
 7:  13d0720d6 !  7:  4ee7f8774 cryptodisk: Incorrect calculation of start sector for grub_disk_read in grub_cryptodisk_read
    @@ Metadata
     Author: Glenn Washburn <development@efficientek.com>
     
      ## Commit message ##
    -    cryptodisk: Incorrect calculation of start sector for grub_disk_read in grub_cryptodisk_read
    +    cryptodisk: Fix incorrect calculation of start sector
     
         Here dev is a grub_cryptodisk_t and dev->offset is offset in sectors of size
         native to the cryptodisk device. The sector is correctly transformed into
    @@ Commit message
         transformed. It would be nice if the type system would help us with this.
     
         Signed-off-by: Glenn Washburn <development@efficientek.com>
    +    Reviewed-by: Patrick Steinhardt <ps@pks.im>
     
      ## grub-core/disk/cryptodisk.c ##
     @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_read (grub_disk_t disk, grub_disk_addr_t sector,
    @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_read (grub_disk_t disk, grub_disk_a
        err = grub_disk_read (dev->source_disk,
     -			(sector << (disk->log_sector_size
     -				   - GRUB_DISK_SECTOR_BITS)) + dev->offset, 0,
    -+			((sector + dev->offset) << (disk->log_sector_size
    -+						   - GRUB_DISK_SECTOR_BITS)), 0,
    - 			size << disk->log_sector_size, buf);
    +-			size << disk->log_sector_size, buf);
    ++			grub_disk_from_native_sector (disk, sector + dev->offset),
    ++			0, size << disk->log_sector_size, buf);
        if (err)
          {
    +       grub_dprintf ("cryptodisk", "grub_disk_read failed with error %d\n", err);
    +@@ grub-core/disk/cryptodisk.c: grub_cryptodisk_write (grub_disk_t disk, grub_disk_addr_t sector,
    +     }
    + 
    +   /* Since ->write was called so disk.mod is loaded but be paranoid  */
    +-  
    ++  sector = sector + dev->offset;
    +   if (grub_disk_write_weak)
    +     err = grub_disk_write_weak (dev->source_disk,
    +-				(sector << (disk->log_sector_size
    +-					    - GRUB_DISK_SECTOR_BITS))
    +-				+ dev->offset,
    ++				grub_disk_from_native_sector (disk, sector),
    + 				0, size << disk->log_sector_size, tmp);
    +   else
    +     err = grub_error (GRUB_ERR_BUG, "disk.mod not loaded");
    +
    + ## include/grub/disk.h ##
    +@@ include/grub/disk.h: typedef struct grub_disk_memberlist *grub_disk_memberlist_t;
    + /* Return value of grub_disk_get_size() in case disk size is unknown. */
    + #define GRUB_DISK_SIZE_UNKNOWN	 0xffffffffffffffffULL
    + 
    ++/* Convert to grub native disk sized sector from disk sized sector */
    ++static inline grub_disk_addr_t
    ++grub_disk_from_native_sector (grub_disk_t disk, grub_disk_addr_t sector)
    ++{
    ++  return sector << (disk->log_sector_size - GRUB_DISK_SECTOR_BITS);
    ++}
    ++
    + /* This is called from the memory manager.  */
    + void grub_disk_cache_invalidate_all (void);
    + 
 8:  12bc26698 =  8:  4aecb174b cryptodisk: Fix cipher IV mode 'plain64' always being set as 'plain'
 9:  32b463e00 !  9:  f520aecfa cryptodisk: Properly handle non-512 byte sized sectors
    @@ Commit message
         aestetically pleasing than desired.
     
         Signed-off-by: Glenn Washburn <development@efficientek.com>
    +    Reviewed-by: Patrick Steinhardt <ps@pks.im>
     
      ## grub-core/disk/cryptodisk.c ##
     @@
-- 
2.28.0


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

[PATCH v2 1/9] json: Remove invalid typedef redefinition

[Patrick Steinhardt]

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

The C standard does not allow for typedef redefinitions, even if they
map to the same underlying type. In order to avoid including the
"jsmn.h" in "json.h" and thus exposing jsmn's internals, we have exactly
such a forward-declaring typedef in "json.h". If enforcing the GNU99 C
standard, clang may generate a warning about this non-standard
construct.

Fix the issue by using a simple `struct jsmntok` forward declaration
instead of using a typedef.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Tested-by: Chuck Tuffli <chuck@freebsd.org>
---
 grub-core/lib/json/json.h | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/grub-core/lib/json/json.h b/grub-core/lib/json/json.h
index 01614f6df..657902cfc 100644
--- a/grub-core/lib/json/json.h
+++ b/grub-core/lib/json/json.h
@@ -36,13 +36,14 @@ enum grub_json_type
 };
 typedef enum grub_json_type grub_json_type_t;
 
-typedef struct jsmntok jsmntok_t;
+/* Forward-declaration to avoid including "jsmn.h" */
+struct jsmntok;
 
 struct grub_json
 {
-  jsmntok_t   *tokens;
-  char	      *string;
-  grub_size_t idx;
+  struct jsmntok *tokens;
+  char		 *string;
+  grub_size_t	 idx;
 };
 typedef struct grub_json grub_json_t;
 
-- 
2.28.0


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

[PATCH v2 2/9] luks: Fix out-of-bounds copy of UUID

[Patrick Steinhardt]

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

When configuring a LUKS disk, we copy over the UUID from the LUKS header
into the new `grub_cryptodisk_t` structure via `grub_memcpy ()`. As size
we mistakenly use the size of the `grub_cryptodisk_t` UUID field, which
is guaranteed to be strictly bigger than the LUKS UUID field we're
copying. As a result, the copy always goes out-of-bounds and copies some
garbage from other surrounding fields. During runtime, this isn't
noticed due to the fact that we always NUL-terminate the UUID and thus
never hit the trailing garbage.

Fix the issue by using the size of the local stripped UUID field.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 grub-core/disk/luks.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index 6ae162601..59702067a 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -95,6 +95,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
       || grub_be_to_cpu16 (header.version) != 1)
     return NULL;
 
+  grub_memset (uuid, 0, sizeof (uuid));
   optr = uuid;
   for (iptr = header.uuid; iptr < &header.uuid[ARRAY_SIZE (header.uuid)];
        iptr++)
@@ -125,7 +126,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
   newdev->source_disk = NULL;
   newdev->log_sector_size = 9;
   newdev->total_length = grub_disk_get_size (disk) - newdev->offset;
-  grub_memcpy (newdev->uuid, uuid, sizeof (newdev->uuid));
+  grub_memcpy (newdev->uuid, uuid, sizeof (uuid));
   newdev->modname = "luks";
 
   /* Configure the hash used for the AF splitter and HMAC.  */
-- 
2.28.0


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

[PATCH v2 3/9] luks2: Fix use of incorrect index and some error messages

[Patrick Steinhardt]

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

From: Glenn Washburn <development@efficientek.com>

Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Patrick Steinhardt <ps@pks.im>
---
 grub-core/disk/luks2.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index e3ff7c83d..c4c6ac90c 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -255,54 +255,55 @@ luks2_parse_digest (grub_luks2_digest_t *out, const grub_json_t *digest)
 
 static grub_err_t
 luks2_get_keyslot (grub_luks2_keyslot_t *k, grub_luks2_digest_t *d, grub_luks2_segment_t *s,
-		   const grub_json_t *root, grub_size_t i)
+		   const grub_json_t *root, grub_size_t keyslot_idx)
 {
   grub_json_t keyslots, keyslot, digests, digest, segments, segment;
-  grub_size_t j, size;
-  grub_uint64_t idx;
+  grub_size_t i, size;
+  grub_uint64_t keyslot_key, digest_key, segment_key;
 
   /* Get nth keyslot */
   if (grub_json_getvalue (&keyslots, root, "keyslots") ||
-      grub_json_getchild (&keyslot, &keyslots, i) ||
-      grub_json_getuint64 (&idx, &keyslot, NULL) ||
+      grub_json_getchild (&keyslot, &keyslots, keyslot_idx) ||
+      grub_json_getuint64 (&keyslot_key, &keyslot, NULL) ||
       grub_json_getchild (&keyslot, &keyslot, 0) ||
       luks2_parse_keyslot (k, &keyslot))
-    return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse keyslot %"PRIuGRUB_SIZE, i);
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse keyslot index %"PRIuGRUB_SIZE, keyslot_idx);
 
   /* Get digest that matches the keyslot. */
   if (grub_json_getvalue (&digests, root, "digests") ||
       grub_json_getsize (&size, &digests))
     return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not get digests");
-  for (j = 0; j < size; j++)
+  for (i = 0; i < size; i++)
     {
       if (grub_json_getchild (&digest, &digests, i) ||
+	  grub_json_getuint64 (&digest_key, &digest, NULL) ||
           grub_json_getchild (&digest, &digest, 0) ||
           luks2_parse_digest (d, &digest))
-	return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse digest %"PRIuGRUB_SIZE, i);
+	return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse digest index %"PRIuGRUB_SIZE, i);
 
-      if ((d->keyslots & (1 << idx)))
+      if ((d->keyslots & (1 << keyslot_key)))
 	break;
     }
-  if (j == size)
-      return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for keyslot %"PRIuGRUB_SIZE);
+  if (i == size)
+      return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for keyslot \"%"PRIuGRUB_UINT64_T"\"", keyslot_key);
 
   /* Get segment that matches the digest. */
   if (grub_json_getvalue (&segments, root, "segments") ||
       grub_json_getsize (&size, &segments))
     return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not get segments");
-  for (j = 0; j < size; j++)
+  for (i = 0; i < size; i++)
     {
       if (grub_json_getchild (&segment, &segments, i) ||
-	  grub_json_getuint64 (&idx, &segment, NULL) ||
+	  grub_json_getuint64 (&segment_key, &segment, NULL) ||
 	  grub_json_getchild (&segment, &segment, 0) ||
           luks2_parse_segment (s, &segment))
-	return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse segment %"PRIuGRUB_SIZE, i);
+	return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse segment index %"PRIuGRUB_SIZE, i);
 
-      if ((d->segments & (1 << idx)))
+      if ((d->segments & (1 << segment_key)))
 	break;
     }
-  if (j == size)
-    return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No segment for digest %"PRIuGRUB_SIZE);
+  if (i == size)
+    return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No segment for digest \"%"PRIuGRUB_UINT64_T"\"", digest_key);
 
   return GRUB_ERR_NONE;
 }
-- 
2.28.0


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

[PATCH v2 4/9] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors

[Patrick Steinhardt]

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

From: Glenn Washburn <development@efficientek.com>

The total_length field is named confusingly because length usually refers to
bytes, whereas in this case its really the total number of sectors on the
device. Also counter-intuitively, grub_disk_get_size returns the total
number of device native sectors sectors. We need to convert the sectors from
the size of the underlying device to the cryptodisk sector size. And
segment.size is in bytes which need to be converted to cryptodisk sectors.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/luks2.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index c4c6ac90c..5f15a4d2c 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -417,7 +417,7 @@ luks2_decrypt_key (grub_uint8_t *out_key,
   grub_uint8_t salt[GRUB_CRYPTODISK_MAX_KEYLEN];
   grub_uint8_t *split_key = NULL;
   grub_size_t saltlen = sizeof (salt);
-  char cipher[32], *p;;
+  char cipher[32], *p;
   const gcry_md_spec_t *hash;
   gcry_err_code_t gcry_ret;
   grub_err_t ret;
@@ -603,9 +603,10 @@ luks2_recover_key (grub_disk_t disk,
       crypt->log_sector_size = sizeof (unsigned int) * 8
 		- __builtin_clz ((unsigned int) segment.sector_size) - 1;
       if (grub_strcmp (segment.size, "dynamic") == 0)
-	crypt->total_length = grub_disk_get_size (disk) - crypt->offset;
+	crypt->total_length = (grub_disk_get_size (disk) >> (crypt->log_sector_size - disk->log_sector_size))
+			       - crypt->offset;
       else
-	crypt->total_length = grub_strtoull (segment.size, NULL, 10);
+	crypt->total_length = grub_strtoull (segment.size, NULL, 10) >> crypt->log_sector_size;
 
       ret = luks2_decrypt_key (candidate_key, disk, crypt, &keyslot,
 			       (const grub_uint8_t *) passphrase, grub_strlen (passphrase));
-- 
2.28.0


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

[PATCH v2 5/9] luks2: Improve error reporting when decrypting/verifying key

[Patrick Steinhardt]

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

While we already set up error messages in both `luks2_verify_key()` and
`luks2_decrypt_key()`, we do not ever print them. This makes it really
hard to discover why a given key actually failed to decrypt a disk.

Improve this by including the error message in the user-visible output.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 grub-core/disk/luks2.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 5f15a4d2c..26e1126b1 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -612,14 +612,16 @@ luks2_recover_key (grub_disk_t disk,
 			       (const grub_uint8_t *) passphrase, grub_strlen (passphrase));
       if (ret)
 	{
-	  grub_dprintf ("luks2", "Decryption with keyslot %"PRIuGRUB_SIZE" failed\n", i);
+	  grub_dprintf ("luks2", "Decryption with keyslot %"PRIuGRUB_SIZE" failed: %s\n",
+			i, grub_errmsg);
 	  continue;
 	}
 
       ret = luks2_verify_key (&digest, candidate_key, keyslot.key_size);
       if (ret)
 	{
-	  grub_dprintf ("luks2", "Could not open keyslot %"PRIuGRUB_SIZE"\n", i);
+	  grub_dprintf ("luks2", "Could not open keyslot %"PRIuGRUB_SIZE": %s\n",
+			i, grub_errmsg);
 	  continue;
 	}
 
-- 
2.28.0


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

[PATCH v2 6/9] cryptodisk: Unregister cryptomount command when removing module

[Patrick Steinhardt]

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

From: Glenn Washburn <development@efficientek.com>

Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Patrick Steinhardt <ps@pks.im>
---
 grub-core/disk/cryptodisk.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 1897acc4b..b2c6e9a7d 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1311,5 +1311,6 @@ GRUB_MOD_FINI (cryptodisk)
 {
   grub_disk_dev_unregister (&grub_cryptodisk_dev);
   cryptodisk_cleanup ();
+  grub_unregister_extcmd (cmd);
   grub_procfs_unregister (&luks_script);
 }
-- 
2.28.0


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

[PATCH v2 7/9] cryptodisk: Fix incorrect calculation of start sector

[Patrick Steinhardt]

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

From: Glenn Washburn <development@efficientek.com>

Here dev is a grub_cryptodisk_t and dev->offset is offset in sectors of size
native to the cryptodisk device. The sector is correctly transformed into
native grub sector size, but then added to dev->offset which is not
transformed. It would be nice if the type system would help us with this.

Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Patrick Steinhardt <ps@pks.im>
---
 grub-core/disk/cryptodisk.c | 11 ++++-------
 include/grub/disk.h         |  7 +++++++
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index b2c6e9a7d..1eea4161f 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -757,9 +757,8 @@ grub_cryptodisk_read (grub_disk_t disk, grub_disk_addr_t sector,
 		size, sector, dev->offset);
 
   err = grub_disk_read (dev->source_disk,
-			(sector << (disk->log_sector_size
-				   - GRUB_DISK_SECTOR_BITS)) + dev->offset, 0,
-			size << disk->log_sector_size, buf);
+			grub_disk_from_native_sector (disk, sector + dev->offset),
+			0, size << disk->log_sector_size, buf);
   if (err)
     {
       grub_dprintf ("cryptodisk", "grub_disk_read failed with error %d\n", err);
@@ -816,12 +815,10 @@ grub_cryptodisk_write (grub_disk_t disk, grub_disk_addr_t sector,
     }
 
   /* Since ->write was called so disk.mod is loaded but be paranoid  */
-  
+  sector = sector + dev->offset;
   if (grub_disk_write_weak)
     err = grub_disk_write_weak (dev->source_disk,
-				(sector << (disk->log_sector_size
-					    - GRUB_DISK_SECTOR_BITS))
-				+ dev->offset,
+				grub_disk_from_native_sector (disk, sector),
 				0, size << disk->log_sector_size, tmp);
   else
     err = grub_error (GRUB_ERR_BUG, "disk.mod not loaded");
diff --git a/include/grub/disk.h b/include/grub/disk.h
index 316659fee..af9f886d3 100644
--- a/include/grub/disk.h
+++ b/include/grub/disk.h
@@ -174,6 +174,13 @@ typedef struct grub_disk_memberlist *grub_disk_memberlist_t;
 /* Return value of grub_disk_get_size() in case disk size is unknown. */
 #define GRUB_DISK_SIZE_UNKNOWN	 0xffffffffffffffffULL
 
+/* Convert to grub native disk sized sector from disk sized sector */
+static inline grub_disk_addr_t
+grub_disk_from_native_sector (grub_disk_t disk, grub_disk_addr_t sector)
+{
+  return sector << (disk->log_sector_size - GRUB_DISK_SECTOR_BITS);
+}
+
 /* This is called from the memory manager.  */
 void grub_disk_cache_invalidate_all (void);
 
-- 
2.28.0


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

[PATCH v2 8/9] cryptodisk: Fix cipher IV mode 'plain64' always being set as 'plain'

[Patrick Steinhardt]

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

From: Glenn Washburn <development@efficientek.com>

Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Patrick Steinhardt <ps@pks.im>
---
 grub-core/disk/cryptodisk.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 1eea4161f..0b63b7d96 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -501,10 +501,10 @@ grub_cryptodisk_setcipher (grub_cryptodisk_t crypt, const char *ciphername, cons
 
   if (cipheriv == NULL)
       ;
-  else if (grub_memcmp (cipheriv, "plain", sizeof ("plain") - 1) == 0)
-      mode_iv = GRUB_CRYPTODISK_MODE_IV_PLAIN;
   else if (grub_memcmp (cipheriv, "plain64", sizeof ("plain64") - 1) == 0)
       mode_iv = GRUB_CRYPTODISK_MODE_IV_PLAIN64;
+  else if (grub_memcmp (cipheriv, "plain", sizeof ("plain") - 1) == 0)
+      mode_iv = GRUB_CRYPTODISK_MODE_IV_PLAIN;
   else if (grub_memcmp (cipheriv, "benbi", sizeof ("benbi") - 1) == 0)
     {
       if (cipher->cipher->blocksize & (cipher->cipher->blocksize - 1)
-- 
2.28.0


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

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

[Patrick Steinhardt]

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

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
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 | 47 +++++++++++++++++++++----------------
 grub-core/disk/luks.c       |  5 ++--
 grub-core/disk/luks2.c      |  6 ++++-
 include/grub/cryptodisk.h   |  2 +-
 4 files changed, 36 insertions(+), 24 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 0b63b7d96..0b8ac5b30 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -33,6 +33,9 @@
 
 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[] =
@@ -224,7 +227,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 sector_size,
+			   int do_encrypt)
 {
   grub_size_t i;
   gcry_err_code_t err;
@@ -237,12 +241,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 << 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;
 
       if (dev->rekey)
 	{
@@ -270,7 +275,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 << 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 +286,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 << (sector_size - CRYPT_LOG_SECTOR_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[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)
 				    & 0xFFFFFFFF);
 	  break;
 	case GRUB_CRYPTODISK_MODE_IV_BENBI:
@@ -311,10 +318,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 << 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);
 	  if (err)
 	    return err;
 	  break;
@@ -322,10 +329,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 << 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);
 	  if (err)
 	    return err;
 	  break;
@@ -337,7 +344,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 << sector_size);
 		 j += dev->cipher->cipher->blocksize)
 	      {
 		grub_crypto_xor (data + i + j, data + i + j, iv,
@@ -368,11 +375,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 << sector_size));
 	    else
 	      err = grub_crypto_ecb_decrypt (dev->cipher, data + i, 
 					     data + i,
-					     (1U << dev->log_sector_size));
+					     (1U << sector_size));
 	    if (err)
 	      return err;
 	    lrw_xor (&sec, dev, data + i);
@@ -381,10 +388,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 << sector_size));
 	  else
 	    err = grub_crypto_ecb_decrypt (dev->cipher, data + i, data + i,
-					   (1U << dev->log_sector_size));
+					   (1U << sector_size));
 	  if (err)
 	    return err;
 	  break;
@@ -399,9 +406,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 sector_size)
 {
-  return grub_cryptodisk_endecrypt (dev, data, len, sector, 0);
+  return grub_cryptodisk_endecrypt (dev, data, len, sector, sector_size, 0);
 }
 
 grub_err_t
@@ -766,7 +773,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 +814,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..b30c4551e 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -30,6 +30,7 @@
 GRUB_MOD_LICENSE ("GPLv3+");
 
 #define MAX_PASSPHRASE 256
+#define LOG_SECTOR_SIZE 9
 
 #define LUKS_KEY_ENABLED  0x00AC71F3
 
@@ -124,7 +125,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 = 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 +248,7 @@ 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, 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..1f5f9766a 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -492,7 +492,11 @@ luks2_decrypt_key (grub_uint8_t *out_key,
       goto err;
     }
 
-  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
+   */
+  gcry_ret = grub_cryptodisk_decrypt (crypt, split_key, k->area.size, 0, 9);
   if (gcry_ret)
     {
       ret = grub_crypto_gcry_error (gcry_ret);
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index e1b21e785..06653a622 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -139,7 +139,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 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 v2 0/9] Cryptodisk fixes for v2.06

[Glenn Washburn]

On Wed, 26 Aug 2020 10:13:18 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> I didn't yet get your test series to work, Glenn. I'll try again on
> another day as I'm not on top of things today. Meanwhile, could you
> please give it a go with this updated patch series?

I've tested with this patch series and all tests pass.

Which part isn't working for the tests?  The patchset (on the
mailinglist) has expected failures for all LUKS2 tests because my LUKS2
keyfile patch is needed. So the tests should be able to verify that
LUKS1 tests are passing before and after this patchset.

Here's the branch I created that has your patch series, the v5 crypto
keyfile and detached header patch series, my cryptomount test patches,
my luks2 keyfile patch, and patches turning on tests now expecting to
pass in that order applied to master.

https://github.com/crass/grub/tree/cryptodisk-fixes-for-v2.06.v2+cryptodisk-v5+cryptodisk-tests

Here is a command sequence that should show the cryptomount test as
passing.  You can look at some more details of the testing in the test
log file grub_cmd_cryptomount.log.  Once you see the tests passing, you
can remove some of the commits in the patch series and verify that the
test fails and that the appropriate subtest fails by checking the log
file.

-------------------------------------------
git clone https://github.com/crass/grub.git
cd grub
git checkout
cryptodisk-fixes-for-v2.06.v2+cryptodisk-v5+cryptodisk-tests
./bootstrap && ./configure && make -j8
sudo env TESTS=grub_cmd_cryptomount make -e check VERBOSE=yes SUBDIRS=
-------------------------------------------

Glenn

[PATCH] cryptodisk: Incorrect calculation of sector in grub_cryptodisk_read/write.

[Glenn Washburn]

Here dev is a grub_cryptodisk_t and dev->offset is offset in sectors of size
native to the cryptodisk device. The function grub_disk_read expects that
the given start sector is in the native grub sector size, so dev->offset
must be converted as well. Create function grub_disk_from_native_sector to
convert disk sectors to grub sectors.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/cryptodisk.c | 11 ++++-------
 include/grub/disk.h         |  7 +++++++
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index b87925ad3..38bd49835 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -762,9 +762,8 @@ grub_cryptodisk_read (grub_disk_t disk, grub_disk_addr_t sector,
 		size, sector, dev->offset);
 
   err = grub_disk_read (dev->source_disk,
-			(sector << (disk->log_sector_size
-				   - GRUB_DISK_SECTOR_BITS)) + dev->offset, 0,
-			size << disk->log_sector_size, buf);
+			grub_disk_from_native_sector (disk, sector + dev->offset),
+			0, size << disk->log_sector_size, buf);
   if (err)
     {
       grub_dprintf ("cryptodisk", "grub_disk_read failed with error %d\n", err);
@@ -821,12 +820,10 @@ grub_cryptodisk_write (grub_disk_t disk, grub_disk_addr_t sector,
     }
 
   /* Since ->write was called so disk.mod is loaded but be paranoid  */
-  
+  sector = sector + dev->offset;
   if (grub_disk_write_weak)
     err = grub_disk_write_weak (dev->source_disk,
-				(sector << (disk->log_sector_size
-					    - GRUB_DISK_SECTOR_BITS))
-				+ dev->offset,
+				grub_disk_from_native_sector (disk, sector),
 				0, size << disk->log_sector_size, tmp);
   else
     err = grub_error (GRUB_ERR_BUG, "disk.mod not loaded");
diff --git a/include/grub/disk.h b/include/grub/disk.h
index 316659fee..af9f886d3 100644
--- a/include/grub/disk.h
+++ b/include/grub/disk.h
@@ -174,6 +174,13 @@ typedef struct grub_disk_memberlist *grub_disk_memberlist_t;
 /* Return value of grub_disk_get_size() in case disk size is unknown. */
 #define GRUB_DISK_SIZE_UNKNOWN	 0xffffffffffffffffULL
 
+/* Convert to grub native disk sized sector from disk sized sector */
+static inline grub_disk_addr_t
+grub_disk_from_native_sector (grub_disk_t disk, grub_disk_addr_t sector)
+{
+  return sector << (disk->log_sector_size - GRUB_DISK_SECTOR_BITS);
+}
+
 /* This is called from the memory manager.  */
 void grub_disk_cache_invalidate_all (void);
 
-- 
2.27.0

Re: [PATCH] cryptodisk: Incorrect calculation of sector in grub_cryptodisk_read/write.

[Glenn Washburn]

On Wed, 26 Aug 2020 18:44:35 -0500
Glenn Washburn <development@efficientek.com> wrote:

> Here dev is a grub_cryptodisk_t and dev->offset is offset in sectors
> of size native to the cryptodisk device. The function grub_disk_read
> expects that the given start sector is in the native grub sector
> size, so dev->offset must be converted as well. Create function
> grub_disk_from_native_sector to convert disk sectors to grub sectors.

Upon reviewing my previous patch, I realized that the updated commit
message had gotten lost.  So this only changes the commit message and
not the diff itself.  I don't think its imperative to have this updated
commit message, but I do think it more clearly and accurately describes
the patch.

Glenn

Re: [PATCH] cryptodisk: Incorrect calculation of sector in grub_cryptodisk_read/write.

[Patrick Steinhardt]

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

On Wed, Aug 26, 2020 at 06:50:29PM -0500, Glenn Washburn wrote:
> On Wed, 26 Aug 2020 18:44:35 -0500
> Glenn Washburn <development@efficientek.com> wrote:
> 
> > Here dev is a grub_cryptodisk_t and dev->offset is offset in sectors
> > of size native to the cryptodisk device. The function grub_disk_read
> > expects that the given start sector is in the native grub sector
> > size, so dev->offset must be converted as well. Create function
> > grub_disk_from_native_sector to convert disk sectors to grub sectors.
> 
> Upon reviewing my previous patch, I realized that the updated commit
> message had gotten lost.  So this only changes the commit message and
> not the diff itself.  I don't think its imperative to have this updated
> commit message, but I do think it more clearly and accurately describes
> the patch.
> 
> Glenn

I did already update the commit's title in v2 of my patch series, but
not its body. I'll pick your improvement if there's a need for v3 of my
patch series, thanks!

Patrick

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

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

[Patrick Steinhardt]

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

On Wed, Aug 26, 2020 at 05:16:57PM -0500, Glenn Washburn wrote:
> On Wed, 26 Aug 2020 10:13:18 +0200
> Patrick Steinhardt <ps@pks.im> wrote:
> 
> > I didn't yet get your test series to work, Glenn. I'll try again on
> > another day as I'm not on top of things today. Meanwhile, could you
> > please give it a go with this updated patch series?
> 
> I've tested with this patch series and all tests pass.

Thanks for letting me know.

> Which part isn't working for the tests?  The patchset (on the
> mailinglist) has expected failures for all LUKS2 tests because my LUKS2
> keyfile patch is needed. So the tests should be able to verify that
> LUKS1 tests are passing before and after this patchset.

As I said, it's mostly me not having the brain capacity right now. Next
to the keyfile dependency I'd also have to change MAC policies on my
system or set up a VM for it, but I don't feel too well this week and
thus need to defer this.

> Here's the branch I created that has your patch series, the v5 crypto
> keyfile and detached header patch series, my cryptomount test patches,
> my luks2 keyfile patch, and patches turning on tests now expecting to
> pass in that order applied to master.
> 
> https://github.com/crass/grub/tree/cryptodisk-fixes-for-v2.06.v2+cryptodisk-v5+cryptodisk-tests
> 
> Here is a command sequence that should show the cryptomount test as
> passing.  You can look at some more details of the testing in the test
> log file grub_cmd_cryptomount.log.  Once you see the tests passing, you
> can remove some of the commits in the patch series and verify that the
> test fails and that the appropriate subtest fails by checking the log
> file.
> 
> -------------------------------------------
> git clone https://github.com/crass/grub.git
> cd grub
> git checkout
> cryptodisk-fixes-for-v2.06.v2+cryptodisk-v5+cryptodisk-tests
> ./bootstrap && ./configure && make -j8
> sudo env TESTS=grub_cmd_cryptomount make -e check VERBOSE=yes SUBDIRS=
> -------------------------------------------

Thanks a lot for these!

Patrick

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

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

[Glenn Washburn]

I just noticed that I have a couple minor non-functional changes that I
think will make this patch a little better.  I had been planning on
updating my original patch, but since you're picking this up, this is a
better place to update.  Let me know if you'd like the updated version
of the patch instead of my inline comments below.

On Wed, 26 Aug 2020 10:14:02 +0200
Patrick Steinhardt <ps@pks.im> 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 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 | 47
> +++++++++++++++++++++---------------- grub-core/disk/luks.c       |
> 5 ++-- grub-core/disk/luks2.c      |  6 ++++-
>  include/grub/cryptodisk.h   |  2 +-
>  4 files changed, 36 insertions(+), 24 deletions(-)
> 
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 0b63b7d96..0b8ac5b30 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -33,6 +33,9 @@
>  
>  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[] =
> @@ -224,7 +227,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
> sector_size,
> +			   int do_encrypt)

I've changed the sector_size parameter to log_sector_size, which is
what it really is.  I think this is really important to change because
there's already enough confusingly named identifiers.

>  {
>    grub_size_t i;
>    gcry_err_code_t err;
> @@ -237,12 +241,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 << 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;
>  
>        if (dev->rekey)
>  	{
> @@ -270,7 +275,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 << 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 +286,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 << (sector_size - CRYPT_LOG_SECTOR_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[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)
>  				    & 0xFFFFFFFF);
>  	  break;
>  	case GRUB_CRYPTODISK_MODE_IV_BENBI:
> @@ -311,10 +318,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 << 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);
>  	  if (err)
>  	    return err;
>  	  break;
> @@ -322,10 +329,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 << 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);
>  	  if (err)
>  	    return err;
>  	  break;
> @@ -337,7 +344,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 << sector_size);
>  		 j += dev->cipher->cipher->blocksize)
>  	      {
>  		grub_crypto_xor (data + i + j, data + i + j, iv,
> @@ -368,11 +375,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 << sector_size));
>  	    else
>  	      err = grub_crypto_ecb_decrypt (dev->cipher, data + i, 
>  					     data + i,
> -					     (1U <<
> dev->log_sector_size));
> +					     (1U << sector_size));
>  	    if (err)
>  	      return err;
>  	    lrw_xor (&sec, dev, data + i);
> @@ -381,10 +388,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 << sector_size));
>  	  else
>  	    err = grub_crypto_ecb_decrypt (dev->cipher, data + i,
> data + i,
> -					   (1U <<
> dev->log_sector_size));
> +					   (1U << sector_size));
>  	  if (err)
>  	    return err;
>  	  break;
> @@ -399,9 +406,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
> sector_size) {
> -  return grub_cryptodisk_endecrypt (dev, data, len, sector, 0);
> +  return grub_cryptodisk_endecrypt (dev, data, len, sector,
> sector_size, 0); }
>  
>  grub_err_t
> @@ -766,7 +773,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 +814,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..b30c4551e 100644
> --- a/grub-core/disk/luks.c
> +++ b/grub-core/disk/luks.c
> @@ -30,6 +30,7 @@
>  GRUB_MOD_LICENSE ("GPLv3+");
>  
>  #define MAX_PASSPHRASE 256
> +#define LOG_SECTOR_SIZE 9
>  
>  #define LUKS_KEY_ENABLED  0x00AC71F3
>  
> @@ -124,7 +125,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 = 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 +248,7 @@ 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,
> 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..1f5f9766a 100644
> --- a/grub-core/disk/luks2.c
> +++ b/grub-core/disk/luks2.c
> @@ -492,7 +492,11 @@ luks2_decrypt_key (grub_uint8_t *out_key,
>        goto err;
>      }
>  
> -  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
> +   */
> +  gcry_ret = grub_cryptodisk_decrypt (crypt, split_key,
> k->area.size, 0, 9); if (gcry_ret)

I've reworded the comment to be slightly more readable:
  The key slots area is always encrypted in 512-byte sectors,
  regardless of encrypted data sector size.

>      {
>        ret = grub_crypto_gcry_error (gcry_ret);
> diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> index e1b21e785..06653a622 100644
> --- a/include/grub/cryptodisk.h
> +++ b/include/grub/cryptodisk.h
> @@ -139,7 +139,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
> sector_size); grub_err_t
>  grub_cryptodisk_insert (grub_cryptodisk_t newdev, const char *name,
>  			grub_disk_t source);

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

[Patrick Steinhardt]

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

On Mon, Aug 31, 2020 at 01:43:36PM -0500, Glenn Washburn wrote:
> I just noticed that I have a couple minor non-functional changes that I
> think will make this patch a little better.  I had been planning on
> updating my original patch, but since you're picking this up, this is a
> better place to update.  Let me know if you'd like the updated version
> of the patch instead of my inline comments below.

Yeah, I guess it's easier if you just send the updated patch in response
to my patch series. I'll make sure to pick it up then. Thanks in
advance!

Patrick

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

[PATCH] cryptodisk: Properly handle non-512 byte sized sectors.

[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
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>
---
 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);
 	  /* 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));
-	  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 +315,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 +326,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 +341,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 +372,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 +385,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 +403,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 +770,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 +811,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..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);
   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);
-- 
2.27.0

Re: [PATCH] cryptodisk: Properly handle non-512 byte sized sectors.

[Glenn Washburn]

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.

Glenn

[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 --]

[PATCH v3 1/9] json: Remove invalid typedef redefinition

[Patrick Steinhardt]

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

The C standard does not allow for typedef redefinitions, even if they
map to the same underlying type. In order to avoid including the
"jsmn.h" in "json.h" and thus exposing jsmn's internals, we have exactly
such a forward-declaring typedef in "json.h". If enforcing the GNU99 C
standard, clang may generate a warning about this non-standard
construct.

Fix the issue by using a simple `struct jsmntok` forward declaration
instead of using a typedef.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Tested-by: Chuck Tuffli <chuck@freebsd.org>
---
 grub-core/lib/json/json.h | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/grub-core/lib/json/json.h b/grub-core/lib/json/json.h
index 01614f6df..657902cfc 100644
--- a/grub-core/lib/json/json.h
+++ b/grub-core/lib/json/json.h
@@ -36,13 +36,14 @@ enum grub_json_type
 };
 typedef enum grub_json_type grub_json_type_t;
 
-typedef struct jsmntok jsmntok_t;
+/* Forward-declaration to avoid including "jsmn.h" */
+struct jsmntok;
 
 struct grub_json
 {
-  jsmntok_t   *tokens;
-  char	      *string;
-  grub_size_t idx;
+  struct jsmntok *tokens;
+  char		 *string;
+  grub_size_t	 idx;
 };
 typedef struct grub_json grub_json_t;
 
-- 
2.28.0


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

[PATCH v3 2/9] luks: Fix out-of-bounds copy of UUID

[Patrick Steinhardt]

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

When configuring a LUKS disk, we copy over the UUID from the LUKS header
into the new `grub_cryptodisk_t` structure via `grub_memcpy ()`. As size
we mistakenly use the size of the `grub_cryptodisk_t` UUID field, which
is guaranteed to be strictly bigger than the LUKS UUID field we're
copying. As a result, the copy always goes out-of-bounds and copies some
garbage from other surrounding fields. During runtime, this isn't
noticed due to the fact that we always NUL-terminate the UUID and thus
never hit the trailing garbage.

Fix the issue by using the size of the local stripped UUID field.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 grub-core/disk/luks.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index 6ae162601..59702067a 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -95,6 +95,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
       || grub_be_to_cpu16 (header.version) != 1)
     return NULL;
 
+  grub_memset (uuid, 0, sizeof (uuid));
   optr = uuid;
   for (iptr = header.uuid; iptr < &header.uuid[ARRAY_SIZE (header.uuid)];
        iptr++)
@@ -125,7 +126,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
   newdev->source_disk = NULL;
   newdev->log_sector_size = 9;
   newdev->total_length = grub_disk_get_size (disk) - newdev->offset;
-  grub_memcpy (newdev->uuid, uuid, sizeof (newdev->uuid));
+  grub_memcpy (newdev->uuid, uuid, sizeof (uuid));
   newdev->modname = "luks";
 
   /* Configure the hash used for the AF splitter and HMAC.  */
-- 
2.28.0


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

[PATCH v3 3/9] luks2: Fix use of incorrect index and some error messages

[Patrick Steinhardt]

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

From: Glenn Washburn <development@efficientek.com>

Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Patrick Steinhardt <ps@pks.im>
---
 grub-core/disk/luks2.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index e3ff7c83d..c4c6ac90c 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -255,54 +255,55 @@ luks2_parse_digest (grub_luks2_digest_t *out, const grub_json_t *digest)
 
 static grub_err_t
 luks2_get_keyslot (grub_luks2_keyslot_t *k, grub_luks2_digest_t *d, grub_luks2_segment_t *s,
-		   const grub_json_t *root, grub_size_t i)
+		   const grub_json_t *root, grub_size_t keyslot_idx)
 {
   grub_json_t keyslots, keyslot, digests, digest, segments, segment;
-  grub_size_t j, size;
-  grub_uint64_t idx;
+  grub_size_t i, size;
+  grub_uint64_t keyslot_key, digest_key, segment_key;
 
   /* Get nth keyslot */
   if (grub_json_getvalue (&keyslots, root, "keyslots") ||
-      grub_json_getchild (&keyslot, &keyslots, i) ||
-      grub_json_getuint64 (&idx, &keyslot, NULL) ||
+      grub_json_getchild (&keyslot, &keyslots, keyslot_idx) ||
+      grub_json_getuint64 (&keyslot_key, &keyslot, NULL) ||
       grub_json_getchild (&keyslot, &keyslot, 0) ||
       luks2_parse_keyslot (k, &keyslot))
-    return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse keyslot %"PRIuGRUB_SIZE, i);
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse keyslot index %"PRIuGRUB_SIZE, keyslot_idx);
 
   /* Get digest that matches the keyslot. */
   if (grub_json_getvalue (&digests, root, "digests") ||
       grub_json_getsize (&size, &digests))
     return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not get digests");
-  for (j = 0; j < size; j++)
+  for (i = 0; i < size; i++)
     {
       if (grub_json_getchild (&digest, &digests, i) ||
+	  grub_json_getuint64 (&digest_key, &digest, NULL) ||
           grub_json_getchild (&digest, &digest, 0) ||
           luks2_parse_digest (d, &digest))
-	return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse digest %"PRIuGRUB_SIZE, i);
+	return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse digest index %"PRIuGRUB_SIZE, i);
 
-      if ((d->keyslots & (1 << idx)))
+      if ((d->keyslots & (1 << keyslot_key)))
 	break;
     }
-  if (j == size)
-      return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for keyslot %"PRIuGRUB_SIZE);
+  if (i == size)
+      return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for keyslot \"%"PRIuGRUB_UINT64_T"\"", keyslot_key);
 
   /* Get segment that matches the digest. */
   if (grub_json_getvalue (&segments, root, "segments") ||
       grub_json_getsize (&size, &segments))
     return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not get segments");
-  for (j = 0; j < size; j++)
+  for (i = 0; i < size; i++)
     {
       if (grub_json_getchild (&segment, &segments, i) ||
-	  grub_json_getuint64 (&idx, &segment, NULL) ||
+	  grub_json_getuint64 (&segment_key, &segment, NULL) ||
 	  grub_json_getchild (&segment, &segment, 0) ||
           luks2_parse_segment (s, &segment))
-	return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse segment %"PRIuGRUB_SIZE, i);
+	return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse segment index %"PRIuGRUB_SIZE, i);
 
-      if ((d->segments & (1 << idx)))
+      if ((d->segments & (1 << segment_key)))
 	break;
     }
-  if (j == size)
-    return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No segment for digest %"PRIuGRUB_SIZE);
+  if (i == size)
+    return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No segment for digest \"%"PRIuGRUB_UINT64_T"\"", digest_key);
 
   return GRUB_ERR_NONE;
 }
-- 
2.28.0


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

[PATCH v3 4/9] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors

[Patrick Steinhardt]

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

From: Glenn Washburn <development@efficientek.com>

The total_length field is named confusingly because length usually refers to
bytes, whereas in this case its really the total number of sectors on the
device. Also counter-intuitively, grub_disk_get_size returns the total
number of device native sectors sectors. We need to convert the sectors from
the size of the underlying device to the cryptodisk sector size. And
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 | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index c4c6ac90c..5f15a4d2c 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -417,7 +417,7 @@ luks2_decrypt_key (grub_uint8_t *out_key,
   grub_uint8_t salt[GRUB_CRYPTODISK_MAX_KEYLEN];
   grub_uint8_t *split_key = NULL;
   grub_size_t saltlen = sizeof (salt);
-  char cipher[32], *p;;
+  char cipher[32], *p;
   const gcry_md_spec_t *hash;
   gcry_err_code_t gcry_ret;
   grub_err_t ret;
@@ -603,9 +603,10 @@ luks2_recover_key (grub_disk_t disk,
       crypt->log_sector_size = sizeof (unsigned int) * 8
 		- __builtin_clz ((unsigned int) segment.sector_size) - 1;
       if (grub_strcmp (segment.size, "dynamic") == 0)
-	crypt->total_length = grub_disk_get_size (disk) - crypt->offset;
+	crypt->total_length = (grub_disk_get_size (disk) >> (crypt->log_sector_size - disk->log_sector_size))
+			       - crypt->offset;
       else
-	crypt->total_length = grub_strtoull (segment.size, NULL, 10);
+	crypt->total_length = grub_strtoull (segment.size, NULL, 10) >> crypt->log_sector_size;
 
       ret = luks2_decrypt_key (candidate_key, disk, crypt, &keyslot,
 			       (const grub_uint8_t *) passphrase, grub_strlen (passphrase));
-- 
2.28.0


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

[PATCH v3 5/9] luks2: Improve error reporting when decrypting/verifying key

[Patrick Steinhardt]

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

While we already set up error messages in both `luks2_verify_key()` and
`luks2_decrypt_key()`, we do not ever print them. This makes it really
hard to discover why a given key actually failed to decrypt a disk.

Improve this by including the error message in the user-visible output.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 grub-core/disk/luks2.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 5f15a4d2c..26e1126b1 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -612,14 +612,16 @@ luks2_recover_key (grub_disk_t disk,
 			       (const grub_uint8_t *) passphrase, grub_strlen (passphrase));
       if (ret)
 	{
-	  grub_dprintf ("luks2", "Decryption with keyslot %"PRIuGRUB_SIZE" failed\n", i);
+	  grub_dprintf ("luks2", "Decryption with keyslot %"PRIuGRUB_SIZE" failed: %s\n",
+			i, grub_errmsg);
 	  continue;
 	}
 
       ret = luks2_verify_key (&digest, candidate_key, keyslot.key_size);
       if (ret)
 	{
-	  grub_dprintf ("luks2", "Could not open keyslot %"PRIuGRUB_SIZE"\n", i);
+	  grub_dprintf ("luks2", "Could not open keyslot %"PRIuGRUB_SIZE": %s\n",
+			i, grub_errmsg);
 	  continue;
 	}
 
-- 
2.28.0


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

[PATCH v3 6/9] cryptodisk: Unregister cryptomount command when removing module

[Patrick Steinhardt]

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

From: Glenn Washburn <development@efficientek.com>

Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Patrick Steinhardt <ps@pks.im>
---
 grub-core/disk/cryptodisk.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 1897acc4b..b2c6e9a7d 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1311,5 +1311,6 @@ GRUB_MOD_FINI (cryptodisk)
 {
   grub_disk_dev_unregister (&grub_cryptodisk_dev);
   cryptodisk_cleanup ();
+  grub_unregister_extcmd (cmd);
   grub_procfs_unregister (&luks_script);
 }
-- 
2.28.0


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

[PATCH v3 7/9] cryptodisk: Fix incorrect calculation of start sector

[Patrick Steinhardt]

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

From: Glenn Washburn <development@efficientek.com>

Here dev is a grub_cryptodisk_t and dev->offset is offset in sectors of size
native to the cryptodisk device. The sector is correctly transformed into
native grub sector size, but then added to dev->offset which is not
transformed. It would be nice if the type system would help us with this.

Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Patrick Steinhardt <ps@pks.im>
---
 grub-core/disk/cryptodisk.c | 11 ++++-------
 include/grub/disk.h         |  7 +++++++
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index b2c6e9a7d..1eea4161f 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -757,9 +757,8 @@ grub_cryptodisk_read (grub_disk_t disk, grub_disk_addr_t sector,
 		size, sector, dev->offset);
 
   err = grub_disk_read (dev->source_disk,
-			(sector << (disk->log_sector_size
-				   - GRUB_DISK_SECTOR_BITS)) + dev->offset, 0,
-			size << disk->log_sector_size, buf);
+			grub_disk_from_native_sector (disk, sector + dev->offset),
+			0, size << disk->log_sector_size, buf);
   if (err)
     {
       grub_dprintf ("cryptodisk", "grub_disk_read failed with error %d\n", err);
@@ -816,12 +815,10 @@ grub_cryptodisk_write (grub_disk_t disk, grub_disk_addr_t sector,
     }
 
   /* Since ->write was called so disk.mod is loaded but be paranoid  */
-  
+  sector = sector + dev->offset;
   if (grub_disk_write_weak)
     err = grub_disk_write_weak (dev->source_disk,
-				(sector << (disk->log_sector_size
-					    - GRUB_DISK_SECTOR_BITS))
-				+ dev->offset,
+				grub_disk_from_native_sector (disk, sector),
 				0, size << disk->log_sector_size, tmp);
   else
     err = grub_error (GRUB_ERR_BUG, "disk.mod not loaded");
diff --git a/include/grub/disk.h b/include/grub/disk.h
index 316659fee..af9f886d3 100644
--- a/include/grub/disk.h
+++ b/include/grub/disk.h
@@ -174,6 +174,13 @@ typedef struct grub_disk_memberlist *grub_disk_memberlist_t;
 /* Return value of grub_disk_get_size() in case disk size is unknown. */
 #define GRUB_DISK_SIZE_UNKNOWN	 0xffffffffffffffffULL
 
+/* Convert to grub native disk sized sector from disk sized sector */
+static inline grub_disk_addr_t
+grub_disk_from_native_sector (grub_disk_t disk, grub_disk_addr_t sector)
+{
+  return sector << (disk->log_sector_size - GRUB_DISK_SECTOR_BITS);
+}
+
 /* This is called from the memory manager.  */
 void grub_disk_cache_invalidate_all (void);
 
-- 
2.28.0


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

[PATCH v3 8/9] cryptodisk: Fix cipher IV mode 'plain64' always being set as 'plain'

[Patrick Steinhardt]

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

From: Glenn Washburn <development@efficientek.com>

Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Patrick Steinhardt <ps@pks.im>
---
 grub-core/disk/cryptodisk.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 1eea4161f..0b63b7d96 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -501,10 +501,10 @@ grub_cryptodisk_setcipher (grub_cryptodisk_t crypt, const char *ciphername, cons
 
   if (cipheriv == NULL)
       ;
-  else if (grub_memcmp (cipheriv, "plain", sizeof ("plain") - 1) == 0)
-      mode_iv = GRUB_CRYPTODISK_MODE_IV_PLAIN;
   else if (grub_memcmp (cipheriv, "plain64", sizeof ("plain64") - 1) == 0)
       mode_iv = GRUB_CRYPTODISK_MODE_IV_PLAIN64;
+  else if (grub_memcmp (cipheriv, "plain", sizeof ("plain") - 1) == 0)
+      mode_iv = GRUB_CRYPTODISK_MODE_IV_PLAIN;
   else if (grub_memcmp (cipheriv, "benbi", sizeof ("benbi") - 1) == 0)
     {
       if (cipher->cipher->blocksize & (cipher->cipher->blocksize - 1)
-- 
2.28.0


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

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

[Patrick Steinhardt]

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

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
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);
 	  /* 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));
-	  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 +315,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 +326,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 +341,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 +372,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 +385,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 +403,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 +770,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 +811,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..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);
   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);
-- 
2.28.0


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

Re: [PATCH] cryptodisk: Properly handle non-512 byte sized sectors.

[Patrick Steinhardt]

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

On Tue, Sep 01, 2020 at 07:01:24PM -0500, Glenn Washburn wrote:
> 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.
> 
> Glenn

Now picked up and sent as part of v3 of the patch series, thanks a lot!

Patrick

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

Re: [PATCH v3 3/9] luks2: Fix use of incorrect index and some error messages

[Daniel Kiper]

On Mon, Sep 07, 2020 at 05:27:41PM +0200, Patrick Steinhardt wrote:
> From: Glenn Washburn <development@efficientek.com>

It seems to me this patch should be split into two and and begs for
commit message improvement. In general it would be nice to know why
we need these fixes.

Daniel

Re: [PATCH v3 4/9] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors

[Daniel Kiper]

On Mon, Sep 07, 2020 at 05:27:46PM +0200, Patrick Steinhardt wrote:
> From: Glenn Washburn <development@efficientek.com>
>
> The total_length field is named confusingly because length usually refers to
> bytes, whereas in this case its really the total number of sectors on the
> device. Also counter-intuitively, grub_disk_get_size returns the total

Could we change total_length name? Or should it stay as is because this
name is used in other implementations too?

> number of device native sectors sectors. We need to convert the sectors from
> the size of the underlying device to the cryptodisk sector size. And
> 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 | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> index c4c6ac90c..5f15a4d2c 100644
> --- a/grub-core/disk/luks2.c
> +++ b/grub-core/disk/luks2.c
> @@ -417,7 +417,7 @@ luks2_decrypt_key (grub_uint8_t *out_key,
>    grub_uint8_t salt[GRUB_CRYPTODISK_MAX_KEYLEN];
>    grub_uint8_t *split_key = NULL;
>    grub_size_t saltlen = sizeof (salt);
> -  char cipher[32], *p;;
> +  char cipher[32], *p;

I am OK with changes like that but they should be mentioned shortly in
the commit message.

>    const gcry_md_spec_t *hash;
>    gcry_err_code_t gcry_ret;
>    grub_err_t ret;
> @@ -603,9 +603,10 @@ luks2_recover_key (grub_disk_t disk,
>        crypt->log_sector_size = sizeof (unsigned int) * 8
>  		- __builtin_clz ((unsigned int) segment.sector_size) - 1;
>        if (grub_strcmp (segment.size, "dynamic") == 0)
> -	crypt->total_length = grub_disk_get_size (disk) - crypt->offset;
> +	crypt->total_length = (grub_disk_get_size (disk) >> (crypt->log_sector_size - disk->log_sector_size))
> +			       - crypt->offset;
>        else
> -	crypt->total_length = grub_strtoull (segment.size, NULL, 10);
> +	crypt->total_length = grub_strtoull (segment.size, NULL, 10) >> crypt->log_sector_size;

I do not like that you ignore grub_strtoull() errors. Additionally, what
will happen if segment.size is smaller than LUKS2 sector size? Should
not you round segment.size up to the nearest multiple of LUKS2 sector
size first? I think the same applies to the earlier change too.

Daniel

Re: [PATCH v3 6/9] cryptodisk: Unregister cryptomount command when removing module

[Daniel Kiper]

On Mon, Sep 07, 2020 at 05:27:55PM +0200, Patrick Steinhardt wrote:
> From: Glenn Washburn <development@efficientek.com>
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> Reviewed-by: Patrick Steinhardt <ps@pks.im>
> ---
>  grub-core/disk/cryptodisk.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 1897acc4b..b2c6e9a7d 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -1311,5 +1311,6 @@ GRUB_MOD_FINI (cryptodisk)
>  {
>    grub_disk_dev_unregister (&grub_cryptodisk_dev);
>    cryptodisk_cleanup ();

I am OK with this patch but I realized that cryptodisk_cleanup() body is
commented out, So, could you add another patch which drops
cryptodisk_cleanup() entirely from the GRUB code? Or make it work as it
supposed to work. Maybe the latter is better...

Daniel

Re: [PATCH v3 8/9] cryptodisk: Fix cipher IV mode 'plain64' always being set as 'plain'

[Daniel Kiper]

On Mon, Sep 07, 2020 at 05:28:04PM +0200, Patrick Steinhardt wrote:
> From: Glenn Washburn <development@efficientek.com>

Could explain here why this patch is needed? I know why but I would like
to have this explicitly stated in the commit message.

Daniel

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

[Daniel Kiper]

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/?

> 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?

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

> -	  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/?

>    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

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 --]

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

Re: [PATCH v3 4/9] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors

[Glenn Washburn]

Sep 8, 2020 7:21:31 AM Daniel Kiper <daniel.kiper@oracle.com>:

> On Mon, Sep 07, 2020 at 05:27:46PM +0200, Patrick Steinhardt wrote:
>> From: Glenn Washburn <development@efficientek.com>
>>
>> The total_length field is named confusingly because length usually refers to
>> bytes, whereas in this case its really the total number of sectors on the
>> device. Also counter-intuitively, grub_disk_get_size returns the total
>
> Could we change total_length name? Or should it stay as is because this
> name is used in other implementations too?

I sent a patch which renamed total_length to total_sectors. I believe Patrick chose not to include it because I did not fix a bug in the code and this patch series was only patches he thought essential to be included in the next release. I'll include that patch again in a follow up patch series.

>> number of device native sectors sectors. We need to convert the sectors from
>> the size of the underlying device to the cryptodisk sector size. And
>> 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 | 7 ++++---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
>> index c4c6ac90c..5f15a4d2c 100644
>> --- a/grub-core/disk/luks2.c
>> +++ b/grub-core/disk/luks2.c
>> @@ -417,7 +417,7 @@ luks2_decrypt_key (grub_uint8_t *out_key,
>> grub_uint8_t salt[GRUB_CRYPTODISK_MAX_KEYLEN];
>> grub_uint8_t *split_key = NULL;
>> grub_size_t saltlen = sizeof (salt);
>> -  char cipher[32], *p;;
>> +  char cipher[32], *p;
>
> I am OK with changes like that but they should be mentioned shortly in
> the commit message.

Noted, I'll put update the commit message.

>> const gcry_md_spec_t *hash;
>> gcry_err_code_t gcry_ret;
>> grub_err_t ret;
>> @@ -603,9 +603,10 @@ luks2_recover_key (grub_disk_t disk,
>> crypt->log_sector_size = sizeof (unsigned int) * 8
>> - __builtin_clz ((unsigned int) segment.sector_size) - 1;
>> if (grub_strcmp (segment.size, "dynamic") == 0)
>> - crypt->total_length = grub_disk_get_size (disk) - crypt->offset;
>> + crypt->total_length = (grub_disk_get_size (disk) >> (crypt->log_sector_size - disk->log_sector_size))
>> +            - crypt->offset;
>> else
>> - crypt->total_length = grub_strtoull (segment.size, NULL, 10);
>> + crypt->total_length = grub_strtoull (segment.size, NULL, 10) >> crypt->log_sector_size;
>
> I do not like that you ignore grub_strtoull() errors. Additionally, what
> will happen if segment.size is smaller than LUKS2 sector size? Should
> not you round segment.size up to the nearest multiple of LUKS2 sector
> size first? I think the same applies to the earlier change too.

Again, I was making a minimal set of changes for this fix. Your comments about grub_strtoull, while valid, don't apply to this patch and should be addressed in a new patch.

Your concern about rounding segment.size up, is also valid and pertinent to this patch, I'll update that in a following patch series. This may get more complicated if the last partial sector is at the end of the disk.

> Daniel
>

Re: [PATCH v3 3/9] luks2: Fix use of incorrect index and some error messages

[Glenn Washburn]

Sep 8, 2020 6:58:48 AM Daniel Kiper <daniel.kiper@oracle.com>:

> On Mon, Sep 07, 2020 at 05:27:41PM +0200, Patrick Steinhardt wrote:
>> From: Glenn Washburn <development@efficientek.com>
>
> It seems to me this patch should be split into two and and begs for
> commit message improvement. In general it would be nice to know why
> we need these fixes.

This patch is doing two things, fixing a bug and making the code more understandable. My original patch just fixed the bug, then Patrick suggested making it more readable. I can split it into two patches.

The bug it's fixing is that the keyslot index variable was being used to index the digests and segments, whereas the digest and segment index variables should be used to index their respective json arrays.

Glenn

Re: [PATCH v3 6/9] cryptodisk: Unregister cryptomount command when removing module

[Glenn Washburn]

Sep 8, 2020 7:28:13 AM Daniel Kiper <daniel.kiper@oracle.com>:

> On Mon, Sep 07, 2020 at 05:27:55PM +0200, Patrick Steinhardt wrote:
>> From: Glenn Washburn <development@efficientek.com>
>>
>> Signed-off-by: Glenn Washburn <development@efficientek.com>
>> Reviewed-by: Patrick Steinhardt <ps@pks.im>
>> ---
>> grub-core/disk/cryptodisk.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
>> index 1897acc4b..b2c6e9a7d 100644
>> --- a/grub-core/disk/cryptodisk.c
>> +++ b/grub-core/disk/cryptodisk.c
>> @@ -1311,5 +1311,6 @@ GRUB_MOD_FINI (cryptodisk)
>> {
>> grub_disk_dev_unregister (&grub_cryptodisk_dev);
>> cryptodisk_cleanup ();
>
> I am OK with this patch but I realized that cryptodisk_cleanup() body is
> commented out, So, could you add another patch which drops
> cryptodisk_cleanup() entirely from the GRUB code? Or make it work as it
> supposed to work. Maybe the latter is better...
>
> Daniel
>

I can look in to this when I get to the other changes.

Glenn

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

[Daniel Kiper]

On Mon, Sep 21, 2020 at 05:58:06AM +0000, Glenn Washburn wrote:
> 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."

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 <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?

If you touch this code I want it fixed this way or another, If you think
comment should be added in separate patch and/or in different place I am
OK with it.

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

As above, if you think separate patch is better go for it...

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

Please add similar comments to places where these constants are defined.

Daniel

Re: [PATCH v3 4/9] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors

[Daniel Kiper]

On Mon, Sep 21, 2020 at 06:28:28AM +0000, Glenn Washburn wrote:
> Sep 8, 2020 7:21:31 AM Daniel Kiper <daniel.kiper@oracle.com>:
> > On Mon, Sep 07, 2020 at 05:27:46PM +0200, Patrick Steinhardt wrote:
> >> From: Glenn Washburn <development@efficientek.com>
> >>
> >> The total_length field is named confusingly because length usually refers to
> >> bytes, whereas in this case its really the total number of sectors on the
> >> device. Also counter-intuitively, grub_disk_get_size returns the total
> >
> > Could we change total_length name? Or should it stay as is because this
> > name is used in other implementations too?
>
> I sent a patch which renamed total_length to total_sectors. I believe
> Patrick chose not to include it because I did not fix a bug in the
> code and this patch series was only patches he thought essential to be
> included in the next release. I'll include that patch again in a
> follow up patch series.

Please do. I want to have this fixed before 2.06 release...

> >> number of device native sectors sectors. We need to convert the sectors from
> >> the size of the underlying device to the cryptodisk sector size. And
> >> 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 | 7 ++++---
> >> 1 file changed, 4 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> >> index c4c6ac90c..5f15a4d2c 100644
> >> --- a/grub-core/disk/luks2.c
> >> +++ b/grub-core/disk/luks2.c
> >> @@ -417,7 +417,7 @@ luks2_decrypt_key (grub_uint8_t *out_key,
> >> grub_uint8_t salt[GRUB_CRYPTODISK_MAX_KEYLEN];
> >> grub_uint8_t *split_key = NULL;
> >> grub_size_t saltlen = sizeof (salt);
> >> -  char cipher[32], *p;;
> >> +  char cipher[32], *p;
> >
> > I am OK with changes like that but they should be mentioned shortly in
> > the commit message.
>
> Noted, I'll put update the commit message.
>
> >> const gcry_md_spec_t *hash;
> >> gcry_err_code_t gcry_ret;
> >> grub_err_t ret;
> >> @@ -603,9 +603,10 @@ luks2_recover_key (grub_disk_t disk,
> >> crypt->log_sector_size = sizeof (unsigned int) * 8
> >> - __builtin_clz ((unsigned int) segment.sector_size) - 1;
> >> if (grub_strcmp (segment.size, "dynamic") == 0)
> >> - crypt->total_length = grub_disk_get_size (disk) - crypt->offset;
> >> + crypt->total_length = (grub_disk_get_size (disk) >> (crypt->log_sector_size - disk->log_sector_size))
> >> +            - crypt->offset;
> >> else
> >> - crypt->total_length = grub_strtoull (segment.size, NULL, 10);
> >> + crypt->total_length = grub_strtoull (segment.size, NULL, 10) >> crypt->log_sector_size;
> >
> > I do not like that you ignore grub_strtoull() errors. Additionally, what
> > will happen if segment.size is smaller than LUKS2 sector size? Should
> > not you round segment.size up to the nearest multiple of LUKS2 sector
> > size first? I think the same applies to the earlier change too.
>
> Again, I was making a minimal set of changes for this fix. Your
> comments about grub_strtoull, while valid, don't apply to this patch
> and should be addressed in a new patch.

OK, please fix it then in separate patch.

> Your concern about rounding segment.size up, is also valid and
> pertinent to this patch, I'll update that in a following patch series.
> This may get more complicated if the last partial sector is at the end
> of the disk.

Yeah, but please try to fix it somehow...

Daniel

Re: [PATCH v3 3/9] luks2: Fix use of incorrect index and some error messages

[Daniel Kiper]

On Mon, Sep 21, 2020 at 06:45:04AM +0000, Glenn Washburn wrote:
> Sep 8, 2020 6:58:48 AM Daniel Kiper <daniel.kiper@oracle.com>:
> > On Mon, Sep 07, 2020 at 05:27:41PM +0200, Patrick Steinhardt wrote:
> >> From: Glenn Washburn <development@efficientek.com>
> >
> > It seems to me this patch should be split into two and and begs for
> > commit message improvement. In general it would be nice to know why
> > we need these fixes.
>
> This patch is doing two things, fixing a bug and making the code more
> understandable. My original patch just fixed the bug, then Patrick
> suggested making it more readable. I can split it into two patches.

That will be perfect!

Daniel

Re: [PATCH v3 6/9] cryptodisk: Unregister cryptomount command when removing module

[Daniel Kiper]

On Mon, Sep 21, 2020 at 06:45:57AM +0000, Glenn Washburn wrote:
> Sep 8, 2020 7:28:13 AM Daniel Kiper <daniel.kiper@oracle.com>:
> > On Mon, Sep 07, 2020 at 05:27:55PM +0200, Patrick Steinhardt wrote:
> >> From: Glenn Washburn <development@efficientek.com>
> >>
> >> Signed-off-by: Glenn Washburn <development@efficientek.com>
> >> Reviewed-by: Patrick Steinhardt <ps@pks.im>
> >> ---
> >> grub-core/disk/cryptodisk.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> >> index 1897acc4b..b2c6e9a7d 100644
> >> --- a/grub-core/disk/cryptodisk.c
> >> +++ b/grub-core/disk/cryptodisk.c
> >> @@ -1311,5 +1311,6 @@ GRUB_MOD_FINI (cryptodisk)
> >> {
> >> grub_disk_dev_unregister (&grub_cryptodisk_dev);
> >> cryptodisk_cleanup ();
> >
> > I am OK with this patch but I realized that cryptodisk_cleanup() body is
> > commented out, So, could you add another patch which drops
> > cryptodisk_cleanup() entirely from the GRUB code? Or make it work as it
> > supposed to work. Maybe the latter is better...
> >
> > Daniel
> >
>
> I can look in to this when I get to the other changes.

OK...

Daniel

Re: [PATCH v3 4/9] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors

[Glenn Washburn]

On Mon, 21 Sep 2020 13:23:04 +0200
Daniel Kiper <daniel.kiper@oracle.com> wrote:

> On Mon, Sep 21, 2020 at 06:28:28AM +0000, Glenn Washburn wrote:
> > Sep 8, 2020 7:21:31 AM Daniel Kiper <daniel.kiper@oracle.com>:
> > > On Mon, Sep 07, 2020 at 05:27:46PM +0200, Patrick Steinhardt
> > > wrote:
> > >> From: Glenn Washburn <development@efficientek.com>
> > >>
> > >> The total_length field is named confusingly because length
> > >> usually refers to bytes, whereas in this case its really the
> > >> total number of sectors on the device. Also counter-intuitively,
> > >> grub_disk_get_size returns the total
> > >
> > > Could we change total_length name? Or should it stay as is
> > > because this name is used in other implementations too?
> >
> > I sent a patch which renamed total_length to total_sectors. I
> > believe Patrick chose not to include it because I did not fix a bug
> > in the code and this patch series was only patches he thought
> > essential to be included in the next release. I'll include that
> > patch again in a follow up patch series.
> 
> Please do. I want to have this fixed before 2.06 release...
> 
> > >> number of device native sectors sectors. We need to convert the
> > >> sectors from the size of the underlying device to the cryptodisk
> > >> sector size. And 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 | 7 ++++---
> > >> 1 file changed, 4 insertions(+), 3 deletions(-)
> > >>
> > >> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> > >> index c4c6ac90c..5f15a4d2c 100644
> > >> --- a/grub-core/disk/luks2.c
> > >> +++ b/grub-core/disk/luks2.c
> > >> @@ -417,7 +417,7 @@ luks2_decrypt_key (grub_uint8_t *out_key,
> > >> grub_uint8_t salt[GRUB_CRYPTODISK_MAX_KEYLEN];
> > >> grub_uint8_t *split_key = NULL;
> > >> grub_size_t saltlen = sizeof (salt);
> > >> -  char cipher[32], *p;;
> > >> +  char cipher[32], *p;
> > >
> > > I am OK with changes like that but they should be mentioned
> > > shortly in the commit message.
> >
> > Noted, I'll put update the commit message.
> >
> > >> const gcry_md_spec_t *hash;
> > >> gcry_err_code_t gcry_ret;
> > >> grub_err_t ret;
> > >> @@ -603,9 +603,10 @@ luks2_recover_key (grub_disk_t disk,
> > >> crypt->log_sector_size = sizeof (unsigned int) * 8
> > >> - __builtin_clz ((unsigned int) segment.sector_size) - 1;
> > >> if (grub_strcmp (segment.size, "dynamic") == 0)
> > >> - crypt->total_length = grub_disk_get_size (disk) -
> > >> crypt->offset;
> > >> + crypt->total_length = (grub_disk_get_size (disk) >>
> > >> (crypt->log_sector_size - disk->log_sector_size))
> > >> +            - crypt->offset;
> > >> else
> > >> - crypt->total_length = grub_strtoull (segment.size, NULL, 10);
> > >> + crypt->total_length = grub_strtoull (segment.size, NULL, 10)
> > >> >> crypt->log_sector_size;
> > >
> > > I do not like that you ignore grub_strtoull() errors.
> > > Additionally, what will happen if segment.size is smaller than
> > > LUKS2 sector size? Should not you round segment.size up to the
> > > nearest multiple of LUKS2 sector size first? I think the same
> > > applies to the earlier change too.
> >
> > Again, I was making a minimal set of changes for this fix. Your
> > comments about grub_strtoull, while valid, don't apply to this patch
> > and should be addressed in a new patch.
> 
> OK, please fix it then in separate patch.

I've now looked in this more and feel that ignoring grub_strtoull()
errors is not a bad idea.  There are two error states where the
return value is either 0 if the first character is not a valid digit or
(1<<64)-1 in the case of overflow (actually could be more depending on
size of long long type). If grub_strtoull() returns 0 as an error, then
the segment size string is not compliant with the specification.  If
grub_strtoull() returns an error because of overflow, then the segment
size is greater than 16 exbibytes or 16777216 tebibytes.  If someone
has that size storage capacity, I'll wager they are not booting grub
off that storage.  And even if I'm wrong, its an even more
astronomically improbable that they would need to read past the 16th
exbibyte.  As is, this error case would still allow decrypting LUKS
sectors up to the 16th exbibyte.

Also, I looked at grub_strtoull() in hdparm.c, acpi.c, xnu_uuid.c, and
iorw.c and none of those do any error checking of grub_strtoull()
errors.  In fact, this could have serious implications for a typo in
iorw.

My professional conclusion is that I see no reason to do any error
checking.  Do you have a suggestion on how you would like the
grub_strtoull() errors handled?

> > Your concern about rounding segment.size up, is also valid and
> > pertinent to this patch, I'll update that in a following patch
> > series. This may get more complicated if the last partial sector is
> > at the end of the disk.
> 
> Yeah, but please try to fix it somehow...

On second thought, this is an edge case for a nonexistent problem.  If
segment.size is smaller than the LUKS2 sector size, then you have a
segment size of less than 4K, the current max supported sector size.
And having a filesystem smaller than 4K is pathological and I dare say
not supported by any filesystem supported by linux.

There's a more general problem of a segment size that is not a multiple
of the sector size.  In this case, there could be unreadable (by grub)
data at the end of the device.  But again, this is not something we
should worry about.  The cryptsetup program will refuse to create LUKS2
devices where the disk size is not a multiple of the sector size. It
will give the error: "Device size is not aligned to requested sector
size." The only ways I can think of where the segment size is not a
multiple of sector size is if the segment size string is corrupted or
set incorrectly.  In either case, reading the last partial sector isn't
going to matter.  The same logic holds for the case where sector size
is "dynamic".

So currently, I do not think we should support reading partial LUKS2
sectors at the end of a LUKS2 device.  And regardless, whether or not
we support reading partial sectors should not be something that
prevents this patch, which fixes a bug, from being merged.  Do you
disagree?

Glenn

Re: [PATCH v3 4/9] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors

[Daniel Kiper]

On Sat, Oct 03, 2020 at 12:42:55AM -0500, Glenn Washburn wrote:
> On Mon, 21 Sep 2020 13:23:04 +0200
> Daniel Kiper <daniel.kiper@oracle.com> wrote:
>
> > On Mon, Sep 21, 2020 at 06:28:28AM +0000, Glenn Washburn wrote:
> > > Sep 8, 2020 7:21:31 AM Daniel Kiper <daniel.kiper@oracle.com>:
> > > > On Mon, Sep 07, 2020 at 05:27:46PM +0200, Patrick Steinhardt
> > > > wrote:
> > > >> From: Glenn Washburn <development@efficientek.com>
> > > >>
> > > >> The total_length field is named confusingly because length
> > > >> usually refers to bytes, whereas in this case its really the
> > > >> total number of sectors on the device. Also counter-intuitively,
> > > >> grub_disk_get_size returns the total
> > > >
> > > > Could we change total_length name? Or should it stay as is
> > > > because this name is used in other implementations too?
> > >
> > > I sent a patch which renamed total_length to total_sectors. I
> > > believe Patrick chose not to include it because I did not fix a bug
> > > in the code and this patch series was only patches he thought
> > > essential to be included in the next release. I'll include that
> > > patch again in a follow up patch series.
> >
> > Please do. I want to have this fixed before 2.06 release...
> >
> > > >> number of device native sectors sectors. We need to convert the
> > > >> sectors from the size of the underlying device to the cryptodisk
> > > >> sector size. And 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 | 7 ++++---
> > > >> 1 file changed, 4 insertions(+), 3 deletions(-)
> > > >>
> > > >> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> > > >> index c4c6ac90c..5f15a4d2c 100644
> > > >> --- a/grub-core/disk/luks2.c
> > > >> +++ b/grub-core/disk/luks2.c
> > > >> @@ -417,7 +417,7 @@ luks2_decrypt_key (grub_uint8_t *out_key,
> > > >> grub_uint8_t salt[GRUB_CRYPTODISK_MAX_KEYLEN];
> > > >> grub_uint8_t *split_key = NULL;
> > > >> grub_size_t saltlen = sizeof (salt);
> > > >> -  char cipher[32], *p;;
> > > >> +  char cipher[32], *p;
> > > >
> > > > I am OK with changes like that but they should be mentioned
> > > > shortly in the commit message.
> > >
> > > Noted, I'll put update the commit message.
> > >
> > > >> const gcry_md_spec_t *hash;
> > > >> gcry_err_code_t gcry_ret;
> > > >> grub_err_t ret;
> > > >> @@ -603,9 +603,10 @@ luks2_recover_key (grub_disk_t disk,
> > > >> crypt->log_sector_size = sizeof (unsigned int) * 8
> > > >> - __builtin_clz ((unsigned int) segment.sector_size) - 1;
> > > >> if (grub_strcmp (segment.size, "dynamic") == 0)
> > > >> - crypt->total_length = grub_disk_get_size (disk) -
> > > >> crypt->offset;
> > > >> + crypt->total_length = (grub_disk_get_size (disk) >>
> > > >> (crypt->log_sector_size - disk->log_sector_size))
> > > >> +            - crypt->offset;
> > > >> else
> > > >> - crypt->total_length = grub_strtoull (segment.size, NULL, 10);
> > > >> + crypt->total_length = grub_strtoull (segment.size, NULL, 10)
> > > >> >> crypt->log_sector_size;
> > > >
> > > > I do not like that you ignore grub_strtoull() errors.
> > > > Additionally, what will happen if segment.size is smaller than
> > > > LUKS2 sector size? Should not you round segment.size up to the
> > > > nearest multiple of LUKS2 sector size first? I think the same
> > > > applies to the earlier change too.
> > >
> > > Again, I was making a minimal set of changes for this fix. Your
> > > comments about grub_strtoull, while valid, don't apply to this patch
> > > and should be addressed in a new patch.
> >
> > OK, please fix it then in separate patch.
>
> I've now looked in this more and feel that ignoring grub_strtoull()
> errors is not a bad idea.  There are two error states where the
> return value is either 0 if the first character is not a valid digit or
> (1<<64)-1 in the case of overflow (actually could be more depending on
> size of long long type). If grub_strtoull() returns 0 as an error, then
> the segment size string is not compliant with the specification.  If
> grub_strtoull() returns an error because of overflow, then the segment
> size is greater than 16 exbibytes or 16777216 tebibytes.  If someone
> has that size storage capacity, I'll wager they are not booting grub
> off that storage.  And even if I'm wrong, its an even more
> astronomically improbable that they would need to read past the 16th
> exbibyte.  As is, this error case would still allow decrypting LUKS
> sectors up to the 16th exbibyte.
>
> Also, I looked at grub_strtoull() in hdparm.c, acpi.c, xnu_uuid.c, and
> iorw.c and none of those do any error checking of grub_strtoull()
> errors.  In fact, this could have serious implications for a typo in
> iorw.

I know about these issues and I think they should be fixed at some
point. Or at least there should be a comment added why it is safe here
and there to ignore grub_strtoull() errors. Anyway, it is not an excuse
to do things in wrong way if we are touching this code here.

> My professional conclusion is that I see no reason to do any error
> checking.  Do you have a suggestion on how you would like the
> grub_strtoull() errors handled?

What is the worst scenario if somebody plays bad games with segment.size
string? If nothing dangerous happens I am OK with the comment explaining
why it is safe to ignore grub_strtoull() errors here.

> > > Your concern about rounding segment.size up, is also valid and
> > > pertinent to this patch, I'll update that in a following patch
> > > series. This may get more complicated if the last partial sector is
> > > at the end of the disk.
> >
> > Yeah, but please try to fix it somehow...
>
> On second thought, this is an edge case for a nonexistent problem.  If
> segment.size is smaller than the LUKS2 sector size, then you have a
> segment size of less than 4K, the current max supported sector size.
> And having a filesystem smaller than 4K is pathological and I dare say
> not supported by any filesystem supported by linux.
>
> There's a more general problem of a segment size that is not a multiple
> of the sector size.  In this case, there could be unreadable (by grub)
> data at the end of the device.  But again, this is not something we
> should worry about.  The cryptsetup program will refuse to create LUKS2
> devices where the disk size is not a multiple of the sector size. It
> will give the error: "Device size is not aligned to requested sector
> size." The only ways I can think of where the segment size is not a
> multiple of sector size is if the segment size string is corrupted or
> set incorrectly.  In either case, reading the last partial sector isn't
> going to matter.  The same logic holds for the case where sector size
> is "dynamic".
>
> So currently, I do not think we should support reading partial LUKS2
> sectors at the end of a LUKS2 device.  And regardless, whether or not
> we support reading partial sectors should not be something that
> prevents this patch, which fixes a bug, from being merged.  Do you
> disagree?

I am not saying we should care about such crazy scenarios. However,
I care a lot if GRUB fails safely in cases where somebody feeds it
with invalid data. So, please add code which protects against crashes
or explain in the comments why such protections are not needed.

Daniel

Re: [PATCH v3 4/9] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors

[Glenn Washburn]

On Tue, 27 Oct 2020 20:11:56 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Sat, Oct 03, 2020 at 12:42:55AM -0500, Glenn Washburn wrote:
> > On Mon, 21 Sep 2020 13:23:04 +0200
> > Daniel Kiper <daniel.kiper@oracle.com> wrote:
> >
> > > On Mon, Sep 21, 2020 at 06:28:28AM +0000, Glenn Washburn wrote:
> > > > Sep 8, 2020 7:21:31 AM Daniel Kiper <daniel.kiper@oracle.com>:
> > > > > On Mon, Sep 07, 2020 at 05:27:46PM +0200, Patrick Steinhardt
> > > > > wrote:
> > > > >> From: Glenn Washburn <development@efficientek.com>
> > > > >>
> > > > >> The total_length field is named confusingly because length
> > > > >> usually refers to bytes, whereas in this case its really the
> > > > >> total number of sectors on the device. Also
> > > > >> counter-intuitively, grub_disk_get_size returns the total
> > > > >
> > > > > Could we change total_length name? Or should it stay as is
> > > > > because this name is used in other implementations too?
> > > >
> > > > I sent a patch which renamed total_length to total_sectors. I
> > > > believe Patrick chose not to include it because I did not fix a
> > > > bug in the code and this patch series was only patches he
> > > > thought essential to be included in the next release. I'll
> > > > include that patch again in a follow up patch series.
> > >
> > > Please do. I want to have this fixed before 2.06 release...
> > >
> > > > >> number of device native sectors sectors. We need to convert
> > > > >> the sectors from the size of the underlying device to the
> > > > >> cryptodisk sector size. And 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 | 7 ++++---
> > > > >> 1 file changed, 4 insertions(+), 3 deletions(-)
> > > > >>
> > > > >> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> > > > >> index c4c6ac90c..5f15a4d2c 100644
> > > > >> --- a/grub-core/disk/luks2.c
> > > > >> +++ b/grub-core/disk/luks2.c
> > > > >> @@ -417,7 +417,7 @@ luks2_decrypt_key (grub_uint8_t *out_key,
> > > > >> grub_uint8_t salt[GRUB_CRYPTODISK_MAX_KEYLEN];
> > > > >> grub_uint8_t *split_key = NULL;
> > > > >> grub_size_t saltlen = sizeof (salt);
> > > > >> -  char cipher[32], *p;;
> > > > >> +  char cipher[32], *p;
> > > > >
> > > > > I am OK with changes like that but they should be mentioned
> > > > > shortly in the commit message.
> > > >
> > > > Noted, I'll put update the commit message.
> > > >
> > > > >> const gcry_md_spec_t *hash;
> > > > >> gcry_err_code_t gcry_ret;
> > > > >> grub_err_t ret;
> > > > >> @@ -603,9 +603,10 @@ luks2_recover_key (grub_disk_t disk,
> > > > >> crypt->log_sector_size = sizeof (unsigned int) * 8
> > > > >> - __builtin_clz ((unsigned int) segment.sector_size) - 1;
> > > > >> if (grub_strcmp (segment.size, "dynamic") == 0)
> > > > >> - crypt->total_length = grub_disk_get_size (disk) -
> > > > >> crypt->offset;
> > > > >> + crypt->total_length = (grub_disk_get_size (disk) >>
> > > > >> (crypt->log_sector_size - disk->log_sector_size))
> > > > >> +            - crypt->offset;
> > > > >> else
> > > > >> - crypt->total_length = grub_strtoull (segment.size, NULL,
> > > > >> 10);
> > > > >> + crypt->total_length = grub_strtoull (segment.size, NULL,
> > > > >> 10)
> > > > >> >> crypt->log_sector_size;
> > > > >
> > > > > I do not like that you ignore grub_strtoull() errors.
> > > > > Additionally, what will happen if segment.size is smaller than
> > > > > LUKS2 sector size? Should not you round segment.size up to the
> > > > > nearest multiple of LUKS2 sector size first? I think the same
> > > > > applies to the earlier change too.
> > > >
> > > > Again, I was making a minimal set of changes for this fix. Your
> > > > comments about grub_strtoull, while valid, don't apply to this
> > > > patch and should be addressed in a new patch.
> > >
> > > OK, please fix it then in separate patch.
> >
> > I've now looked in this more and feel that ignoring grub_strtoull()
> > errors is not a bad idea.  There are two error states where the
> > return value is either 0 if the first character is not a valid
> > digit or (1<<64)-1 in the case of overflow (actually could be more
> > depending on size of long long type). If grub_strtoull() returns 0
> > as an error, then the segment size string is not compliant with the
> > specification.  If grub_strtoull() returns an error because of
> > overflow, then the segment size is greater than 16 exbibytes or
> > 16777216 tebibytes.  If someone has that size storage capacity,
> > I'll wager they are not booting grub off that storage.  And even if
> > I'm wrong, its an even more astronomically improbable that they
> > would need to read past the 16th exbibyte.  As is, this error case
> > would still allow decrypting LUKS sectors up to the 16th exbibyte.
> >
> > Also, I looked at grub_strtoull() in hdparm.c, acpi.c, xnu_uuid.c,
> > and iorw.c and none of those do any error checking of
> > grub_strtoull() errors.  In fact, this could have serious
> > implications for a typo in iorw.
> 
> I know about these issues and I think they should be fixed at some
> point. Or at least there should be a comment added why it is safe here
> and there to ignore grub_strtoull() errors. Anyway, it is not an
> excuse to do things in wrong way if we are touching this code here.

Actually, I do think this is a good excuse to do things wrong _IF_ what
is wrong is also wrong in the original code.  Are you going to reject a
patch that _fixes_ a bug, because it does not fix a different bug around
it? So after rejecting the patch, then _both_ bugs will continue to
exist? This seems quite ridiculous to me.

> > My professional conclusion is that I see no reason to do any error
> > checking.  Do you have a suggestion on how you would like the
> > grub_strtoull() errors handled?
> 
> What is the worst scenario if somebody plays bad games with
> segment.size string? If nothing dangerous happens I am OK with the
> comment explaining why it is safe to ignore grub_strtoull() errors
> here.

I think part of my pushback on this is I don't see a good solution. How
do you know when grub_strtoull() errors here?  And even if it doesn't
error, how do you know that a segment.size of 3 or 128 won't cause a
crash? I don't have good answers to these questions.  If grub does
crash, then a bug has been exposed in grub which should be fixed.
Perhaps if we had functional testing (see my previous patch series
implementing functional testing), we could test this.  But even then
there's a problem, how do we know some grub file system or disk doesn't
have a crashing bug on too small of disks?

> > > > Your concern about rounding segment.size up, is also valid and
> > > > pertinent to this patch, I'll update that in a following patch
> > > > series. This may get more complicated if the last partial
> > > > sector is at the end of the disk.
> > >
> > > Yeah, but please try to fix it somehow...
> >
> > On second thought, this is an edge case for a nonexistent problem.
> > If segment.size is smaller than the LUKS2 sector size, then you
> > have a segment size of less than 4K, the current max supported
> > sector size. And having a filesystem smaller than 4K is
> > pathological and I dare say not supported by any filesystem
> > supported by linux.
> >
> > There's a more general problem of a segment size that is not a
> > multiple of the sector size.  In this case, there could be
> > unreadable (by grub) data at the end of the device.  But again,
> > this is not something we should worry about.  The cryptsetup
> > program will refuse to create LUKS2 devices where the disk size is
> > not a multiple of the sector size. It will give the error: "Device
> > size is not aligned to requested sector size." The only ways I can
> > think of where the segment size is not a multiple of sector size is
> > if the segment size string is corrupted or set incorrectly.  In
> > either case, reading the last partial sector isn't going to matter.
> >  The same logic holds for the case where sector size is "dynamic".
> >
> > So currently, I do not think we should support reading partial LUKS2
> > sectors at the end of a LUKS2 device.  And regardless, whether or
> > not we support reading partial sectors should not be something that
> > prevents this patch, which fixes a bug, from being merged.  Do you
> > disagree?
> 
> I am not saying we should care about such crazy scenarios. However,
> I care a lot if GRUB fails safely in cases where somebody feeds it
> with invalid data. So, please add code which protects against crashes
> or explain in the comments why such protections are not needed.
> 
> Daniel

Since you seem to have a clear idea of what should be done here,
perhaps you insert a patch to your liking?  Or just tell me exactly
what you think should be done to protect against crashes.  I can just
add a zero check if that's what you want.  But that just adding a
"check" to check a box and make people feel safe and comfortable that
something is being done, when in fact it may do little to fix a
potential crash.  When you say "somebody feeds it with invalid data", I
take you to be concerned about someone maliciously crafting data to
exploit grub, in which case a more in-depth audit of the use of
total_length and offset should be done.  Perhaps a compromise would be
a comment saying "FIXME: Verify that grub does not crash for any value
of total_length, offset, and sector_size combination."

Honestly, I'm frustrated at how much time this whole patch series is
requiring of me and dragging on. I feel like this patch is being held
hostage in order to strong-arm me in to fixing something unrelated to
my patch.

Glenn

Re: [PATCH v3 4/9] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors

[Daniel Kiper]

On Thu, Oct 29, 2020 at 02:53:34PM -0500, Glenn Washburn wrote:
> On Tue, 27 Oct 2020 20:11:56 +0100
> Daniel Kiper <dkiper@net-space.pl> wrote:
> > On Sat, Oct 03, 2020 at 12:42:55AM -0500, Glenn Washburn wrote:
> > > On Mon, 21 Sep 2020 13:23:04 +0200
> > > Daniel Kiper <daniel.kiper@oracle.com> wrote:
> > > > On Mon, Sep 21, 2020 at 06:28:28AM +0000, Glenn Washburn wrote:
> > > > > Sep 8, 2020 7:21:31 AM Daniel Kiper <daniel.kiper@oracle.com>:
> > > > > > On Mon, Sep 07, 2020 at 05:27:46PM +0200, Patrick Steinhardt
> > > > > > wrote:
> > > > > >> From: Glenn Washburn <development@efficientek.com>
> > > > > >>
> > > > > >> The total_length field is named confusingly because length
> > > > > >> usually refers to bytes, whereas in this case its really the
> > > > > >> total number of sectors on the device. Also
> > > > > >> counter-intuitively, grub_disk_get_size returns the total
> > > > > >
> > > > > > Could we change total_length name? Or should it stay as is
> > > > > > because this name is used in other implementations too?
> > > > >
> > > > > I sent a patch which renamed total_length to total_sectors. I
> > > > > believe Patrick chose not to include it because I did not fix a
> > > > > bug in the code and this patch series was only patches he
> > > > > thought essential to be included in the next release. I'll
> > > > > include that patch again in a follow up patch series.
> > > >
> > > > Please do. I want to have this fixed before 2.06 release...
> > > >
> > > > > >> number of device native sectors sectors. We need to convert
> > > > > >> the sectors from the size of the underlying device to the
> > > > > >> cryptodisk sector size. And 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 | 7 ++++---
> > > > > >> 1 file changed, 4 insertions(+), 3 deletions(-)
> > > > > >>
> > > > > >> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> > > > > >> index c4c6ac90c..5f15a4d2c 100644
> > > > > >> --- a/grub-core/disk/luks2.c
> > > > > >> +++ b/grub-core/disk/luks2.c
> > > > > >> @@ -417,7 +417,7 @@ luks2_decrypt_key (grub_uint8_t *out_key,
> > > > > >> grub_uint8_t salt[GRUB_CRYPTODISK_MAX_KEYLEN];
> > > > > >> grub_uint8_t *split_key = NULL;
> > > > > >> grub_size_t saltlen = sizeof (salt);
> > > > > >> -  char cipher[32], *p;;
> > > > > >> +  char cipher[32], *p;
> > > > > >
> > > > > > I am OK with changes like that but they should be mentioned
> > > > > > shortly in the commit message.
> > > > >
> > > > > Noted, I'll put update the commit message.
> > > > >
> > > > > >> const gcry_md_spec_t *hash;
> > > > > >> gcry_err_code_t gcry_ret;
> > > > > >> grub_err_t ret;
> > > > > >> @@ -603,9 +603,10 @@ luks2_recover_key (grub_disk_t disk,
> > > > > >> crypt->log_sector_size = sizeof (unsigned int) * 8
> > > > > >> - __builtin_clz ((unsigned int) segment.sector_size) - 1;
> > > > > >> if (grub_strcmp (segment.size, "dynamic") == 0)
> > > > > >> - crypt->total_length = grub_disk_get_size (disk) -
> > > > > >> crypt->offset;
> > > > > >> + crypt->total_length = (grub_disk_get_size (disk) >>
> > > > > >> (crypt->log_sector_size - disk->log_sector_size))
> > > > > >> +            - crypt->offset;
> > > > > >> else
> > > > > >> - crypt->total_length = grub_strtoull (segment.size, NULL,
> > > > > >> 10);
> > > > > >> + crypt->total_length = grub_strtoull (segment.size, NULL,
> > > > > >> 10)
> > > > > >> >> crypt->log_sector_size;
> > > > > >
> > > > > > I do not like that you ignore grub_strtoull() errors.
> > > > > > Additionally, what will happen if segment.size is smaller than
> > > > > > LUKS2 sector size? Should not you round segment.size up to the
> > > > > > nearest multiple of LUKS2 sector size first? I think the same
> > > > > > applies to the earlier change too.
> > > > >
> > > > > Again, I was making a minimal set of changes for this fix. Your
> > > > > comments about grub_strtoull, while valid, don't apply to this
> > > > > patch and should be addressed in a new patch.
> > > >
> > > > OK, please fix it then in separate patch.
> > >
> > > I've now looked in this more and feel that ignoring grub_strtoull()
> > > errors is not a bad idea.  There are two error states where the
> > > return value is either 0 if the first character is not a valid
> > > digit or (1<<64)-1 in the case of overflow (actually could be more
> > > depending on size of long long type). If grub_strtoull() returns 0
> > > as an error, then the segment size string is not compliant with the
> > > specification.  If grub_strtoull() returns an error because of
> > > overflow, then the segment size is greater than 16 exbibytes or
> > > 16777216 tebibytes.  If someone has that size storage capacity,
> > > I'll wager they are not booting grub off that storage.  And even if
> > > I'm wrong, its an even more astronomically improbable that they
> > > would need to read past the 16th exbibyte.  As is, this error case
> > > would still allow decrypting LUKS sectors up to the 16th exbibyte.
> > >
> > > Also, I looked at grub_strtoull() in hdparm.c, acpi.c, xnu_uuid.c,
> > > and iorw.c and none of those do any error checking of
> > > grub_strtoull() errors.  In fact, this could have serious
> > > implications for a typo in iorw.
> >
> > I know about these issues and I think they should be fixed at some
> > point. Or at least there should be a comment added why it is safe here
> > and there to ignore grub_strtoull() errors. Anyway, it is not an
> > excuse to do things in wrong way if we are touching this code here.
>
> Actually, I do think this is a good excuse to do things wrong _IF_ what
> is wrong is also wrong in the original code.  Are you going to reject a
> patch that _fixes_ a bug, because it does not fix a different bug around
> it? So after rejecting the patch, then _both_ bugs will continue to
> exist? This seems quite ridiculous to me.

No, this was never my goal. I am just trying to find the best solution here...

> > > My professional conclusion is that I see no reason to do any error
> > > checking.  Do you have a suggestion on how you would like the
> > > grub_strtoull() errors handled?
> >
> > What is the worst scenario if somebody plays bad games with
> > segment.size string? If nothing dangerous happens I am OK with the
> > comment explaining why it is safe to ignore grub_strtoull() errors
> > here.
>
> I think part of my pushback on this is I don't see a good solution. How

OK...

> do you know when grub_strtoull() errors here?  And even if it doesn't

Just check values/errors returned by it?

> error, how do you know that a segment.size of 3 or 128 won't cause a
> crash? I don't have good answers to these questions.  If grub does

This question is more difficult and I am afraid that you are right.

> crash, then a bug has been exposed in grub which should be fixed.

If possible we should prevent against the crashes but I am also aware
that it is not always feasible to predict when the GRUB will crash.

> Perhaps if we had functional testing (see my previous patch series
> implementing functional testing), we could test this.  But even then

I will try to take a look at it next week...

> there's a problem, how do we know some grub file system or disk doesn't
> have a crashing bug on too small of disks?

We can improve a situation a bit here by running some tests you are
mentioning above.

> > > > > Your concern about rounding segment.size up, is also valid and
> > > > > pertinent to this patch, I'll update that in a following patch
> > > > > series. This may get more complicated if the last partial
> > > > > sector is at the end of the disk.
> > > >
> > > > Yeah, but please try to fix it somehow...
> > >
> > > On second thought, this is an edge case for a nonexistent problem.
> > > If segment.size is smaller than the LUKS2 sector size, then you
> > > have a segment size of less than 4K, the current max supported
> > > sector size. And having a filesystem smaller than 4K is
> > > pathological and I dare say not supported by any filesystem
> > > supported by linux.
> > >
> > > There's a more general problem of a segment size that is not a
> > > multiple of the sector size.  In this case, there could be
> > > unreadable (by grub) data at the end of the device.  But again,
> > > this is not something we should worry about.  The cryptsetup
> > > program will refuse to create LUKS2 devices where the disk size is
> > > not a multiple of the sector size. It will give the error: "Device
> > > size is not aligned to requested sector size." The only ways I can
> > > think of where the segment size is not a multiple of sector size is
> > > if the segment size string is corrupted or set incorrectly.  In
> > > either case, reading the last partial sector isn't going to matter.
> > >  The same logic holds for the case where sector size is "dynamic".
> > >
> > > So currently, I do not think we should support reading partial LUKS2
> > > sectors at the end of a LUKS2 device.  And regardless, whether or
> > > not we support reading partial sectors should not be something that
> > > prevents this patch, which fixes a bug, from being merged.  Do you
> > > disagree?
> >
> > I am not saying we should care about such crazy scenarios. However,
> > I care a lot if GRUB fails safely in cases where somebody feeds it
> > with invalid data. So, please add code which protects against crashes
> > or explain in the comments why such protections are not needed.
> >
> > Daniel
>
> Since you seem to have a clear idea of what should be done here,
> perhaps you insert a patch to your liking?  Or just tell me exactly
> what you think should be done to protect against crashes.  I can just
> add a zero check if that's what you want.  But that just adding a
> "check" to check a box and make people feel safe and comfortable that

I am not interested in adding `"check" to check a box`...

> something is being done, when in fact it may do little to fix a
> potential crash.  When you say "somebody feeds it with invalid data", I
> take you to be concerned about someone maliciously crafting data to
> exploit grub, in which case a more in-depth audit of the use of

Yep...

> total_length and offset should be done.  Perhaps a compromise would be

That would be perfect.

> a comment saying "FIXME: Verify that grub does not crash for any value
> of total_length, offset, and sector_size combination."

OK but I would be more happy if you add to the compromise a promise that
you will continue the work on the functional testing mentioned above... :-)

> Honestly, I'm frustrated at how much time this whole patch series is
> requiring of me and dragging on. I feel like this patch is being held

This is partially by my lack of time. However, I hope it will be
changing. Anyway, sorry about the delays on my side.

> hostage in order to strong-arm me in to fixing something unrelated to
> my patch.

I think it is related to some extend. Anyway, I am open to discuss any
solution to this issue except ignoring it. Though I think we are close
to the compromise... :-)

Daniel

Re: [PATCH v3 4/9] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors

[Glenn Washburn]

Oct 30, 2020 7:50:08 AM Daniel Kiper <dkiper@net-space.pl>:

> On Thu, Oct 29, 2020 at 02:53:34PM -0500, Glenn Washburn wrote:
>> On Tue, 27 Oct 2020 20:11:56 +0100
>> Daniel Kiper <dkiper@net-space.pl> wrote:
>>> On Sat, Oct 03, 2020 at 12:42:55AM -0500, Glenn Washburn wrote:
>>>> On Mon, 21 Sep 2020 13:23:04 +0200
>>>> Daniel Kiper <daniel.kiper@oracle.com> wrote:
>>>>> On Mon, Sep 21, 2020 at 06:28:28AM +0000, Glenn Washburn wrote:
>>>>>> Sep 8, 2020 7:21:31 AM Daniel Kiper <daniel.kiper@oracle.com>:
>>>>>>> On Mon, Sep 07, 2020 at 05:27:46PM +0200, Patrick Steinhardt
>>>>>>> wrote:
>>>>>>>> From: Glenn Washburn <development@efficientek.com>
>>>>>>>>
>>>>>>>> The total_length field is named confusingly because length
>>>>>>>> usually refers to bytes, whereas in this case its really the
>>>>>>>> total number of sectors on the device. Also
>>>>>>>> counter-intuitively, grub_disk_get_size returns the total
>>>>>>>
>>>>>>> Could we change total_length name? Or should it stay as is
>>>>>>> because this name is used in other implementations too?
>>>>>>
>>>>>> I sent a patch which renamed total_length to total_sectors. I
>>>>>> believe Patrick chose not to include it because I did not fix a
>>>>>> bug in the code and this patch series was only patches he
>>>>>> thought essential to be included in the next release. I'll
>>>>>> include that patch again in a follow up patch series.
>>>>>
>>>>> Please do. I want to have this fixed before 2.06 release...
>>>>>
>>>>>>>> number of device native sectors sectors. We need to convert
>>>>>>>> the sectors from the size of the underlying device to the
>>>>>>>> cryptodisk sector size. And 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 | 7 ++++---
>>>>>>>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
>>>>>>>> index c4c6ac90c..5f15a4d2c 100644
>>>>>>>> --- a/grub-core/disk/luks2.c
>>>>>>>> +++ b/grub-core/disk/luks2.c
>>>>>>>> @@ -417,7 +417,7 @@ luks2_decrypt_key (grub_uint8_t *out_key,
>>>>>>>> grub_uint8_t salt[GRUB_CRYPTODISK_MAX_KEYLEN];
>>>>>>>> grub_uint8_t *split_key = NULL;
>>>>>>>> grub_size_t saltlen = sizeof (salt);
>>>>>>>> -  char cipher[32], *p;;
>>>>>>>> +  char cipher[32], *p;
>>>>>>>
>>>>>>> I am OK with changes like that but they should be mentioned
>>>>>>> shortly in the commit message.
>>>>>>
>>>>>> Noted, I'll put update the commit message.
>>>>>>
>>>>>>>> const gcry_md_spec_t *hash;
>>>>>>>> gcry_err_code_t gcry_ret;
>>>>>>>> grub_err_t ret;
>>>>>>>> @@ -603,9 +603,10 @@ luks2_recover_key (grub_disk_t disk,
>>>>>>>> crypt->log_sector_size = sizeof (unsigned int) * 8
>>>>>>>> - __builtin_clz ((unsigned int) segment.sector_size) - 1;
>>>>>>>> if (grub_strcmp (segment.size, "dynamic") == 0)
>>>>>>>> - crypt->total_length = grub_disk_get_size (disk) -
>>>>>>>> crypt->offset;
>>>>>>>> + crypt->total_length = (grub_disk_get_size (disk) >>
>>>>>>>> (crypt->log_sector_size - disk->log_sector_size))
>>>>>>>> +            - crypt->offset;
>>>>>>>> else
>>>>>>>> - crypt->total_length = grub_strtoull (segment.size, NULL,
>>>>>>>> 10);
>>>>>>>> + crypt->total_length = grub_strtoull (segment.size, NULL,
>>>>>>>> 10)
>>>>>>>>>> crypt->log_sector_size;
>>>>>>>
>>>>>>> I do not like that you ignore grub_strtoull() errors.
>>>>>>> Additionally, what will happen if segment.size is smaller than
>>>>>>> LUKS2 sector size? Should not you round segment.size up to the
>>>>>>> nearest multiple of LUKS2 sector size first? I think the same
>>>>>>> applies to the earlier change too.
>>>>>>
>>>>>> Again, I was making a minimal set of changes for this fix. Your
>>>>>> comments about grub_strtoull, while valid, don't apply to this
>>>>>> patch and should be addressed in a new patch.
>>>>>
>>>>> OK, please fix it then in separate patch.
>>>>
>>>> I've now looked in this more and feel that ignoring grub_strtoull()
>>>> errors is not a bad idea.  There are two error states where the
>>>> return value is either 0 if the first character is not a valid
>>>> digit or (1<<64)-1 in the case of overflow (actually could be more
>>>> depending on size of long long type). If grub_strtoull() returns 0
>>>> as an error, then the segment size string is not compliant with the
>>>> specification.  If grub_strtoull() returns an error because of
>>>> overflow, then the segment size is greater than 16 exbibytes or
>>>> 16777216 tebibytes.  If someone has that size storage capacity,
>>>> I'll wager they are not booting grub off that storage.  And even if
>>>> I'm wrong, its an even more astronomically improbable that they
>>>> would need to read past the 16th exbibyte.  As is, this error case
>>>> would still allow decrypting LUKS sectors up to the 16th exbibyte.
>>>>
>>>> Also, I looked at grub_strtoull() in hdparm.c, acpi.c, xnu_uuid.c,
>>>> and iorw.c and none of those do any error checking of
>>>> grub_strtoull() errors.  In fact, this could have serious
>>>> implications for a typo in iorw.
>>>
>>> I know about these issues and I think they should be fixed at some
>>> point. Or at least there should be a comment added why it is safe here
>>> and there to ignore grub_strtoull() errors. Anyway, it is not an
>>> excuse to do things in wrong way if we are touching this code here.
>>
>> Actually, I do think this is a good excuse to do things wrong _IF_ what
>> is wrong is also wrong in the original code.  Are you going to reject a
>> patch that _fixes_ a bug, because it does not fix a different bug around
>> it? So after rejecting the patch, then _both_ bugs will continue to
>> exist? This seems quite ridiculous to me.
>
> No, this was never my goal. I am just trying to find the best solution here...
>
>>>> My professional conclusion is that I see no reason to do any error
>>>> checking.  Do you have a suggestion on how you would like the
>>>> grub_strtoull() errors handled?
>>>
>>> What is the worst scenario if somebody plays bad games with
>>> segment.size string? If nothing dangerous happens I am OK with the
>>> comment explaining why it is safe to ignore grub_strtoull() errors
>>> here.
>>
>> I think part of my pushback on this is I don't see a good solution. How
>
> OK...
>
>> do you know when grub_strtoull() errors here?  And even if it doesn't
>
> Just check values/errors returned by it?

There are two error values returned by grub_strtoull(), 0 and ~0ULL for unrecognized number and overflow. However, these are _both_ valid non-error return values. So was it an overflow condition or a valid return when ~0ULL is returned? Same for 0. In the case of 0 while it may be valid, it wouldn't reflect a usable segment, so we can filter out those.

>
>> error, how do you know that a segment.size of 3 or 128 won't cause a
>> crash? I don't have good answers to these questions.  If grub does
>
> This question is more difficult and I am afraid that you are right.
>
>> crash, then a bug has been exposed in grub which should be fixed.
>
> If possible we should prevent against the crashes but I am also aware
> that it is not always feasible to predict when the GRUB will crash.

It would be good to detect where grub is crashing because there might be other ways to trigger such a crash (perhaps through loopback?)

>> Perhaps if we had functional testing (see my previous patch series
>> implementing functional testing), we could test this.  But even then
>
> I will try to take a look at it next week...
>
>> there's a problem, how do we know some grub file system or disk doesn't
>> have a crashing bug on too small of disks?
>
> We can improve a situation a bit here by running some tests you are
> mentioning above.
>
>>>>>> Your concern about rounding segment.size up, is also valid and
>>>>>> pertinent to this patch, I'll update that in a following patch
>>>>>> series. This may get more complicated if the last partial
>>>>>> sector is at the end of the disk.
>>>>>
>>>>> Yeah, but please try to fix it somehow...
>>>>
>>>> On second thought, this is an edge case for a nonexistent problem.
>>>> If segment.size is smaller than the LUKS2 sector size, then you
>>>> have a segment size of less than 4K, the current max supported
>>>> sector size. And having a filesystem smaller than 4K is
>>>> pathological and I dare say not supported by any filesystem
>>>> supported by linux.
>>>>
>>>> There's a more general problem of a segment size that is not a
>>>> multiple of the sector size.  In this case, there could be
>>>> unreadable (by grub) data at the end of the device.  But again,
>>>> this is not something we should worry about.  The cryptsetup
>>>> program will refuse to create LUKS2 devices where the disk size is
>>>> not a multiple of the sector size. It will give the error: "Device
>>>> size is not aligned to requested sector size." The only ways I can
>>>> think of where the segment size is not a multiple of sector size is
>>>> if the segment size string is corrupted or set incorrectly.  In
>>>> either case, reading the last partial sector isn't going to matter.
>>>> The same logic holds for the case where sector size is "dynamic".
>>>>
>>>> So currently, I do not think we should support reading partial LUKS2
>>>> sectors at the end of a LUKS2 device.  And regardless, whether or
>>>> not we support reading partial sectors should not be something that
>>>> prevents this patch, which fixes a bug, from being merged.  Do you
>>>> disagree?
>>>
>>> I am not saying we should care about such crazy scenarios. However,
>>> I care a lot if GRUB fails safely in cases where somebody feeds it
>>> with invalid data. So, please add code which protects against crashes
>>> or explain in the comments why such protections are not needed.
>>>
>>> Daniel
>>
>> Since you seem to have a clear idea of what should be done here,
>> perhaps you insert a patch to your liking?  Or just tell me exactly
>> what you think should be done to protect against crashes.  I can just
>> add a zero check if that's what you want.  But that just adding a
>> "check" to check a box and make people feel safe and comfortable that
>
> I am not interested in adding `"check" to check a box`...
>
>> something is being done, when in fact it may do little to fix a
>> potential crash.  When you say "somebody feeds it with invalid data", I
>> take you to be concerned about someone maliciously crafting data to
>> exploit grub, in which case a more in-depth audit of the use of
>
> Yep...
>
>> total_length and offset should be done.  Perhaps a compromise would be
>
> That would be perfect.
>
>> a comment saying "FIXME: Verify that grub does not crash for any value
>> of total_length, offset, and sector_size combination."
>
> OK but I would be more happy if you add to the compromise a promise that
> you will continue the work on the functional testing mentioned above... :-)

Hmm, let me look into it before making a promise. The part with the most work will be adding the ability to create LUKS2 devices that cryptsetup does not currently allow (eg. One with a zero length segment or something grub_strtoull() will error on).

>> Honestly, I'm frustrated at how much time this whole patch series is
>> requiring of me and dragging on. I feel like this patch is being held
>
> This is partially by my lack of time. However, I hope it will be
> changing. Anyway, sorry about the delays on my side.
>
>> hostage in order to strong-arm me in to fixing something unrelated to
>> my patch.
>
> I think it is related to some extend. Anyway, I am open to discuss any
> solution to this issue except ignoring it. Though I think we are close
> to the compromise... :-)

I think a good compromise would be to error on segments where offset > segment.size and crypt->total_length
== 0. And to add a  fixme comment to handle the overflow case for grub_strtoull() better. Overflow won't cause a crash, just the area larger than the overflow amount to be inaccessible. And I don't think we need the previously mentioned fixme, but I'm not opposed to adding it.

Glenn

Re: [PATCH v3 4/9] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors

[Daniel Kiper]

On Tue, Nov 03, 2020 at 08:21:15PM +0000, Glenn Washburn wrote:
> Oct 30, 2020 7:50:08 AM Daniel Kiper <dkiper@net-space.pl>:
> > On Thu, Oct 29, 2020 at 02:53:34PM -0500, Glenn Washburn wrote:
> >> On Tue, 27 Oct 2020 20:11:56 +0100
> >> Daniel Kiper <dkiper@net-space.pl> wrote:
> >>> On Sat, Oct 03, 2020 at 12:42:55AM -0500, Glenn Washburn wrote:
> >>>> On Mon, 21 Sep 2020 13:23:04 +0200
> >>>> Daniel Kiper <daniel.kiper@oracle.com> wrote:
> >>>>> On Mon, Sep 21, 2020 at 06:28:28AM +0000, Glenn Washburn wrote:
> >>>>>> Sep 8, 2020 7:21:31 AM Daniel Kiper <daniel.kiper@oracle.com>:
> >>>>>>> On Mon, Sep 07, 2020 at 05:27:46PM +0200, Patrick Steinhardt
> >>>>>>> wrote:

[...]

> >>> What is the worst scenario if somebody plays bad games with
> >>> segment.size string? If nothing dangerous happens I am OK with the
> >>> comment explaining why it is safe to ignore grub_strtoull() errors
> >>> here.
> >>
> >> I think part of my pushback on this is I don't see a good solution. How
> >
> > OK...
> >
> >> do you know when grub_strtoull() errors here?  And even if it doesn't
> >
> > Just check values/errors returned by it?
>
> There are two error values returned by grub_strtoull(), 0 and ~0ULL
> for unrecognized number and overflow. However, these are _both_ valid
> non-error return values. So was it an overflow condition or a valid
> return when ~0ULL is returned? Same for 0. In the case of 0 while it
> may be valid, it wouldn't reflect a usable segment, so we can filter
> out those.

That is why you should check grub_errno too. In general "man strtoull"
is your friend. However, I agree that this does not prevent against
overflows later. So, I think we should have error checks for
grub_strtoull() to detect incorrect input and further some other checks
for overflows in math if needed. Potentially we can use only the latter
if we put these protection properly.

> >> error, how do you know that a segment.size of 3 or 128 won't cause a
> >> crash? I don't have good answers to these questions.  If grub does
> >
> > This question is more difficult and I am afraid that you are right.
> >
> >> crash, then a bug has been exposed in grub which should be fixed.
> >
> > If possible we should prevent against the crashes but I am also aware
> > that it is not always feasible to predict when the GRUB will crash.
>
> It would be good to detect where grub is crashing because there might
> be other ways to trigger such a crash (perhaps through loopback?)

Yeah, but I think we get back to tests here...

[...]

> >> Since you seem to have a clear idea of what should be done here,
> >> perhaps you insert a patch to your liking?  Or just tell me exactly
> >> what you think should be done to protect against crashes.  I can just
> >> add a zero check if that's what you want.  But that just adding a
> >> "check" to check a box and make people feel safe and comfortable that
> >
> > I am not interested in adding `"check" to check a box`...
> >
> >> something is being done, when in fact it may do little to fix a
> >> potential crash.  When you say "somebody feeds it with invalid data", I
> >> take you to be concerned about someone maliciously crafting data to
> >> exploit grub, in which case a more in-depth audit of the use of
> >
> > Yep...
> >
> >> total_length and offset should be done.  Perhaps a compromise would be
> >
> > That would be perfect.
> >
> >> a comment saying "FIXME: Verify that grub does not crash for any value
> >> of total_length, offset, and sector_size combination."
> >
> > OK but I would be more happy if you add to the compromise a promise that
> > you will continue the work on the functional testing mentioned above... :-)
>
> Hmm, let me look into it before making a promise. The part with the
> most work will be adding the ability to create LUKS2 devices that
> cryptsetup does not currently allow (eg. One with a zero length
> segment or something grub_strtoull() will error on).

Could not you create correct image and break it later by overwriting
some parts of it?

> >> Honestly, I'm frustrated at how much time this whole patch series is
> >> requiring of me and dragging on. I feel like this patch is being held
> >
> > This is partially by my lack of time. However, I hope it will be
> > changing. Anyway, sorry about the delays on my side.
> >
> >> hostage in order to strong-arm me in to fixing something unrelated to
> >> my patch.
> >
> > I think it is related to some extend. Anyway, I am open to discuss any
> > solution to this issue except ignoring it. Though I think we are close
> > to the compromise... :-)
>
> I think a good compromise would be to error on segments where offset >
> segment.size and crypt->total_length == 0. And to add a  fixme comment
> to handle the overflow case for grub_strtoull() better. Overflow won't
> cause a crash, just the area larger than the overflow amount to be
> inaccessible. And I don't think we need the previously mentioned
> fixme, but I'm not opposed to adding it.

Make sense for me.

Daniel

Re: [PATCH v3 4/9] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors

[Glenn Washburn]

Nov 4, 2020 7:15:30 AM Daniel Kiper <dkiper@net-space.pl>:

> On Tue, Nov 03, 2020 at 08:21:15PM +0000, Glenn Washburn wrote:
>> Oct 30, 2020 7:50:08 AM Daniel Kiper <dkiper@net-space.pl>:
>>> On Thu, Oct 29, 2020 at 02:53:34PM -0500, Glenn Washburn wrote:
>>>> On Tue, 27 Oct 2020 20:11:56 +0100
>>>> Daniel Kiper <dkiper@net-space.pl> wrote:
>>>>> On Sat, Oct 03, 2020 at 12:42:55AM -0500, Glenn Washburn wrote:
>>>>>> On Mon, 21 Sep 2020 13:23:04 +0200
>>>>>> Daniel Kiper <daniel.kiper@oracle.com> wrote:
>>>>>>> On Mon, Sep 21, 2020 at 06:28:28AM +0000, Glenn Washburn wrote:
>>>>>>>> Sep 8, 2020 7:21:31 AM Daniel Kiper <daniel.kiper@oracle.com>:
>>>>>>>>> On Mon, Sep 07, 2020 at 05:27:46PM +0200, Patrick Steinhardt
>>>>>>>>> wrote:
>
> [...]
>
>>>>> What is the worst scenario if somebody plays bad games with
>>>>> segment.size string? If nothing dangerous happens I am OK with the
>>>>> comment explaining why it is safe to ignore grub_strtoull() errors
>>>>> here.
>>>>
>>>> I think part of my pushback on this is I don't see a good solution. How
>>>
>>> OK...
>>>
>>>> do you know when grub_strtoull() errors here?  And even if it doesn't
>>>
>>> Just check values/errors returned by it?
>>
>> There are two error values returned by grub_strtoull(), 0 and ~0ULL
>> for unrecognized number and overflow. However, these are _both_ valid
>> non-error return values. So was it an overflow condition or a valid
>> return when ~0ULL is returned? Same for 0. In the case of 0 while it
>> may be valid, it wouldn't reflect a usable segment, so we can filter
>> out those.
>
> That is why you should check grub_errno too. In general "man strtoull"
> is your friend. However, I agree that this does not prevent against
> overflows later. So, I think we should have error checks for
> grub_strtoull() to detect incorrect input and further some other checks
> for overflows in math if needed. Potentially we can use only the latter
> if we put these protection properly.

Normally you only check errno after a function that sets it fails. In this case, we know it sets it, but we don't know it fails. The problem with checking grub_errno when grub_strtoull() has not failed is that grub_errno can be non-zero from being by a failing function prior to grub_strtoull(). I see now that man strtoull suggests setting errno=0 before calling strtoull(). Wrapping the call in a grub_error_push/pop makes better sense here, so we can preserve any previous errors.

As an aside, grub_strtoull() doesn't handle strings prefixed by '-', while strtoull does. I think grub's makes more sense and suits the current purpose better.

>>>> error, how do you know that a segment.size of 3 or 128 won't cause a
>>>> crash? I don't have good answers to these questions.  If grub does
>>>
>>> This question is more difficult and I am afraid that you are right.
>>>
>>>> crash, then a bug has been exposed in grub which should be fixed.
>>>
>>> If possible we should prevent against the crashes but I am also aware
>>> that it is not always feasible to predict when the GRUB will crash.
>>
>> It would be good to detect where grub is crashing because there might
>> be other ways to trigger such a crash (perhaps through loopback?)
>
> Yeah, but I think we get back to tests here...
>
> [...]
>
>>>> Since you seem to have a clear idea of what should be done here,
>>>> perhaps you insert a patch to your liking?  Or just tell me exactly
>>>> what you think should be done to protect against crashes.  I can just
>>>> add a zero check if that's what you want.  But that just adding a
>>>> "check" to check a box and make people feel safe and comfortable that
>>>
>>> I am not interested in adding `"check" to check a box`...
>>>
>>>> something is being done, when in fact it may do little to fix a
>>>> potential crash.  When you say "somebody feeds it with invalid data", I
>>>> take you to be concerned about someone maliciously crafting data to
>>>> exploit grub, in which case a more in-depth audit of the use of
>>>
>>> Yep...
>>>
>>>> total_length and offset should be done.  Perhaps a compromise would be
>>>
>>> That would be perfect.
>>>
>>>> a comment saying "FIXME: Verify that grub does not crash for any value
>>>> of total_length, offset, and sector_size combination."
>>>
>>> OK but I would be more happy if you add to the compromise a promise that
>>> you will continue the work on the functional testing mentioned above... :-)
>>
>> Hmm, let me look into it before making a promise. The part with the
>> most work will be adding the ability to create LUKS2 devices that
>> cryptsetup does not currently allow (eg. One with a zero length
>> segment or something grub_strtoull() will error on).
>
> Could not you create correct image and break it later by overwriting
> some parts of it?

Yes, and I've written python code to do that. But it's not trivial because the header has a checksum and the part we'd be modifying is json. Grub doesn't actually verify the checksum, so we probably don't need to change it. But we will want a json parser. Since the build already requires python, I suppose there's no harm in having it as a dependency of tests too.

>>>> Honestly, I'm frustrated at how much time this whole patch series is
>>>> requiring of me and dragging on. I feel like this patch is being held
>>>
>>> This is partially by my lack of time. However, I hope it will be
>>> changing. Anyway, sorry about the delays on my side.
>>>
>>>> hostage in order to strong-arm me in to fixing something unrelated to
>>>> my patch.
>>>
>>> I think it is related to some extend. Anyway, I am open to discuss any
>>> solution to this issue except ignoring it. Though I think we are close
>>> to the compromise... :-)
>>
>> I think a good compromise would be to error on segments where offset >
>> segment.size and crypt->total_length == 0. And to add a  fixme comment
>> to handle the overflow case for grub_strtoull() better. Overflow won't
>> cause a crash, just the area larger than the overflow amount to be
>> inaccessible. And I don't think we need the previously mentioned
>> fixme, but I'm not opposed to adding it.
>
> Make sense for me.

Ok, I'll combine this with grub_error_push/pop

Glenn

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