[PATCH 00/17] Fixes and improvements for cryptodisks+luks2 and a few other things.

[PATCH 00/17] Fixes and improvements for cryptodisks+luks2 and a few other things.

[development]

From: Glenn Washburn <development@efficientek.com>

Hi All,

Here's a list of patches that mostly have to do with cryptodisk and luks2 fixes
or improvements. The odd balls out are the patches adding the ubuntu font path,
a fix an an improvement to blocklists, and adding a procfs entry for loopbacks.
I'm hoping that at least the luks2 and cryptodisk sector size fixes can be
included for the 2.06 release (non-512 byte sectors are definitely broken for
LUKS2). Let me know if there's anything else I can do to help get these merged
or messed something up in sending the emails (first time sending patches through
git).

Thanks for all the hard work,
Glenn

Glenn Washburn (17):
  configure: Add Ubuntu dejavu font path.
  cryptodisk: Fix cipher IV mode 'plain64' always being set as 'plain'.
  cryptodisk: Incorrect calculation of start sector for grub_disk_read
    in grub_cryptodisk_read.
  cryptodisk: Add more verbosity when reading/writing cryptodisks.
  luks: Add support for LUKS2 in (proc)/luks_script
  luks2: grub_cryptodisk_t->total_length is the max number of device
    native sectors.
  cryptodisk,luks: Allow special processing for comparing UUIDs.
  cryptodisk: Unregister cryptomount command when removing module.
  fs: When checking if a block list goes past the end of the disk, make
    sure the total size of the disk is in grub native sector sizes,
    otherwise there will be blocks at the end of the disk unaccessible
    by block lists.
  cryptodisk: Properly handle non-512 byte sized sectors.
  cryptodisk: Rename total_length field in grub_cryptodisk_t to
    total_sectors.
  fs: Allow number of blocks in block list to be optional, where length
    will be defaulted to the length of the device.
  loopback: Add procfs entry 'loopbacks' to output configured loopback
    devices.
  cryptodisk: Add header line to procfs entry and crypto and source
    device names.
  cryptodisk: Add a couple comments noting the usage of a couple fields
    in grub_cryptodisk_t as is done for grub_disk_t.
  luks2: Ensure that bit fields of grub_luks2_digest_t in
    luks2_parse_digest are initialized before returning.
  luks2: Fix use of incorrect index and some error messages.

 configure.ac                |   2 +-
 grub-core/disk/cryptodisk.c | 111 +++++++++++++++++++++++-------------
 grub-core/disk/geli.c       |   2 +-
 grub-core/disk/loopback.c   |  56 ++++++++++++++++++
 grub-core/disk/luks.c       |  27 +++------
 grub-core/disk/luks2.c      |  31 ++++++----
 grub-core/kern/fs.c         |  10 +++-
 include/grub/cryptodisk.h   |  10 +++-
 include/grub/misc.h         |  21 +++++++
 9 files changed, 192 insertions(+), 78 deletions(-)

-- 
2.25.1

[PATCH 01/17] configure: Add Ubuntu dejavu font path.

[development]

From: Glenn Washburn <development@efficientek.com>

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 configure.ac | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 7c10a4db7..c6b0ef499 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1678,7 +1678,7 @@ fi
 
 if test x"$starfield_excuse" = x; then
    for ext in pcf pcf.gz bdf bdf.gz ttf ttf.gz; do
-     for dir in . /usr/src /usr/share/fonts/X11/misc /usr/share/fonts/truetype/ttf-dejavu /usr/share/fonts/dejavu /usr/share/fonts/truetype; do
+     for dir in . /usr/src /usr/share/fonts/X11/misc /usr/share/fonts/truetype/ttf-dejavu /usr/share/fonts/dejavu /usr/share/fonts/truetype /usr/share/fonts/truetype/dejavu; do
         if test -f "$dir/DejaVuSans.$ext"; then
           DJVU_FONT_SOURCE="$dir/DejaVuSans.$ext"
           break 2
-- 
2.25.1

Re: [PATCH 01/17] configure: Add Ubuntu dejavu font path.

[David Michael]

On Wed, Jul 29, 2020 at 5:52 PM <development@efficientek.com> wrote:
> From: Glenn Washburn <development@efficientek.com>
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  configure.ac | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/configure.ac b/configure.ac
> index 7c10a4db7..c6b0ef499 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1678,7 +1678,7 @@ fi
>
>  if test x"$starfield_excuse" = x; then
>     for ext in pcf pcf.gz bdf bdf.gz ttf ttf.gz; do
> -     for dir in . /usr/src /usr/share/fonts/X11/misc /usr/share/fonts/truetype/ttf-dejavu /usr/share/fonts/dejavu /usr/share/fonts/truetype; do
> +     for dir in . /usr/src /usr/share/fonts/X11/misc /usr/share/fonts/truetype/ttf-dejavu /usr/share/fonts/dejavu /usr/share/fonts/truetype /usr/share/fonts/truetype/dejavu; do

I noticed that the font file isn't found on Fedora 32 anymore either.
This path needs to be added for that to work:
/usr/share/fonts/dejavu-sans-fonts

Maybe it could take a configure argument or environment variable to
handle all the different installation locations so the list doesn't
need to keep growing?

Thanks.

David

Re: [PATCH 01/17] configure: Add Ubuntu dejavu font path.

[Mike Gilbert]

On Wed, Jul 29, 2020 at 6:08 PM David Michael <fedora.dm0@gmail.com> wrote:
>
> On Wed, Jul 29, 2020 at 5:52 PM <development@efficientek.com> wrote:
> > From: Glenn Washburn <development@efficientek.com>
> >
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  configure.ac | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/configure.ac b/configure.ac
> > index 7c10a4db7..c6b0ef499 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -1678,7 +1678,7 @@ fi
> >
> >  if test x"$starfield_excuse" = x; then
> >     for ext in pcf pcf.gz bdf bdf.gz ttf ttf.gz; do
> > -     for dir in . /usr/src /usr/share/fonts/X11/misc /usr/share/fonts/truetype/ttf-dejavu /usr/share/fonts/dejavu /usr/share/fonts/truetype; do
> > +     for dir in . /usr/src /usr/share/fonts/X11/misc /usr/share/fonts/truetype/ttf-dejavu /usr/share/fonts/dejavu /usr/share/fonts/truetype /usr/share/fonts/truetype/dejavu; do
>
> I noticed that the font file isn't found on Fedora 32 anymore either.
> This path needs to be added for that to work:
> /usr/share/fonts/dejavu-sans-fonts
>
> Maybe it could take a configure argument or environment variable to
> handle all the different installation locations so the list doesn't
> need to keep growing?

A workaround is to copy/symlink the font file into the build
directory, where it will get picked up by the first item in the list
(".").

Also, Gentoo ebuild fetches a private copy of the fonts to avoid
adding a build dependency.

[PATCH 02/17] cryptodisk: Fix cipher IV mode 'plain64' always being set as 'plain'.

[development]

From: Glenn Washburn <development@efficientek.com>

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 1897acc4b..d8f66e9ef 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.25.1

[PATCH 03/17] cryptodisk: Incorrect calculation of start sector for grub_disk_read in grub_cryptodisk_read.

[development]

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 d8f66e9ef..2791a4870 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.25.1

[PATCH 04/17] cryptodisk: Add more verbosity when reading/writing cryptodisks.

[development]

From: Glenn Washburn <development@efficientek.com>

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

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 2791a4870..c21be7d52 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -753,8 +753,10 @@ grub_cryptodisk_read (grub_disk_t disk, grub_disk_addr_t sector,
 
   grub_dprintf ("cryptodisk",
 		"Reading %" PRIuGRUB_SIZE " sectors from sector 0x%"
-		PRIxGRUB_UINT64_T " with offset of %" PRIuGRUB_UINT64_T "\n",
-		size, sector, dev->offset);
+		PRIxGRUB_UINT64_T " with offset of %" PRIuGRUB_UINT64_T
+		" sectors and sector size of %u on disk (%s)\n",
+		size, sector, dev->offset, 1U << disk->log_sector_size,
+		dev->source_disk->name);
 
   err = grub_disk_read (dev->source_disk,
 			((sector + dev->offset) << (disk->log_sector_size
@@ -803,8 +805,10 @@ grub_cryptodisk_write (grub_disk_t disk, grub_disk_addr_t sector,
 
   grub_dprintf ("cryptodisk",
 		"Writing %" PRIuGRUB_SIZE " sectors to sector 0x%"
-		PRIxGRUB_UINT64_T " with offset of %" PRIuGRUB_UINT64_T "\n",
-		size, sector, dev->offset);
+		PRIxGRUB_UINT64_T " with offset of %" PRIuGRUB_UINT64_T
+		" sectors and sector size of %u on disk (%s)\n",
+		size, sector, dev->offset, 1U << disk->log_sector_size,
+		dev->source_disk->name);
 
   gcry_err = grub_cryptodisk_endecrypt (dev, (grub_uint8_t *) tmp,
 					size << disk->log_sector_size,
-- 
2.25.1

[PATCH 05/17] luks: Add support for LUKS2 in (proc)/luks_script

[development]

From: Glenn Washburn <development@efficientek.com>

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

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index c21be7d52..f6b6302e1 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1210,12 +1210,14 @@ luks_script_get (grub_size_t *sz)
   *sz = 0;
 
   for (i = cryptodisk_list; i != NULL; i = i->next)
-    if (grub_strcmp (i->modname, "luks") == 0)
+    if (grub_strncmp (i->modname, "luks", 4) == 0)
       {
-	size += sizeof ("luks_mount ");
+	size += grub_strlen (i->modname);
+	size += sizeof ("_mount");
 	size += grub_strlen (i->uuid);
 	size += grub_strlen (i->cipher->cipher->name);
-	size += 54;
+	/* mode + mode_iv + spaces + offset + sector size + ??? + '\n' + NULL */
+	size += 5 + 8 + 5 + 20 + 4 + 16 + 1 + 1;
 	if (i->essiv_hash)
 	  size += grub_strlen (i->essiv_hash->name);
 	size += i->keysize * 2;
@@ -1228,16 +1230,16 @@ luks_script_get (grub_size_t *sz)
   ptr = ret;
 
   for (i = cryptodisk_list; i != NULL; i = i->next)
-    if (grub_strcmp (i->modname, "luks") == 0)
+    if (grub_strncmp (i->modname, "luks", 4) == 0)
       {
 	unsigned j;
 	const char *iptr;
-	ptr = grub_stpcpy (ptr, "luks_mount ");
+	ptr = grub_stpcpy (ptr, i->modname);
+	ptr = grub_stpcpy (ptr, "_mount ");
 	ptr = grub_stpcpy (ptr, i->uuid);
 	*ptr++ = ' ';
-	grub_snprintf (ptr, 21, "%" PRIuGRUB_UINT64_T " ", i->offset);
-	while (*ptr)
-	  ptr++;
+	ptr += grub_snprintf (ptr, 21, "%" PRIuGRUB_UINT64_T " ", i->offset);
+	ptr += grub_snprintf (ptr, 6, "%d ", 1 << i->log_sector_size);
 	for (iptr = i->cipher->cipher->name; *iptr; iptr++)
 	  *ptr++ = grub_tolower (*iptr);
 	switch (i->mode)
-- 
2.25.1

Re: [PATCH 05/17] luks: Add support for LUKS2 in (proc)/luks_script

[Patrick Steinhardt]

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

On Wed, Jul 29, 2020 at 04:50:10PM -0500, development@efficientek.com wrote:
> From: Glenn Washburn <development@efficientek.com>
> 
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/disk/cryptodisk.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index c21be7d52..f6b6302e1 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -1210,12 +1210,14 @@ luks_script_get (grub_size_t *sz)
>    *sz = 0;
>  
>    for (i = cryptodisk_list; i != NULL; i = i->next)
> -    if (grub_strcmp (i->modname, "luks") == 0)
> +    if (grub_strncmp (i->modname, "luks", 4) == 0)

While it works, I think it's a bit too magical. I'd personally prefer to
be a bit more verbose:

    if (grub_strcmp (i->modname, "luks") == 0 ||
        grub_strcmp (i->modname, "luks2") == 0)
      {
        ...
      }

>        {
> -	size += sizeof ("luks_mount ");
> +	size += grub_strlen (i->modname);
> +	size += sizeof ("_mount");
>  	size += grub_strlen (i->uuid);
>  	size += grub_strlen (i->cipher->cipher->name);
> -	size += 54;
> +	/* mode + mode_iv + spaces + offset + sector size + ??? + '\n' + NULL */
> +	size += 5 + 8 + 5 + 20 + 4 + 16 + 1 + 1;

Is it expected that the `size` is now bigger than before? This adds up
to `60` now. It's fine as it is more verbose than it previously has
been, but a comment in the commit message explaining that the different
size is intentional would've helped.

>  	if (i->essiv_hash)
>  	  size += grub_strlen (i->essiv_hash->name);
>  	size += i->keysize * 2;
> @@ -1228,16 +1230,16 @@ luks_script_get (grub_size_t *sz)
>    ptr = ret;
>  
>    for (i = cryptodisk_list; i != NULL; i = i->next)
> -    if (grub_strcmp (i->modname, "luks") == 0)
> +    if (grub_strncmp (i->modname, "luks", 4) == 0)

Same remark as above.

>        {
>  	unsigned j;
>  	const char *iptr;
> -	ptr = grub_stpcpy (ptr, "luks_mount ");
> +	ptr = grub_stpcpy (ptr, i->modname);
> +	ptr = grub_stpcpy (ptr, "_mount ");
>  	ptr = grub_stpcpy (ptr, i->uuid);
>  	*ptr++ = ' ';
> -	grub_snprintf (ptr, 21, "%" PRIuGRUB_UINT64_T " ", i->offset);
> -	while (*ptr)
> -	  ptr++;
> +	ptr += grub_snprintf (ptr, 21, "%" PRIuGRUB_UINT64_T " ", i->offset);
> +	ptr += grub_snprintf (ptr, 6, "%d ", 1 << i->log_sector_size);
>  	for (iptr = i->cipher->cipher->name; *iptr; iptr++)
>  	  *ptr++ = grub_tolower (*iptr);
>  	switch (i->mode)
> -- 
> 2.25.1
> 

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

Re: [PATCH 05/17] luks: Add support for LUKS2 in (proc)/luks_script

[Glenn Washburn]

On Thu, 30 Jul 2020 17:14:54 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> >        {
> > -	size += sizeof ("luks_mount ");
> > +	size += grub_strlen (i->modname);
> > +	size += sizeof ("_mount");
> >  	size += grub_strlen (i->uuid);
> >  	size += grub_strlen (i->cipher->cipher->name);
> > -	size += 54;
> > +	/* mode + mode_iv + spaces + offset + sector size + ??? +
> > '\n' + NULL */
> > +	size += 5 + 8 + 5 + 20 + 4 + 16 + 1 + 1;
> 
> Is it expected that the `size` is now bigger than before? This adds up
> to `60` now. It's fine as it is more verbose than it previously has
> been, but a comment in the commit message explaining that the
> different size is intentional would've helped.

Yes it the size is expected to be bigger, I also added a "sector size"
field, which is at most 4 digits and a space.  I added an extra byte
for NULL, but later realized I don't need that and that later commit is
in patch 14.  That should account for the 6 extra bytes.  I'm still not
sure why there's an extra 16 bytes unaccounted for, but I've left it
in.  The requested changes will be in the forth coming patchset.

> >  	if (i->essiv_hash)
> >  	  size += grub_strlen (i->essiv_hash->name);
> >  	size += i->keysize * 2;

[PATCH 06/17] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors.

[development]

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 e3ff7c83d..632309e3c 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.25.1

Re: [PATCH 06/17] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors.

[Patrick Steinhardt]

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

On Wed, Jul 29, 2020 at 04:50:11PM -0500, development@efficientek.com 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
> 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 e3ff7c83d..632309e3c 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;

Oops, thanks for catching this. Could you maybe add a comment wrt to the
magic going on with `(crypt->log_sector_size - disk->log_sector_size)`?
I didn't think too hard about it and am in a hurry, but the conversion
isn't that obvious to me.

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

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

Re: [PATCH 06/17] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors.

[Glenn Washburn]

On Thu, 30 Jul 2020 17:21:16 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> On Wed, Jul 29, 2020 at 04:50:11PM -0500, development@efficientek.com
> 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 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 e3ff7c83d..632309e3c 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;
> 
> Oops, thanks for catching this. Could you maybe add a comment wrt to
> the magic going on with `(crypt->log_sector_size -
> disk->log_sector_size)`? I didn't think too hard about it and am in a
> hurry, but the conversion isn't that obvious to me.

I'm adding an extra patch to the patch set to change the name of "disk"
to "source" like it is in luks.c for clarity.  Since we're getting the
size of the source disk in source disk sector sizes, we need to convert
it to cryptodisk sector sizes, which is what offset and total_length
are in.  I'll add a comment.

[PATCH 07/17] cryptodisk, luks: Allow special processing for comparing UUIDs.

[development]

From: Glenn Washburn <development@efficientek.com>

Create grub_uuidcasecmp to compare UUIDs in a case-insensitive manner and
that ignores '-' characters. This is backwards compatible with the old LUKS1
code that stored and compared against UUIDs without dashes. However, the new
LUKS2 code stores and compares UUIDs that contain dashes. Really, the UUID
comparison shouldn't care about the dashes, as this change implements. Now
your old scripts will continue to work with UUIDs without dashes, but you
may choose to use UUIDs with dashes now too for both LUKS1 and LUKS2.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/cryptodisk.c |  4 ++--
 grub-core/disk/luks.c       | 20 ++++----------------
 grub-core/disk/luks2.c      |  2 +-
 include/grub/misc.h         | 21 +++++++++++++++++++++
 4 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index f6b6302e1..f460ab838 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -660,7 +660,7 @@ grub_cryptodisk_open (const char *name, grub_disk_t disk)
   if (grub_memcmp (name, "cryptouuid/", sizeof ("cryptouuid/") - 1) == 0)
     {
       for (dev = cryptodisk_list; dev != NULL; dev = dev->next)
-	if (grub_strcasecmp (name + sizeof ("cryptouuid/") - 1, dev->uuid) == 0)
+	if (grub_uuidcasecmp (name + sizeof ("cryptouuid/") - 1, dev->uuid) == 0)
 	  break;
     }
   else
@@ -897,7 +897,7 @@ grub_cryptodisk_get_by_uuid (const char *uuid)
 {
   grub_cryptodisk_t dev;
   for (dev = cryptodisk_list; dev != NULL; dev = dev->next)
-    if (grub_strcasecmp (dev->uuid, uuid) == 0)
+    if (grub_uuidcasecmp (dev->uuid, uuid) == 0)
       return dev;
   return NULL;
 }
diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index 6ae162601..ea54a9d10 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -69,10 +69,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
 		   int check_boot)
 {
   grub_cryptodisk_t newdev;
-  const char *iptr;
   struct grub_luks_phdr header;
-  char *optr;
-  char uuid[sizeof (header.uuid) + 1];
   char ciphername[sizeof (header.cipherName) + 1];
   char ciphermode[sizeof (header.cipherMode) + 1];
   char hashspec[sizeof (header.hashSpec) + 1];
@@ -95,18 +92,9 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
       || grub_be_to_cpu16 (header.version) != 1)
     return NULL;
 
-  optr = uuid;
-  for (iptr = header.uuid; iptr < &header.uuid[ARRAY_SIZE (header.uuid)];
-       iptr++)
+  if (check_uuid && grub_uuidcasecmp (check_uuid, header.uuid) != 0)
     {
-      if (*iptr != '-')
-	*optr++ = *iptr;
-    }
-  *optr = 0;
-
-  if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0)
-    {
-      grub_dprintf ("luks", "%s != %s\n", uuid, check_uuid);
+      grub_dprintf ("luks", "%s != %s\n", header.uuid, check_uuid);
       return NULL;
     }
 
@@ -125,7 +113,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, header.uuid, sizeof (newdev->uuid));
   newdev->modname = "luks";
 
   /* Configure the hash used for the AF splitter and HMAC.  */
@@ -145,7 +133,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
       return NULL;
     }
 
-  COMPILE_TIME_ASSERT (sizeof (newdev->uuid) >= sizeof (uuid));
+  COMPILE_TIME_ASSERT (sizeof (newdev->uuid) >= sizeof (header.uuid));
   return newdev;
 }
 
diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 632309e3c..3c571b7fd 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -356,7 +356,7 @@ luks2_scan (grub_disk_t disk, const char *check_uuid, int check_boot)
       return NULL;
     }
 
-  if (check_uuid && grub_strcasecmp (check_uuid, header.uuid) != 0)
+  if (check_uuid && grub_uuidcasecmp (check_uuid, header.uuid) != 0)
     return NULL;
 
   cryptodisk = grub_zalloc (sizeof (*cryptodisk));
diff --git a/include/grub/misc.h b/include/grub/misc.h
index b7ca6dd58..3f0f42c22 100644
--- a/include/grub/misc.h
+++ b/include/grub/misc.h
@@ -243,6 +243,27 @@ grub_strncasecmp (const char *s1, const char *s2, grub_size_t n)
     - (int) grub_tolower ((grub_uint8_t) *s2);
 }
 
+static inline int
+grub_uuidcasecmp (const char *uuid1, const char *uuid2)
+{
+  while (*uuid1 && *uuid2)
+    {
+      while (*uuid1 == '-')
+        uuid1++;
+      while (*uuid2 == '-')
+        uuid2++;
+
+      if (grub_tolower (*uuid1) != grub_tolower (*uuid2))
+	break;
+
+      uuid1++;
+      uuid2++;
+    }
+
+  return (int) grub_tolower ((grub_uint8_t) *uuid1)
+    - (int) grub_tolower ((grub_uint8_t) *uuid2);
+}
+
 /*
  * Note that these differ from the C standard's definitions of strtol,
  * strtoul(), and strtoull() by the addition of two const qualifiers on the end
-- 
2.25.1

Re: [PATCH 07/17] cryptodisk,luks: Allow special processing for comparing UUIDs.

[Patrick Steinhardt]

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

On Wed, Jul 29, 2020 at 04:50:12PM -0500, development@efficientek.com wrote:
> From: Glenn Washburn <development@efficientek.com>
> 
> Create grub_uuidcasecmp to compare UUIDs in a case-insensitive manner and
> that ignores '-' characters. This is backwards compatible with the old LUKS1
> code that stored and compared against UUIDs without dashes. However, the new
> LUKS2 code stores and compares UUIDs that contain dashes. Really, the UUID
> comparison shouldn't care about the dashes, as this change implements. Now
> your old scripts will continue to work with UUIDs without dashes, but you
> may choose to use UUIDs with dashes now too for both LUKS1 and LUKS2.

FYI, this is crossing with my own patchset to implement probing support
for LUKS2 [1]. I do like that we start becoming agnostic of those dashes
now, though, as it really is kind of annoying to handle as a user.

1: https://lists.gnu.org/archive/html/grub-devel/2020-05/msg00235.html

> 
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/disk/cryptodisk.c |  4 ++--
>  grub-core/disk/luks.c       | 20 ++++----------------
>  grub-core/disk/luks2.c      |  2 +-
>  include/grub/misc.h         | 21 +++++++++++++++++++++
>  4 files changed, 28 insertions(+), 19 deletions(-)
> 
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index f6b6302e1..f460ab838 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -660,7 +660,7 @@ grub_cryptodisk_open (const char *name, grub_disk_t disk)
>    if (grub_memcmp (name, "cryptouuid/", sizeof ("cryptouuid/") - 1) == 0)
>      {
>        for (dev = cryptodisk_list; dev != NULL; dev = dev->next)
> -	if (grub_strcasecmp (name + sizeof ("cryptouuid/") - 1, dev->uuid) == 0)
> +	if (grub_uuidcasecmp (name + sizeof ("cryptouuid/") - 1, dev->uuid) == 0)
>  	  break;
>      }
>    else
> @@ -897,7 +897,7 @@ grub_cryptodisk_get_by_uuid (const char *uuid)
>  {
>    grub_cryptodisk_t dev;
>    for (dev = cryptodisk_list; dev != NULL; dev = dev->next)
> -    if (grub_strcasecmp (dev->uuid, uuid) == 0)
> +    if (grub_uuidcasecmp (dev->uuid, uuid) == 0)
>        return dev;
>    return NULL;
>  }
> diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> index 6ae162601..ea54a9d10 100644
> --- a/grub-core/disk/luks.c
> +++ b/grub-core/disk/luks.c
> @@ -69,10 +69,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
>  		   int check_boot)
>  {
>    grub_cryptodisk_t newdev;
> -  const char *iptr;
>    struct grub_luks_phdr header;
> -  char *optr;
> -  char uuid[sizeof (header.uuid) + 1];
>    char ciphername[sizeof (header.cipherName) + 1];
>    char ciphermode[sizeof (header.cipherMode) + 1];
>    char hashspec[sizeof (header.hashSpec) + 1];
> @@ -95,18 +92,9 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
>        || grub_be_to_cpu16 (header.version) != 1)
>      return NULL;
>  
> -  optr = uuid;
> -  for (iptr = header.uuid; iptr < &header.uuid[ARRAY_SIZE (header.uuid)];
> -       iptr++)
> +  if (check_uuid && grub_uuidcasecmp (check_uuid, header.uuid) != 0)
>      {
> -      if (*iptr != '-')
> -	*optr++ = *iptr;
> -    }
> -  *optr = 0;
> -
> -  if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0)
> -    {
> -      grub_dprintf ("luks", "%s != %s\n", uuid, check_uuid);
> +      grub_dprintf ("luks", "%s != %s\n", header.uuid, check_uuid);
>        return NULL;
>      }
>  
> @@ -125,7 +113,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, header.uuid, sizeof (newdev->uuid));
>    newdev->modname = "luks";
>  
>    /* Configure the hash used for the AF splitter and HMAC.  */
> @@ -145,7 +133,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
>        return NULL;
>      }
>  
> -  COMPILE_TIME_ASSERT (sizeof (newdev->uuid) >= sizeof (uuid));
> +  COMPILE_TIME_ASSERT (sizeof (newdev->uuid) >= sizeof (header.uuid));
>    return newdev;
>  }
>  
> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> index 632309e3c..3c571b7fd 100644
> --- a/grub-core/disk/luks2.c
> +++ b/grub-core/disk/luks2.c
> @@ -356,7 +356,7 @@ luks2_scan (grub_disk_t disk, const char *check_uuid, int check_boot)
>        return NULL;
>      }
>  
> -  if (check_uuid && grub_strcasecmp (check_uuid, header.uuid) != 0)
> +  if (check_uuid && grub_uuidcasecmp (check_uuid, header.uuid) != 0)
>      return NULL;
>  
>    cryptodisk = grub_zalloc (sizeof (*cryptodisk));
> diff --git a/include/grub/misc.h b/include/grub/misc.h
> index b7ca6dd58..3f0f42c22 100644
> --- a/include/grub/misc.h
> +++ b/include/grub/misc.h
> @@ -243,6 +243,27 @@ grub_strncasecmp (const char *s1, const char *s2, grub_size_t n)
>      - (int) grub_tolower ((grub_uint8_t) *s2);
>  }
>  
> +static inline int
> +grub_uuidcasecmp (const char *uuid1, const char *uuid2)
> +{
> +  while (*uuid1 && *uuid2)
> +    {
> +      while (*uuid1 == '-')
> +        uuid1++;
> +      while (*uuid2 == '-')
> +        uuid2++;
> +
> +      if (grub_tolower (*uuid1) != grub_tolower (*uuid2))
> +	break;
> +
> +      uuid1++;
> +      uuid2++;
> +    }
> +
> +  return (int) grub_tolower ((grub_uint8_t) *uuid1)
> +    - (int) grub_tolower ((grub_uint8_t) *uuid2);
> +}
> +
>  /*
>   * Note that these differ from the C standard's definitions of strtol,
>   * strtoul(), and strtoull() by the addition of two const qualifiers on the end
> -- 
> 2.25.1
> 

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

Re: [PATCH 07/17] cryptodisk,luks: Allow special processing for comparing UUIDs.

[Glenn Washburn]

On Thu, 30 Jul 2020 17:24:12 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> On Wed, Jul 29, 2020 at 04:50:12PM -0500, development@efficientek.com
> wrote:
> > From: Glenn Washburn <development@efficientek.com>
> > 
> > Create grub_uuidcasecmp to compare UUIDs in a case-insensitive
> > manner and that ignores '-' characters. This is backwards
> > compatible with the old LUKS1 code that stored and compared against
> > UUIDs without dashes. However, the new LUKS2 code stores and
> > compares UUIDs that contain dashes. Really, the UUID comparison
> > shouldn't care about the dashes, as this change implements. Now
> > your old scripts will continue to work with UUIDs without dashes,
> > but you may choose to use UUIDs with dashes now too for both LUKS1
> > and LUKS2.
> 
> FYI, this is crossing with my own patchset to implement probing
> support for LUKS2 [1]. I do like that we start becoming agnostic of
> those dashes now, though, as it really is kind of annoying to handle
> as a user.
> 
> 1: https://lists.gnu.org/archive/html/grub-devel/2020-05/msg00235.html

Yep, I saw your patch and like that it brings into alignment LUKS1 and
LUKS2 UUID handling.  Unfortunately, LUKS UUID handling is also
different than FS UUID handling, which uses dashes.  So not only is the
current behavior annoying, its also confusing.  I've been bitten by this
a couple times.  This change brings LUKS* UUID handling into alignment
with FS UUID handling and is backwards compatible with current LUKS
handling.  Since your patch set hasn't been committed, would you be
opposed to dropping that patch?

[PATCH 08/17] cryptodisk: Unregister cryptomount command when removing module.

[development]

From: Glenn Washburn <development@efficientek.com>

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 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 f460ab838..bc38687e4 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1317,5 +1317,6 @@ GRUB_MOD_FINI (cryptodisk)
 {
   grub_disk_dev_unregister (&grub_cryptodisk_dev);
   cryptodisk_cleanup ();
+  grub_unregister_extcmd (cmd);
   grub_procfs_unregister (&luks_script);
 }
-- 
2.25.1

[PATCH 09/17] fs: When checking if a block list goes past the end of the disk, make sure the total size of the disk is in grub native sector sizes, otherwise there will be blocks at the end of the disk unaccessible by block lists.

[development]

From: Glenn Washburn <development@efficientek.com>

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

diff --git a/grub-core/kern/fs.c b/grub-core/kern/fs.c
index fb30da9f4..14c17df74 100644
--- a/grub-core/kern/fs.c
+++ b/grub-core/kern/fs.c
@@ -139,6 +139,7 @@ grub_fs_blocklist_open (grub_file_t file, const char *name)
   unsigned i;
   grub_disk_t disk = file->device->disk;
   struct grub_fs_block *blocks;
+  grub_size_t total_native_sectors;
 
   /* First, count the number of blocks.  */
   do
@@ -156,6 +157,7 @@ grub_fs_blocklist_open (grub_file_t file, const char *name)
     return 0;
 
   file->size = 0;
+  total_native_sectors = disk->total_sectors << (disk->log_sector_size - GRUB_DISK_SECTOR_BITS);
   p = (char *) name;
   for (i = 0; i < num; i++)
     {
@@ -181,7 +183,7 @@ grub_fs_blocklist_open (grub_file_t file, const char *name)
 	  goto fail;
 	}
 
-      if (disk->total_sectors < blocks[i].offset + blocks[i].length)
+      if (total_native_sectors < blocks[i].offset + blocks[i].length)
 	{
 	  grub_error (GRUB_ERR_BAD_FILENAME, "beyond the total sectors");
 	  goto fail;
-- 
2.25.1

Re: [PATCH 09/17] fs: When checking if a block list goes past the end of the disk, make sure the total size of the disk is in grub native sector sizes, otherwise there will be blocks at the end of the disk unaccessible by block lists.

[Patrick Steinhardt]

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

On Wed, Jul 29, 2020 at 04:50:14PM -0500, development@efficientek.com wrote:
> From: Glenn Washburn <development@efficientek.com>

The commit message header is quite long. Could you please shorten it to
at most 80 characters and maybe put remaining info into the message
body?

> 
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/kern/fs.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/grub-core/kern/fs.c b/grub-core/kern/fs.c
> index fb30da9f4..14c17df74 100644
> --- a/grub-core/kern/fs.c
> +++ b/grub-core/kern/fs.c
> @@ -139,6 +139,7 @@ grub_fs_blocklist_open (grub_file_t file, const char *name)
>    unsigned i;
>    grub_disk_t disk = file->device->disk;
>    struct grub_fs_block *blocks;
> +  grub_size_t total_native_sectors;
>  
>    /* First, count the number of blocks.  */
>    do
> @@ -156,6 +157,7 @@ grub_fs_blocklist_open (grub_file_t file, const char *name)
>      return 0;
>  
>    file->size = 0;
> +  total_native_sectors = disk->total_sectors << (disk->log_sector_size - GRUB_DISK_SECTOR_BITS);
>    p = (char *) name;
>    for (i = 0; i < num; i++)
>      {
> @@ -181,7 +183,7 @@ grub_fs_blocklist_open (grub_file_t file, const char *name)
>  	  goto fail;
>  	}
>  
> -      if (disk->total_sectors < blocks[i].offset + blocks[i].length)
> +      if (total_native_sectors < blocks[i].offset + blocks[i].length)
>  	{
>  	  grub_error (GRUB_ERR_BAD_FILENAME, "beyond the total sectors");
>  	  goto fail;
> -- 
> 2.25.1
> 

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

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

[development]

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

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index bc38687e4..c30fb0e16 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,15 @@ 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[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 +317,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 +328,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 +343,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 +374,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 +387,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 +405,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
@@ -769,7 +775,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);
 }
 
@@ -812,7 +818,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 ea54a9d10..a1ac078d1 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
 
@@ -111,7 +112,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, header.uuid, sizeof (newdev->uuid));
   newdev->modname = "luks";
@@ -234,7 +235,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 3c571b7fd..1ba5044f6 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.25.1

[PATCH 11/17] cryptodisk: Rename total_length field in grub_cryptodisk_t to total_sectors.

[development]

From: Glenn Washburn <development@efficientek.com>

This makes the creates an alignment with grub_disk_t naming of the same
field and is more intuitive as to how it should be used.

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

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index c30fb0e16..fc53ab491 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -705,7 +705,7 @@ grub_cryptodisk_open (const char *name, grub_disk_t disk)
     }
 
   disk->data = dev;
-  disk->total_sectors = dev->total_length;
+  disk->total_sectors = dev->total_sectors;
   disk->max_agglomerate = GRUB_DISK_MAX_MAX_AGGLOMERATE;
   disk->id = dev->id;
   dev->ref++;
diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
index e9d23299a..4ec875821 100644
--- a/grub-core/disk/geli.c
+++ b/grub-core/disk/geli.c
@@ -391,7 +391,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
 
   newdev->modname = "geli";
 
-  newdev->total_length = grub_disk_get_size (disk) - 1;
+  newdev->total_sectors = grub_disk_get_size (disk) - 1;
   grub_memcpy (newdev->uuid, uuid, sizeof (newdev->uuid));
   COMPILE_TIME_ASSERT (sizeof (newdev->uuid) >= 32 * 2 + 1);
   return newdev;
diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index a1ac078d1..b1db3d137 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -113,7 +113,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
   newdev->offset = grub_be_to_cpu32 (header.payloadOffset);
   newdev->source_disk = NULL;
   newdev->log_sector_size = LOG_SECTOR_SIZE;
-  newdev->total_length = grub_disk_get_size (disk) - newdev->offset;
+  newdev->total_sectors = grub_disk_get_size (disk) - newdev->offset;
   grub_memcpy (newdev->uuid, header.uuid, sizeof (newdev->uuid));
   newdev->modname = "luks";
 
diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 1ba5044f6..0089d169c 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -606,10 +606,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->log_sector_size - disk->log_sector_size)) 
+	crypt->total_sectors = (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->log_sector_size;
+	crypt->total_sectors = 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));
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index 06653a622..f9e42796e 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -67,7 +67,7 @@ struct grub_cryptodisk
 
   char *source;
   grub_disk_addr_t offset;
-  grub_disk_addr_t total_length;
+  grub_disk_addr_t total_sectors;
   grub_disk_t source_disk;
   int ref;
   grub_crypto_cipher_handle_t cipher;
-- 
2.25.1

Re: [PATCH 11/17] cryptodisk: Rename total_length field in grub_cryptodisk_t to total_sectors.

[Patrick Steinhardt]

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

On Wed, Jul 29, 2020 at 04:50:16PM -0500, development@efficientek.com wrote:
> From: Glenn Washburn <development@efficientek.com>
> 
> This makes the creates an alignment with grub_disk_t naming of the same
> field and is more intuitive as to how it should be used.
> 
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/disk/cryptodisk.c | 2 +-
>  grub-core/disk/geli.c       | 2 +-
>  grub-core/disk/luks.c       | 2 +-
>  grub-core/disk/luks2.c      | 4 ++--
>  include/grub/cryptodisk.h   | 2 +-
>  5 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index c30fb0e16..fc53ab491 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -705,7 +705,7 @@ grub_cryptodisk_open (const char *name, grub_disk_t disk)
>      }
>  
>    disk->data = dev;
> -  disk->total_sectors = dev->total_length;
> +  disk->total_sectors = dev->total_sectors;
>    disk->max_agglomerate = GRUB_DISK_MAX_MAX_AGGLOMERATE;
>    disk->id = dev->id;
>    dev->ref++;
> diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
> index e9d23299a..4ec875821 100644
> --- a/grub-core/disk/geli.c
> +++ b/grub-core/disk/geli.c
> @@ -391,7 +391,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
>  
>    newdev->modname = "geli";
>  
> -  newdev->total_length = grub_disk_get_size (disk) - 1;
> +  newdev->total_sectors = grub_disk_get_size (disk) - 1;
>    grub_memcpy (newdev->uuid, uuid, sizeof (newdev->uuid));
>    COMPILE_TIME_ASSERT (sizeof (newdev->uuid) >= 32 * 2 + 1);
>    return newdev;
> diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> index a1ac078d1..b1db3d137 100644
> --- a/grub-core/disk/luks.c
> +++ b/grub-core/disk/luks.c
> @@ -113,7 +113,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
>    newdev->offset = grub_be_to_cpu32 (header.payloadOffset);
>    newdev->source_disk = NULL;
>    newdev->log_sector_size = LOG_SECTOR_SIZE;
> -  newdev->total_length = grub_disk_get_size (disk) - newdev->offset;
> +  newdev->total_sectors = grub_disk_get_size (disk) - newdev->offset;

Oh, yes please. Care to add in another patch that does the same for
offset? `offset_sectors` would also be much clearer to me.

>    grub_memcpy (newdev->uuid, header.uuid, sizeof (newdev->uuid));
>    newdev->modname = "luks";
>  
> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> index 1ba5044f6..0089d169c 100644
> --- a/grub-core/disk/luks2.c
> +++ b/grub-core/disk/luks2.c
> @@ -606,10 +606,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->log_sector_size - disk->log_sector_size)) 
> +	crypt->total_sectors = (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->log_sector_size;
> +	crypt->total_sectors = 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));
> diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> index 06653a622..f9e42796e 100644
> --- a/include/grub/cryptodisk.h
> +++ b/include/grub/cryptodisk.h
> @@ -67,7 +67,7 @@ struct grub_cryptodisk
>  
>    char *source;
>    grub_disk_addr_t offset;
> -  grub_disk_addr_t total_length;
> +  grub_disk_addr_t total_sectors;
>    grub_disk_t source_disk;
>    int ref;
>    grub_crypto_cipher_handle_t cipher;
> -- 
> 2.25.1
> 

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

Re: [PATCH 11/17] cryptodisk: Rename total_length field in grub_cryptodisk_t to total_sectors.

[Glenn Washburn]

On Thu, 30 Jul 2020 17:29:58 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> On Wed, Jul 29, 2020 at 04:50:16PM -0500, development@efficientek.com
> wrote:
> > From: Glenn Washburn <development@efficientek.com>
> > 
> > This makes the creates an alignment with grub_disk_t naming of the
> > same field and is more intuitive as to how it should be used.
> > 
> > Signed-off-by: Glenn Washburn <development@efficientek.com>

...

> > --- a/grub-core/disk/luks.c
> > +++ b/grub-core/disk/luks.c
> > @@ -113,7 +113,7 @@ configure_ciphers (grub_disk_t disk, const char
> > *check_uuid, newdev->offset = grub_be_to_cpu32
> > (header.payloadOffset); newdev->source_disk = NULL;
> >    newdev->log_sector_size = LOG_SECTOR_SIZE;
> > -  newdev->total_length = grub_disk_get_size (disk) -
> > newdev->offset;
> > +  newdev->total_sectors = grub_disk_get_size (disk) -
> > newdev->offset;
> 
> Oh, yes please. Care to add in another patch that does the same for
> offset? `offset_sectors` would also be much clearer to me.

Yep, I'll add an extra patch for that in the next patch set.

[PATCH 12/17] fs: Allow number of blocks in block list to be optional, where length will be defaulted to the length of the device.

[development]

From: Glenn Washburn <development@efficientek.com>

This is primarily useful to do something like "loopback newdev (dev)8+" to
create a device that skips the first 4K, which may contain a non-standard
RAID1 header that grub does not recognize. This would allow that initial
data to be accessed and potentially mounted by grub up to the rest of the
disk. There is currently not a good way with in grub to programmatically get
the number of sectors on a disk to select the appropriate number of sectors.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/kern/fs.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/grub-core/kern/fs.c b/grub-core/kern/fs.c
index 14c17df74..273115b5a 100644
--- a/grub-core/kern/fs.c
+++ b/grub-core/kern/fs.c
@@ -173,7 +173,11 @@ grub_fs_blocklist_open (grub_file_t file, const char *name)
 	}
 
       p++;
-      blocks[i].length = grub_strtoul (p, &p, 0);
+      if (*p == '\0' || *p == ',')
+        blocks[i].length = total_native_sectors - blocks[i].offset;
+      else
+        blocks[i].length = grub_strtoul (p, &p, 0);
+
       if (grub_errno != GRUB_ERR_NONE
 	  || blocks[i].length == 0
 	  || (*p && *p != ',' && ! grub_isspace (*p)))
-- 
2.25.1

[PATCH 13/17] loopback: Add procfs entry 'loopbacks' to output configured loopback devices.

[development]

From: Glenn Washburn <development@efficientek.com>

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

diff --git a/grub-core/disk/loopback.c b/grub-core/disk/loopback.c
index cdf9123fa..6a2be257b 100644
--- a/grub-core/disk/loopback.c
+++ b/grub-core/disk/loopback.c
@@ -21,6 +21,7 @@
 #include <grub/misc.h>
 #include <grub/file.h>
 #include <grub/disk.h>
+#include <grub/procfs.h>
 #include <grub/mm.h>
 #include <grub/extcmd.h>
 #include <grub/i18n.h>
@@ -229,6 +230,60 @@ static struct grub_disk_dev grub_loopback_dev =
     .next = 0
   };
 
+
+/* Open a file named NAME and initialize FILE.  */
+#define STR(s) #s
+#define MAX_ID_PRINT 10000
+static char *
+loopbacks_get (grub_size_t *sz)
+{
+  struct grub_loopback *i;
+  grub_size_t size = 0;
+  char *ptr, *ret;
+  const char header[] = N_("<id> <devname> <filename>\n");
+  static char errmsg[] = N_("Can not list more than " STR(MAX_ID_PRINT)
+			    " loopback devices.\n");
+
+  for (i = loopback_list; i != NULL; i = i->next)
+    if (i->id < MAX_ID_PRINT)
+      {
+	/* id + spaces + '\n' */
+	size += sizeof (STR(MAX_ID_PRINT)) + 2 + 1;
+	size += grub_strlen (i->devname);
+	size += grub_strlen (i->file->name);
+      }
+    else
+      {
+	*sz = sizeof (errmsg);
+	return errmsg;
+      }
+
+  ret = grub_malloc (sizeof (header) + size + 1);
+  if (!ret)
+    return 0;
+
+  ptr = grub_stpcpy (ret, header);
+
+  for (i = loopback_list; i != NULL; i = i->next)
+    {
+      ptr += grub_snprintf (ptr, 21, "%lu ", i->id);
+      ptr = grub_stpcpy (ptr, i->devname);
+      *ptr++ = ' ';
+      ptr = grub_stpcpy (ptr, i->file->name);
+      *ptr++ = '\n';
+    }
+  *ptr = '\0';
+  *sz = ptr - ret;
+  return ret;
+}
+
+struct grub_procfs_entry loopbacks =
+{
+  .name = "loopbacks",
+  .get_contents = loopbacks_get
+};
+
+
 static grub_extcmd_t cmd;
 
 GRUB_MOD_INIT(loopback)
@@ -239,6 +294,7 @@ GRUB_MOD_INIT(loopback)
 				 or transformed into drive.  */
 			      N_("Make a virtual drive from a file."), options);
   grub_disk_dev_register (&grub_loopback_dev);
+  grub_procfs_register ("loopbacks", &loopbacks);
 }
 
 GRUB_MOD_FINI(loopback)
-- 
2.25.1

[PATCH 14/17] cryptodisk: Add header line to procfs entry and crypto and source device names.

[development]

From: Glenn Washburn <development@efficientek.com>

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

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index fc53ab491..acf87b6c8 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1206,34 +1206,47 @@ hex (grub_uint8_t val)
 }
 
 /* Open a file named NAME and initialize FILE.  */
+#define STR(s) #s
+#define MAX_ID_PRINT 10000
 static char *
 luks_script_get (grub_size_t *sz)
 {
   grub_cryptodisk_t i;
   grub_size_t size = 0;
   char *ptr, *ret;
+  const char header[] = N_("<type> <devname> <source disk> <uuid> <sector "
+			   "offset> <sector size> <cipher> <key> <options>\n");
+  static char errmsg[] = N_("Can not list more than " STR(MAX_ID_PRINT)
+			    " crypto devices.\n");
 
   *sz = 0;
 
   for (i = cryptodisk_list; i != NULL; i = i->next)
-    if (grub_strncmp (i->modname, "luks", 4) == 0)
+    if (i->id >= MAX_ID_PRINT)
+      {
+	*sz = sizeof (errmsg);
+	return errmsg;
+      }
+    else if (grub_strncmp (i->modname, "luks", 4) == 0)
       {
 	size += grub_strlen (i->modname);
 	size += sizeof ("_mount");
+	size += sizeof ("crypto");
+	size += grub_strlen (i->source);
 	size += grub_strlen (i->uuid);
 	size += grub_strlen (i->cipher->cipher->name);
-	/* mode + mode_iv + spaces + offset + sector size + ??? + '\n' + NULL */
-	size += 5 + 8 + 5 + 20 + 4 + 16 + 1 + 1;
+	/* spaces + maxidlen + mode_iv + mode + offset + sector size + ??? + '\n' */
+	size += 4 + sizeof (STR(MAX_ID_PRINT)) + 8 + 5 + 20 + 4 + 16 + 1;
 	if (i->essiv_hash)
 	  size += grub_strlen (i->essiv_hash->name);
 	size += i->keysize * 2;
       }
 
-  ret = grub_malloc (size + 1);
+  ret = grub_malloc (sizeof (header) + size + 1);
   if (!ret)
     return 0;
 
-  ptr = ret;
+  ptr = grub_stpcpy (ret, header);
 
   for (i = cryptodisk_list; i != NULL; i = i->next)
     if (grub_strncmp (i->modname, "luks", 4) == 0)
@@ -1242,6 +1255,9 @@ luks_script_get (grub_size_t *sz)
 	const char *iptr;
 	ptr = grub_stpcpy (ptr, i->modname);
 	ptr = grub_stpcpy (ptr, "_mount ");
+	ptr += grub_snprintf (ptr, 8 + sizeof (STR(MAX_ID_PRINT)), "crypto%lu ", i->id);
+	ptr = grub_stpcpy (ptr, i->source);
+	*ptr++ = ' ';
 	ptr = grub_stpcpy (ptr, i->uuid);
 	*ptr++ = ' ';
 	ptr += grub_snprintf (ptr, 21, "%" PRIuGRUB_UINT64_T " ", i->offset);
-- 
2.25.1

[PATCH 15/17] cryptodisk: Add a couple comments noting the usage of a couple fields in grub_cryptodisk_t as is done for grub_disk_t.

[development]

From: Glenn Washburn <development@efficientek.com>

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 include/grub/cryptodisk.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index f9e42796e..8d3284aad 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -66,9 +66,15 @@ struct grub_cryptodisk
   struct grub_cryptodisk **prev;
 
   char *source;
+
+  /* The offset number of sectors of the encrypt data on the underlying disk
+   * where sectors are the size of this cryptodisk.
+   */
   grub_disk_addr_t offset;
+  /* Total number of encrypted sectors of size (1<<log_sector_size) */
   grub_disk_addr_t total_sectors;
   grub_disk_t source_disk;
+
   int ref;
   grub_crypto_cipher_handle_t cipher;
   grub_crypto_cipher_handle_t secondary_cipher;
-- 
2.25.1

[PATCH 16/17] luks2: Ensure that bit fields of grub_luks2_digest_t in luks2_parse_digest are initialized before returning.

[development]

From: Glenn Washburn <development@efficientek.com>

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

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 0089d169c..44a73d2b8 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -230,6 +230,7 @@ luks2_parse_digest (grub_luks2_digest_t *out, const grub_json_t *digest)
     return grub_error (GRUB_ERR_BAD_ARGUMENT,
 		       "Digest references no segments", type);
 
+  out->segments = 0;
   for (i = 0; i < size; i++)
     {
       if (grub_json_getchild (&o, &segments, i) ||
@@ -242,6 +243,7 @@ luks2_parse_digest (grub_luks2_digest_t *out, const grub_json_t *digest)
     return grub_error (GRUB_ERR_BAD_ARGUMENT,
 		       "Digest references no keyslots", type);
 
+  out->keyslots = 0;
   for (i = 0; i < size; i++)
     {
       if (grub_json_getchild (&o, &keyslots, i) ||
-- 
2.25.1

[PATCH 17/17] luks2: Fix use of incorrect index and some error messages.

[development]

From: Glenn Washburn <development@efficientek.com>

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 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 44a73d2b8..48600db68 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -277,34 +277,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.25.1

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

[Patrick Steinhardt]

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

On Wed, Jul 29, 2020 at 04:50:22PM -0500, development@efficientek.com wrote:
> From: Glenn Washburn <development@efficientek.com>
> 
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  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 44a73d2b8..48600db68 100644
> --- a/grub-core/disk/luks2.c
> +++ b/grub-core/disk/luks2.c
> @@ -277,34 +277,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);

These look correct to me.

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

Why do we reassign `i = j` here? I think it's to fix the "No segment for
digest" message below which previously referred to the wrong digest,
right? I think we should just stop using `i` and `j` here. If we renamed
them `keyslot_index` and `digest_index`, respectively, and only use `i`
as a loop variable, then the code would be much easier to read.

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

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

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

[Glenn Washburn]

On Thu, 30 Jul 2020 22:22:41 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> On Wed, Jul 29, 2020 at 04:50:22PM -0500, development@efficientek.com
> wrote:
> > From: Glenn Washburn <development@efficientek.com>
> > 
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  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 44a73d2b8..48600db68 100644
> > --- a/grub-core/disk/luks2.c
> > +++ b/grub-core/disk/luks2.c
> > @@ -277,34 +277,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);
> 
> These look correct to me.

Are you saying that the original code or my changes look correct here?

> >    /* 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++)
> 
> Why do we reassign `i = j` here? I think it's to fix the "No segment
> for digest" message below which previously referred to the wrong
> digest, right? I think we should just stop using `i` and `j` here. If
> we renamed them `keyslot_index` and `digest_index`, respectively, and
> only use `i` as a loop variable, then the code would be much easier
> to read.

Yep, I was just trying to make minimal changes. I'll update the patch
set with these suggestions.

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