[CRYPTO-LUKS v1 00/19] Fixes and improvements for cryptodisks+luks2 and a few other things.

[CRYPTO-LUKS v1 00/19] Fixes and improvements for cryptodisks+luks2 and a few other things.

[Glenn Washburn]

I've changed the name of the patch series to make it more self-explanatory, but
still incremented the version number.  All of Patrick's suggestions have been
added in.  I've added 2 new patches, one suggested by Patrick for rename
cryptodisk's offset member to offset_sectors. And another to rename 'disk' to
'source' where appropriate in luks2.c.

Glenn Washburn (19):
  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.
  luks2: Add support for LUKS2 in (proc)/luks_script
  luks2: Rename source disk variabled named 'disk' to 'source' as in
    luks.c.
  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: Fix block lists not being able to address to end of disk
    sometimes.
  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,luks2: 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.
  cryptodisk: Rename offset in grub_cryptodisk_t to offset_sectors to
    improve readability.

 configure.ac                |   2 +-
 grub-core/disk/cryptodisk.c | 115 +++++++++++++++++++++++-------------
 grub-core/disk/geli.c       |   4 +-
 grub-core/disk/loopback.c   |  56 ++++++++++++++++++
 grub-core/disk/luks.c       |  29 +++------
 grub-core/disk/luks2.c      |  78 +++++++++++++-----------
 grub-core/kern/fs.c         |  10 +++-
 include/grub/cryptodisk.h   |  12 +++-
 include/grub/misc.h         |  21 +++++++
 9 files changed, 224 insertions(+), 103 deletions(-)

-- 
2.25.1

[CRYPTO-LUKS v1 01/19] configure: Add Ubuntu dejavu font path.

[Glenn Washburn]

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

[CRYPTO-LUKS v1 02/19] cryptodisk: Fix cipher IV mode 'plain64' always being set as 'plain'.

[Glenn Washburn]

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

[CRYPTO-LUKS v1 03/19] cryptodisk: Incorrect calculation of start sector for grub_disk_read in grub_cryptodisk_read.

[Glenn Washburn]

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

Re: [CRYPTO-LUKS v1 03/19] cryptodisk: Incorrect calculation of start sector for grub_disk_read in grub_cryptodisk_read.

[Patrick Steinhardt]

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

On Fri, Jul 31, 2020 at 07:01:44AM -0500, Glenn Washburn wrote:
> 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)
>      {

So for LUKS2, we initialize `crypt->offset` with `crypt->offset =
grub_divmod64 (segment.offset, segment.sector_size, NULL)` where
`segment.offset` is the offset in bytes and `segment.sector_size` is the
sector size. So yup, it's in sectors.

For LUKS1, we do `newdev->offset = grub_be_to_cpu32
(header.payloadOffset)`, which also is in sectors of 512 bytes.

So the fix does seem correct to me, but I think it's incomplete as we'd
have to do the same for `grub_cryptodisk_write`.

Out of curiosity: did you test these changes? The offset should always
be a positive number, except with a detached header. So wouldn't we
always have hit this bug if this behaviour was indeed wrong?

Patrick

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

Re: [CRYPTO-LUKS v1 03/19] cryptodisk: Incorrect calculation of start sector for grub_disk_read in grub_cryptodisk_read.

[Glenn Washburn]

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

> On Fri, Jul 31, 2020 at 07:01:44AM -0500, Glenn Washburn wrote:
> > 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)
> >      {
> 
> So for LUKS2, we initialize `crypt->offset` with `crypt->offset =
> grub_divmod64 (segment.offset, segment.sector_size, NULL)` where
> `segment.offset` is the offset in bytes and `segment.sector_size` is
> the sector size. So yup, it's in sectors.
> 
> For LUKS1, we do `newdev->offset = grub_be_to_cpu32
> (header.payloadOffset)`, which also is in sectors of 512 bytes.
> 
> So the fix does seem correct to me, but I think it's incomplete as
> we'd have to do the same for `grub_cryptodisk_write`.

Yep, good catch. I didn't even think to check for grub_cryptodisk_write
since I tend to think of grub as read-only.  I'm actually not sure how
to trigger a disk write in grub.  The only thing that comes to mind is
I think there's a way to set some partition flags.  

> Out of curiosity: did you test these changes?

Yes, these changes have definitely been tested.  In fact, I found these
bugs precisely because I was trying to get grub to decrypt my LUKS2
device which has a 4096 byte sector size.  Feeling like I wasn't
getting much traction on my patches, I wrote the CRYPTOMOUNT-TEST patch
series to allow independent verification of the bugs and my fixes.
There are a series of tests for block sizes 1024, 2048, and 4096 which
all are successful for me (all the sector size fixes are needed).

Now that I'm looking at grub_cryptodisk_read again, it looks like the
GRUB_UTIL part doesn't even take in to account the offset.  So that's
probably not working either.  I'm not exactly sure how to test that
code right yet either.

> The offset should always
> be a positive number, except with a detached header. So wouldn't we
> always have hit this bug if this behaviour was indeed wrong?

Yes, offset can only be zero in a detached header scenario.  The key
here to why this issue was never hit is because LUKS1 and grub
native disks have a hardcoded 512-byte sector size (ie.
disk->log_sector_size == GRUB_DISK_SECTOR_BITS == 9 ).  So
(disk->log_sector_size - GRUB_DISK_SECTOR_BITS) == 0 and x << 0 == x.
Effectively, the bit shift was always a noop and still is for 512-byte
sector cryptodisks, which is why the LUKS2 support also works in the
default sector-size case of 512-bytes.

Glenn

Re: [CRYPTO-LUKS v1 03/19] cryptodisk: Incorrect calculation of start sector for grub_disk_read in grub_cryptodisk_read.

[Patrick Steinhardt]

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

On Sun, Aug 23, 2020 at 11:31:46PM -0500, Glenn Washburn wrote:
> On Sun, 23 Aug 2020 12:39:03 +0200
> Patrick Steinhardt <ps@pks.im> wrote:
> 
> > On Fri, Jul 31, 2020 at 07:01:44AM -0500, Glenn Washburn wrote:
> > > 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)
> > >      {
> > 
> > So for LUKS2, we initialize `crypt->offset` with `crypt->offset =
> > grub_divmod64 (segment.offset, segment.sector_size, NULL)` where
> > `segment.offset` is the offset in bytes and `segment.sector_size` is
> > the sector size. So yup, it's in sectors.
> > 
> > For LUKS1, we do `newdev->offset = grub_be_to_cpu32
> > (header.payloadOffset)`, which also is in sectors of 512 bytes.
> > 
> > So the fix does seem correct to me, but I think it's incomplete as
> > we'd have to do the same for `grub_cryptodisk_write`.
> 
> Yep, good catch. I didn't even think to check for grub_cryptodisk_write
> since I tend to think of grub as read-only.  I'm actually not sure how
> to trigger a disk write in grub.  The only thing that comes to mind is
> I think there's a way to set some partition flags.  

I'd assume that things like the grub.env file get updated based e.g. on
the last chosen boot entry. Which would require support to write to
disk. But I honestly don't now and have never used write-functionality
in GRUB.

Do you want to update your patch to also fix the write part? I'd then
pull it into a v2 of my patch series for v2.06 fixes.

> > Out of curiosity: did you test these changes?
> 
> Yes, these changes have definitely been tested.  In fact, I found these
> bugs precisely because I was trying to get grub to decrypt my LUKS2
> device which has a 4096 byte sector size.  Feeling like I wasn't
> getting much traction on my patches, I wrote the CRYPTOMOUNT-TEST patch
> series to allow independent verification of the bugs and my fixes.
> There are a series of tests for block sizes 1024, 2048, and 4096 which
> all are successful for me (all the sector size fixes are needed).

I'll have to have a look at them. Improved testability is definitely
something I'm interested in, mostly because I found the manual
compile-boot-test cycle quite unwieldy, even with virtualization.

> Now that I'm looking at grub_cryptodisk_read again, it looks like the
> GRUB_UTIL part doesn't even take in to account the offset.  So that's
> probably not working either.  I'm not exactly sure how to test that
> code right yet either.
> 
> > The offset should always
> > be a positive number, except with a detached header. So wouldn't we
> > always have hit this bug if this behaviour was indeed wrong?
> 
> Yes, offset can only be zero in a detached header scenario.  The key
> here to why this issue was never hit is because LUKS1 and grub
> native disks have a hardcoded 512-byte sector size (ie.
> disk->log_sector_size == GRUB_DISK_SECTOR_BITS == 9 ).  So
> (disk->log_sector_size - GRUB_DISK_SECTOR_BITS) == 0 and x << 0 == x.
> Effectively, the bit shift was always a noop and still is for 512-byte
> sector cryptodisks, which is why the LUKS2 support also works in the
> default sector-size case of 512-bytes.

Cool, thanks a lot for explaining the issue in more depth!

Patrick

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

[CRYPTO-LUKS v2 03/19] 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 sector is correctly transformed into
native grub sector size, but then added to dev->offset which is not
transformed. Create function grub_disk_from_native_sector to do the native
disk sector to grub sector size conversion.

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

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index b87925ad3..b3610a1b6 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -761,10 +761,10 @@ grub_cryptodisk_read (grub_disk_t disk, grub_disk_addr_t sector,
 		PRIxGRUB_UINT64_T " with offset of %" PRIuGRUB_UINT64_T "\n",
 		size, sector, dev->offset);
 
+  sector = 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),
+			0, size << disk->log_sector_size, buf);
   if (err)
     {
       grub_dprintf ("cryptodisk", "grub_disk_read failed with error %d\n", err);
@@ -821,12 +821,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: [CRYPTO-LUKS v2 03/19] cryptodisk: Incorrect calculation of sector in grub_cryptodisk_read/write.

[Glenn Washburn]

Ignore this patch, I neglected to run my test until after I had sent
the patch.  My tests immediately found an error. Please see version v3.

On Mon, 24 Aug 2020 18:42:30 -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 sector is correctly
> transformed into native grub sector size, but then added to
> dev->offset which is not transformed. Create function
> grub_disk_from_native_sector to do the native disk sector to grub
> sector size conversion.
> 
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/disk/cryptodisk.c | 12 +++++-------
>  include/grub/disk.h         |  7 +++++++
>  2 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index b87925ad3..b3610a1b6 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -761,10 +761,10 @@ grub_cryptodisk_read (grub_disk_t disk,
> grub_disk_addr_t sector, PRIxGRUB_UINT64_T " with offset of %"
> PRIuGRUB_UINT64_T "\n", size, sector, dev->offset);
>  
> +  sector = 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),
> +			0, size << disk->log_sector_size, buf);
>    if (err)
>      {
>        grub_dprintf ("cryptodisk", "grub_disk_read failed with error
> %d\n", err); @@ -821,12 +821,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);
>  

Re: [CRYPTO-LUKS v1 03/19] cryptodisk: Incorrect calculation of start sector for grub_disk_read in grub_cryptodisk_read.

[Glenn Washburn]

On Mon, 24 Aug 2020 07:10:51 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> On Sun, Aug 23, 2020 at 11:31:46PM -0500, Glenn Washburn wrote:
> > On Sun, 23 Aug 2020 12:39:03 +0200
> > Patrick Steinhardt <ps@pks.im> wrote:
> > 
...
> > > So the fix does seem correct to me, but I think it's incomplete as
> > > we'd have to do the same for `grub_cryptodisk_write`.
> > 
> > Yep, good catch. I didn't even think to check for
> > grub_cryptodisk_write since I tend to think of grub as read-only.
> > I'm actually not sure how to trigger a disk write in grub.  The
> > only thing that comes to mind is I think there's a way to set some
> > partition flags.  
> 
> I'd assume that things like the grub.env file get updated based e.g.
> on the last chosen boot entry. Which would require support to write to
> disk. But I honestly don't now and have never used write-functionality
> in GRUB.
> 
> Do you want to update your patch to also fix the write part? I'd then
> pull it into a v2 of my patch series for v2.06 fixes.

I sent a single updated patch as opposed to resending the whole patch
set.  I hope that was what was expected.

> > > Out of curiosity: did you test these changes?
> > 
> > Yes, these changes have definitely been tested.  In fact, I found
> > these bugs precisely because I was trying to get grub to decrypt my
> > LUKS2 device which has a 4096 byte sector size.  Feeling like I
> > wasn't getting much traction on my patches, I wrote the
> > CRYPTOMOUNT-TEST patch series to allow independent verification of
> > the bugs and my fixes. There are a series of tests for block sizes
> > 1024, 2048, and 4096 which all are successful for me (all the
> > sector size fixes are needed).
> 
> I'll have to have a look at them. Improved testability is definitely
> something I'm interested in, mostly because I found the manual
> compile-boot-test cycle quite unwieldy, even with virtualization.

When you get around to them, here are some tips I learned.  The full
test suite takes forever to run, so I only run the cryptomount test
using the following command in project root.  Also you'll need to be
root because the tests use dm-crypt to open generated LUKS volumes.  If
you know of a decent way to avoid this, I'd love to hear about it.

  sudo env TESTS="grub_cmd_cryptomount" TEST_SUITE_LOG=partial.log \
          make -e check VERBOSE=yes SUBDIRS=

Glenn

[CRYPTO-LUKS v3 03/19] cryptodisk: Incorrect calculation of start sector for grub_disk_read in grub_cryptodisk_read.

[Glenn Washburn]

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

[CRYPTO-LUKS v1 04/19] cryptodisk: Add more verbosity when reading/writing cryptodisks.

[Glenn Washburn]

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

[CRYPTO-LUKS v1 05/19] luks2: Add support for LUKS2 in (proc)/luks_script

[Glenn Washburn]

The sector size in bytes is added to each line and it is allowed to be 4
decimal digits long, which covers the most common cases of 512 and 4096 byte
sectors. The size allocation is updated to reflect this additional field,
allow up to 4 characters and 1 space added.

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

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index c21be7d52..847337072 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1210,12 +1210,15 @@ 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_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' */
+	size += 5 + 8 + 5 + 20 + 4 + 16 + 1;
 	if (i->essiv_hash)
 	  size += grub_strlen (i->essiv_hash->name);
 	size += i->keysize * 2;
@@ -1228,16 +1231,17 @@ 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_strcmp (i->modname, "luks") == 0 ||
+	grub_strcmp (i->modname, "luks2") == 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

[CRYPTO-LUKS v1 06/19] luks2: Rename source disk variabled named 'disk' to 'source' as in luks.c.

[Glenn Washburn]

This makes it more obvious to the reader that the disk referred to is the
source disk, as opposed to say the disk holding the cryptodisk.

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

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index e3ff7c83d..9f58780a2 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -408,7 +408,7 @@ luks2_verify_key (grub_luks2_digest_t *d, grub_uint8_t *candidate_key,
 
 static grub_err_t
 luks2_decrypt_key (grub_uint8_t *out_key,
-		   grub_disk_t disk, grub_cryptodisk_t crypt,
+		   grub_disk_t source, grub_cryptodisk_t crypt,
 		   grub_luks2_keyslot_t *k,
 		   const grub_uint8_t *passphrase, grub_size_t passphraselen)
 {
@@ -484,7 +484,7 @@ luks2_decrypt_key (grub_uint8_t *out_key,
     }
 
   grub_errno = GRUB_ERR_NONE;
-  ret = grub_disk_read (disk, 0, k->area.offset, k->area.size, split_key);
+  ret = grub_disk_read (source, 0, k->area.offset, k->area.size, split_key);
   if (ret)
     {
       grub_error (GRUB_ERR_IO, "Read error: %s\n", grub_errmsg);
@@ -523,7 +523,7 @@ luks2_decrypt_key (grub_uint8_t *out_key,
 }
 
 static grub_err_t
-luks2_recover_key (grub_disk_t disk,
+luks2_recover_key (grub_disk_t source,
 		   grub_cryptodisk_t crypt)
 {
   grub_uint8_t candidate_key[GRUB_CRYPTODISK_MAX_KEYLEN];
@@ -538,7 +538,7 @@ luks2_recover_key (grub_disk_t disk,
   grub_json_t *json = NULL, keyslots;
   grub_err_t ret;
 
-  ret = luks2_read_header (disk, &header);
+  ret = luks2_read_header (source, &header);
   if (ret)
     return ret;
 
@@ -547,7 +547,7 @@ luks2_recover_key (grub_disk_t disk,
       return GRUB_ERR_OUT_OF_MEMORY;
 
   /* Read the JSON area. */
-  ret = grub_disk_read (disk, 0, grub_be_to_cpu64 (header.hdr_offset) + sizeof (header),
+  ret = grub_disk_read (source, 0, grub_be_to_cpu64 (header.hdr_offset) + sizeof (header),
 			grub_be_to_cpu64 (header.hdr_size) - sizeof (header), json_header);
   if (ret)
       goto err;
@@ -564,10 +564,10 @@ luks2_recover_key (grub_disk_t disk,
     }
 
   /* Get the passphrase from the user. */
-  if (disk->partition)
-    part = grub_partition_get_name (disk->partition);
-  grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), disk->name,
-		disk->partition ? "," : "", part ? : "",
+  if (source->partition)
+    part = grub_partition_get_name (source->partition);
+  grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
+		source->partition ? "," : "", part ? : "",
 		crypt->uuid);
   if (!grub_password_get (passphrase, MAX_PASSPHRASE))
     {
@@ -602,11 +602,11 @@ 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 (source) - crypt->offset;
       else
 	crypt->total_length = grub_strtoull (segment.size, NULL, 10);
 
-      ret = luks2_decrypt_key (candidate_key, disk, crypt, &keyslot,
+      ret = luks2_decrypt_key (candidate_key, source, crypt, &keyslot,
 			       (const grub_uint8_t *) passphrase, grub_strlen (passphrase));
       if (ret)
 	{
-- 
2.25.1

[CRYPTO-LUKS v1 07/19] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors.

[Glenn Washburn]

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 | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 9f58780a2..41329f745 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,14 @@ luks2_recover_key (grub_disk_t source,
       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 (source) - crypt->offset;
+	/*
+	 * Convert to cryptodisk sized sectors from source disk sized sectors
+	 * before subtracting the offset, which is in cryptodisk sized sectors.
+	 */
+	crypt->total_length = (grub_disk_get_size (source) >> (crypt->log_sector_size - source->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, source, crypt, &keyslot,
 			       (const grub_uint8_t *) passphrase, grub_strlen (passphrase));
-- 
2.25.1

Re: [CRYPTO-LUKS v1 07/19] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors.

[Patrick Steinhardt]

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

On Fri, Jul 31, 2020 at 07:01:48AM -0500, Glenn Washburn wrote:
> 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.

Is this actually true in all cases?

```
grub_uint64_t
grub_disk_get_size (grub_disk_t disk)
{
  if (disk->partition)
    return grub_partition_get_len (disk->partition);
  else if (disk->total_sectors != GRUB_DISK_SIZE_UNKNOWN)
    return disk->total_sectors << (disk->log_sector_size - GRUB_DISK_SECTOR_BITS);
  else
    return GRUB_DISK_SIZE_UNKNOWN;
}

static inline grub_uint64_t
grub_partition_get_len (const grub_partition_t p)
{
  return p->len;
}

/* Partition description.  */
struct grub_partition
{
...
  /* The length in sector units.  */
  grub_uint64_t len;
};
```

So for partitions, this does seem correct as it'll return the number of
sectors. But if it's not a partition, we return `disk->total_sectors`
shifted by the sector size. Which is the length in bytes, right? So to
me, it does rather feel like `grub_disk_get_size` is actually wrong.
Naturally, there's no documentation stating what the function is
supposed to do, but judging by its name it should've returned the number
of bytes and not the number of sectors.

I think the fix should rather be:

diff --git a/grub-core/kern/disk.c b/grub-core/kern/disk.c
index ffb09c8ee..f57cf99a1 100644
--- a/grub-core/kern/disk.c
+++ b/grub-core/kern/disk.c
@@ -536,7 +536,7 @@ grub_uint64_t
 grub_disk_get_size (grub_disk_t disk)
 {
   if (disk->partition)
-    return grub_partition_get_len (disk->partition);
+    return grub_partition_get_len (disk->partition) << (disk->log_sector_size - GRUB_DISK_SECTOR_BITS);
   else if (disk->total_sectors != GRUB_DISK_SIZE_UNKNOWN)
     return disk->total_sectors << (disk->log_sector_size - GRUB_DISK_SECTOR_BITS);
   else

I'll send a patch to address this finding.

Patrick


> 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 | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> index 9f58780a2..41329f745 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,14 @@ luks2_recover_key (grub_disk_t source,
>        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 (source) - crypt->offset;
> +	/*
> +	 * Convert to cryptodisk sized sectors from source disk sized sectors
> +	 * before subtracting the offset, which is in cryptodisk sized sectors.
> +	 */
> +	crypt->total_length = (grub_disk_get_size (source) >> (crypt->log_sector_size - source->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, source, crypt, &keyslot,
>  			       (const grub_uint8_t *) passphrase, grub_strlen (passphrase));
> -- 
> 2.25.1
> 

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

[CRYPTO-LUKS v1 08/19] cryptodisk, luks: Allow special processing for comparing UUIDs.

[Glenn Washburn]

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 847337072..e7a4b8082 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 41329f745..7f5deb469 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: [CRYPTO-LUKS v1 08/19] cryptodisk, luks: Allow special processing for comparing UUIDs.

[Patrick Steinhardt]

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

On Fri, Jul 31, 2020 at 07:01:49AM -0500, Glenn Washburn wrote:
> 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 847337072..e7a4b8082 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)

I don't think the header's UUID is NUL-terminated. So if
`strlen(check_uuid) > sizeof(header.uuid)`, we'll overflow the header.
We should probably add a size parameter to `grub_uuidcasecmp` to fix
this.

>      {
> -      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 41329f745..7f5deb469 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;

Same over here.

Patrick

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

[CRYPTO-LUKS v1 09/19] cryptodisk: Unregister cryptomount command when removing module.

[Glenn Washburn]

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 e7a4b8082..263ca2e1a 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1319,5 +1319,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

[CRYPTO-LUKS v1 10/19] fs: Fix block lists not being able to address to end of disk sometimes.

[Glenn Washburn]

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.

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

[CRYPTO-LUKS v1 11/19] 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 | 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 263ca2e1a..014a8a95b 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 7f5deb469..5e4ce858e 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 area always uses 512-byte 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

[CRYPTO-LUKS v1 12/19] cryptodisk: Rename total_length field in grub_cryptodisk_t to total_sectors.

[Glenn Washburn]

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 014a8a95b..9399849b6 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 5e4ce858e..28d9fe0e0 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -610,10 +610,10 @@ luks2_recover_key (grub_disk_t source,
 	 * Convert to cryptodisk sized sectors from source disk sized sectors
 	 * before subtracting the offset, which is in cryptodisk sized sectors.
 	 */
-	crypt->total_length = (grub_disk_get_size (source) >> (crypt->log_sector_size - source->log_sector_size))
+	crypt->total_sectors = (grub_disk_get_size (source) >> (crypt->log_sector_size - source->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, source, 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

[CRYPTO-LUKS v1 13/19] fs: Allow number of blocks in block list to be optional, where length will be defaulted to the length of the device.

[Glenn Washburn]

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

[CRYPTO-LUKS v1 14/19] loopback: Add procfs entry 'loopbacks' to output configured loopback devices.

[Glenn Washburn]

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

[CRYPTO-LUKS v1 15/19] cryptodisk, luks2: Add header line to procfs entry and crypto and source device names.

[Glenn Washburn]

A header line is added to luks_script for easier comprehension of fields by
user. And the crypto disk name and source device names are added.

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

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 9399849b6..2c6e73b39 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1206,35 +1206,48 @@ 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_strcmp (i->modname, "luks") == 0 ||
-	grub_strcmp (i->modname, "luks2") == 0)
+    if (i->id >= MAX_ID_PRINT)
+      {
+	*sz = sizeof (errmsg);
+	return errmsg;
+      }
+    else if (grub_strcmp (i->modname, "luks") == 0 ||
+	     grub_strcmp (i->modname, "luks2") == 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' */
-	size += 5 + 8 + 5 + 20 + 4 + 16 + 1;
+	/* spaces + maxidlen + mode + mode_iv + offset + sector size + ??? + '\n' */
+	size += 7 + sizeof (STR(MAX_ID_PRINT)) + 5 + 8 + 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_strcmp (i->modname, "luks") == 0 ||
@@ -1244,6 +1257,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

Re: [CRYPTO-LUKS v1 15/19] cryptodisk,luks2: Add header line to procfs entry and crypto and source device names.

[Patrick Steinhardt]

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

On Fri, Jul 31, 2020 at 07:01:56AM -0500, Glenn Washburn wrote:
> A header line is added to luks_script for easier comprehension of fields by
> user. And the crypto disk name and source device names are added.
> 
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/disk/cryptodisk.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 9399849b6..2c6e73b39 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -1206,35 +1206,48 @@ 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");

Not sure, but this seems like a backwards-incompatible change to me,
right? In case anybody has been looping over this list, he'll not know
to skip the first line.

Patrick

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

Re: [CRYPTO-LUKS v1 15/19] cryptodisk,luks2: Add header line to procfs entry and crypto and source device names.

[Glenn Washburn]

On Fri, 31 Jul 2020 17:37:46 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> On Fri, Jul 31, 2020 at 07:01:56AM -0500, Glenn Washburn wrote:
> > A header line is added to luks_script for easier comprehension of
> > fields by user. And the crypto disk name and source device names
> > are added.
> > 
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  grub-core/disk/cryptodisk.c | 28 ++++++++++++++++++++++------
> >  1 file changed, 22 insertions(+), 6 deletions(-)
> > 
> > diff --git a/grub-core/disk/cryptodisk.c
> > b/grub-core/disk/cryptodisk.c index 9399849b6..2c6e73b39 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -1206,35 +1206,48 @@ 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");
> 
> Not sure, but this seems like a backwards-incompatible change to me,
> right? In case anybody has been looping over this list, he'll not know
> to skip the first line.
> 
> Patrick

Yes, it is backwards-incompatible. How likely do you think it is that
anyone would be doing pragmatically reading that data? As far as I know
the grub environment isn't powerful enough to do that.  I doubt anyone
writing modules would, since they'd just use the internal
data structures instead.  So that leaves getting that data through the
serial port (am I missing something?). Perhaps as part of some automated
test harness?  I've also added extra fields to each line, which would
be considered backwards-incompatible.  Also, this change should make
future additions more backward-incompatible proof as the header fields
make the output more like an associative array and you can choose your
fields by field name.

Glenn

[CRYPTO-LUKS v1 16/19] cryptodisk: Add a couple comments noting the usage of a couple fields in grub_cryptodisk_t as is done for grub_disk_t.

[Glenn Washburn]

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

[CRYPTO-LUKS v1 17/19] luks2: Ensure that bit fields of grub_luks2_digest_t in luks2_parse_digest are initialized before returning.

[Glenn Washburn]

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 28d9fe0e0..58bb4b9ef 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

[CRYPTO-LUKS v1 18/19] luks2: Fix use of incorrect index and some error messages.

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 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 58bb4b9ef..09584c84c 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -257,54 +257,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.25.1

[CRYPTO-LUKS v1 19/19] cryptodisk: Rename offset in grub_cryptodisk_t to offset_sectors to improve readability.

[Glenn Washburn]

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

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 2c6e73b39..4921d031d 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -761,12 +761,12 @@ grub_cryptodisk_read (grub_disk_t disk, grub_disk_addr_t sector,
 		"Reading %" PRIuGRUB_SIZE " sectors from sector 0x%"
 		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,
+		size, sector, dev->offset_sectors, 1U << disk->log_sector_size,
 		dev->source_disk->name);
 
   err = grub_disk_read (dev->source_disk,
-			((sector + dev->offset) << (disk->log_sector_size
-						   - GRUB_DISK_SECTOR_BITS)), 0,
+			((sector + dev->offset_sectors) << (disk->log_sector_size
+							   - GRUB_DISK_SECTOR_BITS)), 0,
 			size << disk->log_sector_size, buf);
   if (err)
     {
@@ -813,7 +813,7 @@ grub_cryptodisk_write (grub_disk_t disk, grub_disk_addr_t sector,
 		"Writing %" PRIuGRUB_SIZE " sectors to sector 0x%"
 		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,
+		size, sector, dev->offset_sectors, 1U << disk->log_sector_size,
 		dev->source_disk->name);
 
   gcry_err = grub_cryptodisk_endecrypt (dev, (grub_uint8_t *) tmp,
@@ -831,7 +831,7 @@ grub_cryptodisk_write (grub_disk_t disk, grub_disk_addr_t sector,
     err = grub_disk_write_weak (dev->source_disk,
 				(sector << (disk->log_sector_size
 					    - GRUB_DISK_SECTOR_BITS))
-				+ dev->offset,
+				+ dev->offset_sectors,
 				0, size << disk->log_sector_size, tmp);
   else
     err = grub_error (GRUB_ERR_BUG, "disk.mod not loaded");
@@ -1262,7 +1262,7 @@ luks_script_get (grub_size_t *sz)
 	*ptr++ = ' ';
 	ptr = grub_stpcpy (ptr, i->uuid);
 	*ptr++ = ' ';
-	ptr += grub_snprintf (ptr, 21, "%" PRIuGRUB_UINT64_T " ", i->offset);
+	ptr += grub_snprintf (ptr, 21, "%" PRIuGRUB_UINT64_T " ", i->offset_sectors);
 	ptr += grub_snprintf (ptr, 6, "%d ", 1 << i->log_sector_size);
 	for (iptr = i->cipher->cipher->name; *iptr; iptr++)
 	  *ptr++ = grub_tolower (*iptr);
diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
index 4ec875821..0175ce4c4 100644
--- a/grub-core/disk/geli.c
+++ b/grub-core/disk/geli.c
@@ -361,7 +361,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
     }
   newdev->cipher = cipher;
   newdev->secondary_cipher = secondary_cipher;
-  newdev->offset = 0;
+  newdev->offset_sectors = 0;
   newdev->source_disk = NULL;
   newdev->benbi_log = 0;
   if (grub_le_to_cpu16 (header.alg) == 0x16)
diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index b1db3d137..f825fa9a9 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -110,10 +110,10 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
   newdev = grub_zalloc (sizeof (struct grub_cryptodisk));
   if (!newdev)
       return NULL;
-  newdev->offset = grub_be_to_cpu32 (header.payloadOffset);
+  newdev->offset_sectors = grub_be_to_cpu32 (header.payloadOffset);
   newdev->source_disk = NULL;
   newdev->log_sector_size = LOG_SECTOR_SIZE;
-  newdev->total_sectors = grub_disk_get_size (disk) - newdev->offset;
+  newdev->total_sectors = grub_disk_get_size (disk) - newdev->offset_sectors;
   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 09584c84c..35f705076 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -605,7 +605,7 @@ luks2_recover_key (grub_disk_t source,
       grub_dprintf ("luks2", "Trying keyslot %"PRIuGRUB_SIZE"\n", i);
 
       /* Set up disk according to keyslot's segment. */
-      crypt->offset = grub_divmod64 (segment.offset, segment.sector_size, NULL);
+      crypt->offset_sectors = grub_divmod64 (segment.offset, segment.sector_size, NULL);
       crypt->log_sector_size = sizeof (unsigned int) * 8
 		- __builtin_clz ((unsigned int) segment.sector_size) - 1;
       if (grub_strcmp (segment.size, "dynamic") == 0)
@@ -614,7 +614,7 @@ luks2_recover_key (grub_disk_t source,
 	 * before subtracting the offset, which is in cryptodisk sized sectors.
 	 */
 	crypt->total_sectors = (grub_disk_get_size (source) >> (crypt->log_sector_size - source->log_sector_size))
-			       - crypt->offset;
+			       - crypt->offset_sectors;
       else
 	crypt->total_sectors = grub_strtoull (segment.size, NULL, 10) >> crypt->log_sector_size;
 
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index 8d3284aad..1a78f6f58 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -70,7 +70,7 @@ struct grub_cryptodisk
   /* 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;
+  grub_disk_addr_t offset_sectors;
   /* Total number of encrypted sectors of size (1<<log_sector_size) */
   grub_disk_addr_t total_sectors;
   grub_disk_t source_disk;
-- 
2.25.1

Re: [CRYPTO-LUKS v1 00/19] Fixes and improvements for cryptodisks+luks2 and a few other things.

[Patrick Steinhardt]

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

On Fri, Jul 31, 2020 at 07:01:41AM -0500, Glenn Washburn wrote:
> I've changed the name of the patch series to make it more self-explanatory, but
> still incremented the version number.  All of Patrick's suggestions have been
> added in.  I've added 2 new patches, one suggested by Patrick for rename
> cryptodisk's offset member to offset_sectors. And another to rename 'disk' to
> 'source' where appropriate in luks2.c.

Thanks for adressing my feedback! I'm not authoritative in this project
in any way, but I guess you'll have an easier time to get things merged
if you split this up into multiple patch series as there's unrelated
things you're doing here. E.g. something like the following might work
and make the review load a bit more manageable.

Font fix.

>   configure: Add Ubuntu dejavu font path.

Profcs changes:

>   loopback: Add procfs entry 'loopbacks' to output configured loopback
>     devices.
>   cryptodisk,luks2: Add header line to procfs entry and crypto and
>     source device names.

Cryptodisk related changes independent of LUKS:

>   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.
>   cryptodisk: Rename offset in grub_cryptodisk_t to offset_sectors to
>     improve readability.
>   cryptodisk: Add a couple comments noting the usage of a couple fields
>     in grub_cryptodisk_t as is done for grub_disk_t.
>   cryptodisk: Unregister cryptomount command when removing module.
>   cryptodisk: Properly handle non-512 byte sized sectors.
>   cryptodisk: Rename total_length field in grub_cryptodisk_t to
>     total_sectors.

LUKS-specfic changes:

>   luks2: Rename source disk variabled named 'disk' to 'source' as in
>     luks.c.
>   luks2: Fix use of incorrect index and some error messages.
>   luks2: Ensure that bit fields of grub_luks2_digest_t in
>     luks2_parse_digest are initialized before returning.
>   luks2: 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.

fs-specific changes:

>   fs: Fix block lists not being able to address to end of disk
>     sometimes.
>   fs: Allow number of blocks in block list to be optional, where length
>     will be defaulted to the length of the device.

Patrick

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