[PATCH v9 0/6] Cryptodisk fixes for v2.06 redux

[PATCH v9 0/6] Cryptodisk fixes for v2.06 redux

[Glenn Washburn]

This patch series has been rebased onto latest master. The first patch is new
and fixes issue where total_sectors was incorrectly converted from source
disk sized sectors, when it should've been converted from grub native sector
size. The second patch was separated from the previous patch 14. Patch 3 is
most of the previous patch 14 with suggestions incorporated and some minor
debug message changes.

Glenn

Glenn Washburn (6):
  luks2: Convert to crypt sectors from grub native sectors
  luks2: Do not handle disks of size GRUB_DISK_SIZE_UNKNOWN for now
  luks2: Better error handling when setting up the cryptodisk
  mips: Enable __clzdi2()
  misc: Add grub_log2ull macro for calculating log base 2 of 64-bit
    integers
  luks2: Use grub_log2ull to calculate log_sector_size and improve
    readability

 grub-core/disk/luks2.c       | 93 ++++++++++++++++++++++++++++++++++--
 grub-core/kern/compiler-rt.c |  2 +-
 include/grub/compiler-rt.h   |  2 +-
 include/grub/disk.h          | 19 ++++++++
 include/grub/misc.h          |  3 ++
 5 files changed, 112 insertions(+), 7 deletions(-)

Range-diff against v8:
-:  --------- > 1:  2254f177d luks2: Convert to crypt sectors from grub native sectors
-:  --------- > 2:  1b52233d1 luks2: Do not handle disks of size GRUB_DISK_SIZE_UNKNOWN for now
1:  c32b542f2 ! 3:  ceaf95eb1 luks2: Better error handling when setting up the cryptodisk
    @@ Metadata
      ## Commit message ##
         luks2: Better error handling when setting up the cryptodisk
     
    -    First, check to make sure that source disk has a known size. If not, print
    -    debug message and return error. There are 4 cases where
    -    GRUB_DISK_SIZE_UNKNOWN is set (biosdisk, obdisk, ofdisk, and uboot), and in
    -    all those cases processing continues. So this is probably a bit
    -    conservative. However, 3 of the cases seem pathological, and the other,
    -    biosdisk, happens when booting from a cd. Since I doubt booting from a LUKS2
    -    volume on a cd is a big use case, we'll error until someone complains.
    -
         Do some sanity checking on data coming from the luks header. If segment.size
         is "dynamic", verify that the offset is not past the end of disk. Otherwise,
         check for errors from grub_strtoull when converting segment size from
    @@ Commit message
     
      ## grub-core/disk/luks2.c ##
     @@ grub-core/disk/luks2.c: luks2_recover_key (grub_disk_t source,
    -       goto err;
    -     }
    - 
    -+  if (source->total_sectors == GRUB_DISK_SIZE_UNKNOWN)
    -+    {
    -+      /* FIXME: Allow use of source disk, and maybe cause errors in read. */
    -+      grub_dprintf ("luks2", "Source disk %s has an unknown size, "
    -+			     "conservatively returning error\n", source->name);
    -+      ret = grub_error (GRUB_ERR_BUG, "Unknown size of luks2 source device");
    -+      goto err;
    -+    }
    -+
        /* Try all keyslot */
        for (json_idx = 0; json_idx < size; json_idx++)
          {
    @@ grub-core/disk/luks2.c: luks2_recover_key (grub_disk_t source,
            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;
    -+      /* Set to the source disk size, which is the maximum we allow. */
    -+      max_crypt_sectors = grub_disk_convert_sector(source,
    -+						   source->total_sectors,
    -+						   crypt->log_sector_size);
    ++      /* Set to the source disk/partition size, which is the maximum we allow. */
    ++      max_crypt_sectors = grub_disk_native_sectors(source);
    ++      max_crypt_sectors = grub_convert_sector(max_crypt_sectors,
    ++					      GRUB_DISK_SECTOR_BITS,
    ++					      crypt->log_sector_size);
     +
     +      if (max_crypt_sectors < crypt->offset_sectors)
     +	{
    @@ grub-core/disk/luks2.c: luks2_recover_key (grub_disk_t source,
     +	}
     +
            if (grub_strcmp (segment.size, "dynamic") == 0)
    --	crypt->total_sectors = (grub_disk_native_sectors (source) >> (crypt->log_sector_size - source->log_sector_size))
    +-	crypt->total_sectors = (grub_disk_native_sectors (source) >> (crypt->log_sector_size - GRUB_DISK_SECTOR_BITS))
     -			       - crypt->offset_sectors;
     +	crypt->total_sectors = max_crypt_sectors - crypt->offset_sectors;
            else
    @@ grub-core/disk/luks2.c: luks2_recover_key (grub_disk_t source,
     +	  grub_errno = GRUB_ERR_NONE;
     +	  /* Convert segment.size to sectors, rounding up to nearest sector */
     +	  crypt->total_sectors = grub_strtoull (segment.size, NULL, 10);
    -+	  crypt->total_sectors = ALIGN_UP (crypt->total_sectors,
    -+					   1 << crypt->log_sector_size);
    -+	  crypt->total_sectors >>= crypt->log_sector_size;
     +
     +	  if (grub_errno == GRUB_ERR_NONE)
    -+	    ;
    ++	    {
    ++	      crypt->total_sectors = ALIGN_UP (crypt->total_sectors,
    ++					       1 << crypt->log_sector_size);
    ++	      crypt->total_sectors >>= crypt->log_sector_size;
    ++	    }
     +	  else if (grub_errno == GRUB_ERR_BAD_NUMBER)
     +	    {
     +	      grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\" size"
    -+				     " \"%s\" is not a parsable number\n",
    ++				     " \"%s\" is not a parsable number,"
    ++				     " skipping keyslot\n",
     +				     segment.idx, segment.size);
     +	      continue;
     +	    }
    @@ grub-core/disk/luks2.c: luks2_recover_key (grub_disk_t source,
     +	       * There was an overflow in parsing segment.size, so disk must
     +	       * be very large or the string is incorrect.
     +	       */
    ++	      /*
    ++	       * TODO: Allow reading of at least up max_crypt_sectors. Really,
    ++	       * its very unlikely one would be booting from such a large drive
    ++	       * anyway. Use another smaller LUKS boot device.
    ++	       */
     +	      grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\" size"
     +				     " %s overflowed 64-bit unsigned integer,"
    -+				     " the end of the crypto device will be"
    -+				     " inaccessible\n",
    ++				     " skipping keyslot\n",
     +				     segment.idx, segment.size);
    -+	      if (crypt->total_sectors > max_crypt_sectors)
    -+		crypt->total_sectors = max_crypt_sectors;
    ++	      continue;
     +	    }
     +	}
     +
    @@ include/grub/disk.h: typedef struct grub_disk_memberlist *grub_disk_memberlist_t
      /* Return value of grub_disk_native_sectors() in case disk size is unknown. */
      #define GRUB_DISK_SIZE_UNKNOWN	 0xffffffffffffffffULL
      
    -+/* Convert sector number from disk sized sectors to a log-size sized sector. */
    ++/* Convert sector number from one sector size to another. */
     +static inline grub_disk_addr_t
    -+grub_disk_convert_sector (grub_disk_t disk,
    -+			  grub_disk_addr_t sector,
    -+			  grub_size_t log_sector_size)
    ++grub_convert_sector (grub_disk_addr_t sector,
    ++		     grub_size_t log_sector_size_from,
    ++		     grub_size_t log_sector_size_to)
     +{
    -+  if (disk->log_sector_size < log_sector_size)
    ++  if (log_sector_size_from == log_sector_size_to)
    ++    return sector;
    ++  else if (log_sector_size_from < log_sector_size_to)
     +    {
    -+      sector = ALIGN_UP (sector, 1 << (log_sector_size / disk->log_sector_size));
    -+      return sector >> (log_sector_size - disk->log_sector_size);
    ++      sector = ALIGN_UP (sector, 1 << (log_sector_size_to - log_sector_size_from));
    ++      return sector >> (log_sector_size_to - log_sector_size_from);
     +    }
     +  else
    -+    return sector << (disk->log_sector_size - log_sector_size);
    ++    return sector << (log_sector_size_from - log_sector_size_to);
     +}
     +
      /* Convert to GRUB native disk sized sector from disk sized sector. */
2:  f53c505fc = 4:  62d4a6c2e mips: Enable __clzdi2()
3:  9ed86739f = 5:  02c702fbf misc: Add grub_log2ull macro for calculating log base 2 of 64-bit integers
4:  626377092 ! 6:  30dfc9ee5 luks2: Use grub_log2ull to calculate log_sector_size and improve readability
    @@ grub-core/disk/luks2.c: luks2_recover_key (grub_disk_t source,
     -      crypt->log_sector_size = sizeof (unsigned int) * 8
     -		- __builtin_clz ((unsigned int) segment.sector_size) - 1;
     +      crypt->log_sector_size = grub_log2ull (segment.sector_size);
    -       /* Set to the source disk size, which is the maximum we allow. */
    -       max_crypt_sectors = grub_disk_convert_sector(source,
    - 						   source->total_sectors,
    +       /* Set to the source disk/partition size, which is the maximum we allow. */
    +       max_crypt_sectors = grub_disk_native_sectors(source);
    +       max_crypt_sectors = grub_convert_sector(max_crypt_sectors,
-- 
2.27.0

[PATCH v9 1/6] luks2: Convert to crypt sectors from grub native sectors

[Glenn Washburn]

The function grub_disk_native_sectors(source) returns the number of sectors
of source in grub native (512-byte) sectors, not source sized sectors. So
the conversion needs to use GRUB_DISK_SECTOR_BITS. the grub native sector
size.

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

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 8d2457557..8c1156dd0 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -631,7 +631,7 @@ 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_sectors = (grub_disk_native_sectors (source) >> (crypt->log_sector_size - source->log_sector_size))
+	crypt->total_sectors = (grub_disk_native_sectors (source) >> (crypt->log_sector_size - GRUB_DISK_SECTOR_BITS))
 			       - crypt->offset_sectors;
       else
 	crypt->total_sectors = grub_strtoull (segment.size, NULL, 10) >> crypt->log_sector_size;
-- 
2.27.0

[PATCH v9 2/6] luks2: Do not handle disks of size GRUB_DISK_SIZE_UNKNOWN for now

[Glenn Washburn]

Check to make sure that source disk has a known size. If not, print debug
message and return error. There are 4 cases where GRUB_DISK_SIZE_UNKNOWN is
set (biosdisk, obdisk, ofdisk, and uboot), and in all those cases processing
continues. So this is probably a bit conservative. However, 3 of the cases
seem pathological, and the other, biosdisk, happens when booting from a cd.
Since I doubt booting from a LUKS2 volume on a cd is a big use case, we'll
error until someone complains.

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

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 8c1156dd0..d30c3bfff 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -600,6 +600,15 @@ luks2_recover_key (grub_disk_t source,
       goto err;
     }
 
+  if (grub_disk_native_sectors(source) == GRUB_DISK_SIZE_UNKNOWN)
+    {
+      /* FIXME: Allow use of source disk, and maybe cause errors in read. */
+      grub_dprintf ("luks2", "Source disk %s has an unknown size, "
+			     "conservatively returning error\n", source->name);
+      ret = grub_error (GRUB_ERR_BUG, "Unknown size of luks2 source device");
+      goto err;
+    }
+
   /* Try all keyslot */
   for (json_idx = 0; json_idx < size; json_idx++)
     {
-- 
2.27.0

[PATCH v9 3/6] luks2: Better error handling when setting up the cryptodisk

[Glenn Washburn]

Do some sanity checking on data coming from the luks header. If segment.size
is "dynamic", verify that the offset is not past the end of disk. Otherwise,
check for errors from grub_strtoull when converting segment size from
string. If a GRUB_ERR_BAD_NUMBER error was returned, then the string was
not a valid parsable number, so skip the key. If GRUB_ERR_OUT_OF_RANGE was
returned, then there was an overflow in converting to a 64-bit unsigned
integer. So this could be a very large disk (perhaps large raid array).
In this case, we want to continue to try to use this key, but only allow
access up to the end of the source disk.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/luks2.c | 81 ++++++++++++++++++++++++++++++++++++++++--
 include/grub/disk.h    | 19 ++++++++++
 2 files changed, 97 insertions(+), 3 deletions(-)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index d30c3bfff..0e5061243 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -612,9 +612,14 @@ luks2_recover_key (grub_disk_t source,
   /* Try all keyslot */
   for (json_idx = 0; json_idx < size; json_idx++)
     {
+      typeof(source->total_sectors) max_crypt_sectors = 0;
+
+      grub_errno = GRUB_ERR_NONE;
       ret = luks2_get_keyslot (&keyslot, &digest, &segment, json, json_idx);
       if (ret)
 	goto err;
+      if (grub_errno != GRUB_ERR_NONE)
+	  grub_dprintf ("luks2", "Ignoring unhandled error %d from luks2_get_keyslot\n", grub_errno);
 
       if (keyslot.priority == 0)
 	{
@@ -639,11 +644,81 @@ luks2_recover_key (grub_disk_t source,
       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;
+      /* Set to the source disk/partition size, which is the maximum we allow. */
+      max_crypt_sectors = grub_disk_native_sectors(source);
+      max_crypt_sectors = grub_convert_sector(max_crypt_sectors,
+					      GRUB_DISK_SECTOR_BITS,
+					      crypt->log_sector_size);
+
+      if (max_crypt_sectors < crypt->offset_sectors)
+	{
+	  grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\" has offset"
+				 " %"PRIuGRUB_UINT64_T" which is greater than"
+				 " source disk size %"PRIuGRUB_UINT64_T","
+				 " skipping\n",
+				 segment.idx, crypt->offset_sectors,
+				 max_crypt_sectors);
+	  continue;
+	}
+
       if (grub_strcmp (segment.size, "dynamic") == 0)
-	crypt->total_sectors = (grub_disk_native_sectors (source) >> (crypt->log_sector_size - GRUB_DISK_SECTOR_BITS))
-			       - crypt->offset_sectors;
+	crypt->total_sectors = max_crypt_sectors - crypt->offset_sectors;
       else
-	crypt->total_sectors = grub_strtoull (segment.size, NULL, 10) >> crypt->log_sector_size;
+	{
+	  grub_errno = GRUB_ERR_NONE;
+	  /* Convert segment.size to sectors, rounding up to nearest sector */
+	  crypt->total_sectors = grub_strtoull (segment.size, NULL, 10);
+
+	  if (grub_errno == GRUB_ERR_NONE)
+	    {
+	      crypt->total_sectors = ALIGN_UP (crypt->total_sectors,
+					       1 << crypt->log_sector_size);
+	      crypt->total_sectors >>= crypt->log_sector_size;
+	    }
+	  else if (grub_errno == GRUB_ERR_BAD_NUMBER)
+	    {
+	      grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\" size"
+				     " \"%s\" is not a parsable number,"
+				     " skipping keyslot\n",
+				     segment.idx, segment.size);
+	      continue;
+	    }
+	  else if (grub_errno == GRUB_ERR_OUT_OF_RANGE)
+	    {
+	      /*
+	       * There was an overflow in parsing segment.size, so disk must
+	       * be very large or the string is incorrect.
+	       */
+	      /*
+	       * TODO: Allow reading of at least up max_crypt_sectors. Really,
+	       * its very unlikely one would be booting from such a large drive
+	       * anyway. Use another smaller LUKS boot device.
+	       */
+	      grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\" size"
+				     " %s overflowed 64-bit unsigned integer,"
+				     " skipping keyslot\n",
+				     segment.idx, segment.size);
+	      continue;
+	    }
+	}
+
+      if (crypt->total_sectors == 0)
+	{
+	  grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\" has zero"
+				 " sectors, skipping\n",
+				 segment.idx);
+	  continue;
+	}
+      else if (max_crypt_sectors < (crypt->offset_sectors + crypt->total_sectors))
+	{
+	  grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\" has last"
+				 " data position greater than source disk size,"
+				 " the end of the crypto device will be"
+				 " inaccessible\n",
+				 segment.idx);
+	  /* Allow decryption up to the end of the source disk. */
+	  crypt->total_sectors = max_crypt_sectors - crypt->offset_sectors;
+	}
 
       ret = luks2_decrypt_key (candidate_key, source, crypt, &keyslot,
 			       (const grub_uint8_t *) passphrase, grub_strlen (passphrase));
diff --git a/include/grub/disk.h b/include/grub/disk.h
index e08663cdb..f95aca929 100644
--- a/include/grub/disk.h
+++ b/include/grub/disk.h
@@ -27,6 +27,8 @@
 #include <grub/device.h>
 /* For NULL.  */
 #include <grub/mm.h>
+/* For ALIGN_UP.  */
+#include <grub/misc.h>
 
 /* These are used to set a device id. When you add a new disk device,
    you must define a new id for it here.  */
@@ -180,6 +182,23 @@ typedef struct grub_disk_memberlist *grub_disk_memberlist_t;
 /* Return value of grub_disk_native_sectors() in case disk size is unknown. */
 #define GRUB_DISK_SIZE_UNKNOWN	 0xffffffffffffffffULL
 
+/* Convert sector number from one sector size to another. */
+static inline grub_disk_addr_t
+grub_convert_sector (grub_disk_addr_t sector,
+		     grub_size_t log_sector_size_from,
+		     grub_size_t log_sector_size_to)
+{
+  if (log_sector_size_from == log_sector_size_to)
+    return sector;
+  else if (log_sector_size_from < log_sector_size_to)
+    {
+      sector = ALIGN_UP (sector, 1 << (log_sector_size_to - log_sector_size_from));
+      return sector >> (log_sector_size_to - log_sector_size_from);
+    }
+  else
+    return sector << (log_sector_size_from - log_sector_size_to);
+}
+
 /* 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)
-- 
2.27.0

[PATCH v9 4/6] mips: Enable __clzdi2()

[Glenn Washburn]

This patch is similiar to commit 9dab2f51e (sparc: Enable __clzsi2() and
__clzdi2()) but for MIPS target and __clzdi2 only, __clzsi2 was already
enabled.

Suggested-by: Daniel Kiper <dkiper@net-space.pl>
Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/kern/compiler-rt.c | 2 +-
 include/grub/compiler-rt.h   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/grub-core/kern/compiler-rt.c b/grub-core/kern/compiler-rt.c
index a464200c6..2057c2e0c 100644
--- a/grub-core/kern/compiler-rt.c
+++ b/grub-core/kern/compiler-rt.c
@@ -448,7 +448,7 @@ __clzsi2 (grub_uint32_t val)
 }
 #endif
 
-#if defined(__riscv) || defined(__sparc__)
+#if defined(__mips__) || defined(__riscv) || defined(__sparc__)
 int
 __clzdi2 (grub_uint64_t val)
 {
diff --git a/include/grub/compiler-rt.h b/include/grub/compiler-rt.h
index 7591980b4..17828b322 100644
--- a/include/grub/compiler-rt.h
+++ b/include/grub/compiler-rt.h
@@ -115,7 +115,7 @@ int
 EXPORT_FUNC (__clzsi2) (grub_uint32_t val);
 #endif
 
-#if defined(__riscv) || defined(__sparc__)
+#if defined(__mips__) || defined(__riscv) || defined(__sparc__)
 int
 EXPORT_FUNC (__clzdi2) (grub_uint64_t val);
 #endif
-- 
2.27.0

[PATCH v9 5/6] misc: Add grub_log2ull macro for calculating log base 2 of 64-bit integers

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 include/grub/misc.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/grub/misc.h b/include/grub/misc.h
index 780a34e90..73a514eb1 100644
--- a/include/grub/misc.h
+++ b/include/grub/misc.h
@@ -482,4 +482,7 @@ void EXPORT_FUNC(grub_real_boot_time) (const char *file,
 #define grub_max(a, b) (((a) > (b)) ? (a) : (b))
 #define grub_min(a, b) (((a) < (b)) ? (a) : (b))
 
+#define grub_log2ull(n) (GRUB_TYPE_BITS (grub_uint64_t) \
+                         - __builtin_clzll (n) - 1)
+
 #endif /* ! GRUB_MISC_HEADER */
-- 
2.27.0

[PATCH v9 6/6] luks2: Use grub_log2ull to calculate log_sector_size and improve readability

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 grub-core/disk/luks2.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 0e5061243..1ff89089e 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -642,8 +642,7 @@ luks2_recover_key (grub_disk_t source,
 
       /* Set up disk according to keyslot's segment. */
       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;
+      crypt->log_sector_size = grub_log2ull (segment.sector_size);
       /* Set to the source disk/partition size, which is the maximum we allow. */
       max_crypt_sectors = grub_disk_native_sectors(source);
       max_crypt_sectors = grub_convert_sector(max_crypt_sectors,
-- 
2.27.0

Re: [PATCH v9 1/6] luks2: Convert to crypt sectors from grub native sectors

[Daniel Kiper]

On Tue, Dec 15, 2020 at 05:31:06PM -0600, Glenn Washburn wrote:
> The function grub_disk_native_sectors(source) returns the number of sectors
> of source in grub native (512-byte) sectors, not source sized sectors. So
> the conversion needs to use GRUB_DISK_SECTOR_BITS. the grub native sector
> size.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

Re: [PATCH v9 2/6] luks2: Do not handle disks of size GRUB_DISK_SIZE_UNKNOWN for now

[Daniel Kiper]

On Tue, Dec 15, 2020 at 05:31:07PM -0600, Glenn Washburn wrote:
> Check to make sure that source disk has a known size. If not, print debug
> message and return error. There are 4 cases where GRUB_DISK_SIZE_UNKNOWN is
> set (biosdisk, obdisk, ofdisk, and uboot), and in all those cases processing
> continues. So this is probably a bit conservative. However, 3 of the cases
> seem pathological, and the other, biosdisk, happens when booting from a cd.
> Since I doubt booting from a LUKS2 volume on a cd is a big use case, we'll
> error until someone complains.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

Re: [PATCH v9 3/6] luks2: Better error handling when setting up the cryptodisk

[Daniel Kiper]

On Tue, Dec 15, 2020 at 05:31:08PM -0600, Glenn Washburn wrote:
> Do some sanity checking on data coming from the luks header. If segment.size
> is "dynamic", verify that the offset is not past the end of disk. Otherwise,
> check for errors from grub_strtoull when converting segment size from
> string. If a GRUB_ERR_BAD_NUMBER error was returned, then the string was
> not a valid parsable number, so skip the key. If GRUB_ERR_OUT_OF_RANGE was
> returned, then there was an overflow in converting to a 64-bit unsigned
> integer. So this could be a very large disk (perhaps large raid array).
> In this case, we want to continue to try to use this key, but only allow
> access up to the end of the source disk.

Last sentence looks wrong taking into account code below. I can fix it
before committing...

> Signed-off-by: Glenn Washburn <development@efficientek.com>

Otherwise Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

Re: [PATCH v9 4/6] mips: Enable __clzdi2()

[Daniel Kiper]

On Tue, Dec 15, 2020 at 05:31:09PM -0600, Glenn Washburn wrote:
> This patch is similiar to commit 9dab2f51e (sparc: Enable __clzsi2() and
> __clzdi2()) but for MIPS target and __clzdi2 only, __clzsi2 was already
> enabled.
>
> Suggested-by: Daniel Kiper <dkiper@net-space.pl>
> Signed-off-by: Glenn Washburn <development@efficientek.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

Re: [PATCH v9 3/6] luks2: Better error handling when setting up the cryptodisk

[Glenn Washburn]

On Wed, 16 Dec 2020 13:38:16 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Tue, Dec 15, 2020 at 05:31:08PM -0600, Glenn Washburn wrote:
> > Do some sanity checking on data coming from the luks header. If
> > segment.size is "dynamic", verify that the offset is not past the
> > end of disk. Otherwise, check for errors from grub_strtoull when
> > converting segment size from string. If a GRUB_ERR_BAD_NUMBER error
> > was returned, then the string was not a valid parsable number, so
> > skip the key. If GRUB_ERR_OUT_OF_RANGE was returned, then there was
> > an overflow in converting to a 64-bit unsigned integer. So this
> > could be a very large disk (perhaps large raid array). In this
> > case, we want to continue to try to use this key, but only allow
> > access up to the end of the source disk.
> 
> Last sentence looks wrong taking into account code below. I can fix it
> before committing...
> 
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> 
> Otherwise Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
> 
> Daniel

Ugh, yeah, didn't review the commit message. Please change as you see
fit. Thanks.

Glenn